help-smalltalk
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Help-smalltalk] Better compiled method copying


From: Gwenaël Casaccio
Subject: Re: [Help-smalltalk] Better compiled method copying
Date: Tue, 25 Jun 2013 10:03:16 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130510 Thunderbird/17.0.6

On 23/06/2013 19:45, Holger Hans Peter Freyther wrote:
On Tue, Jun 11, 2013 at 11:30:33PM +0200, Gwenaël Casaccio wrote:

Good Evening,

here some quick comments on the code and commit message.


When a compiled method is copied some literals (block and closures)
need to be fixed: they are pointing to the bad method. Also the debug
information need to be patched to point to the new literals array.
"useless"/"bad". These words carry judgement but there is no poin in
judging. I would very much prefer if you could use a more neutral tone
in your commit messages.

Could you elaborate on how you stumbled across this? When did you copy
the CompiledMethod? What was the usecase?
When a compiled method is copied the debug informations attached
to them still referenced the old compiled method, the patch fix the
references to the new compiled method. Moreover the compiled block
or block closure should also reference the new compiled method.


+GST_PACKAGE_ENABLE([Tests], [tests])
"Tests" is very generic. What about "SystemTests"?  I understand that
using SUnit is nicer than the GNU autotest framework and personally I
can understand that.
SystemTests is a better name :)


+    method: aCompiledCode [
+        <category: 'accessing'>
+
+        block method: aCompiledCode
+    ]

Sounds more like a private method to me, than 'accessing'.

+    deepCopy [
           ^super deepCopy
+            fixBlockInformation;
+            fixDebugInformation: self;
+            makeLiteralsReadOnly;
               yourself

why didn't this work? Otherwise you will need to adjust your test
case to also test for classes where isPointers evaluates to true.


+            (literals at: i) class == BlockClosure ifTrue: [
+                | new_block |
+                new_block := (literals at: i) deepCopy.
+                new_block block: new_block block copy.
+                new_block method: self.
+                literals at: i put: new_block ]. ]
can you please elaborate on these lines? First youtake a deep copy
and then you take a copy of the deep copied block? Why is that needed?

I fix the method reference of the block to the new compiled method
otherwise it will reference the older method.

+    postCopy [
+        "Private - Make a deep copy of the descriptor and literals.
+         Don't need to replace the method header and bytecodes, since they
+         are integers."
+
+        <category: 'private-copying'>
+
+        super postCopy.
+        descriptor := descriptor copy.
+        literals := literals copy.
+        self fixBlockInformation.
+        self makeLiteralsReadOnly.
+        "literals := literals deepCopy.
+         self makeLiteralsReadOnly"
time to remove the commented out code as you are doing this now? Did you
do the archology to see if these two lines have ever been enabled in the
last couple of years?

Yes

+    method: aCompiledMethod [
+       <category: 'accessing'>
it is not really accessing when you modify a class. :)

+TestCase subclass: TestCompiledMethod [
+
+    setUp [
+        <category: 'setup'>
+
+        Object subclass: #Bar.
+        Object subclass: #Foo.
a tearDown should remove this class too.
Yes but I prefer a way to create anonymous classes

+    testCopy [
...

+        self assert: old_method ~~ new_method.
+        self assert: old_method literals ~~ new_method literals.
+        self assert: old_method getHeader == new_method getHeader.
+        self assert: old_method descriptor ~~ new_method descriptor.
+        self assert: old_method descriptor debugInformation ~~ new_method 
descriptor debugInformation.
matching bytecodes could be added?
Yes

+    testDeepCopy [
+        <category: 'testing'>
can some code from the above be re-used and also with the below.
Yes it will be refactored

+    ]
+
+    testWithNewMethodClass [
+        <category: 'testing'>

thanks for the patch!


Gwen




reply via email to

[Prev in Thread] Current Thread [Next in Thread]