[Laszlo-dev] For Review: Change 20071213-ptw-c Summary: Fix swf try/catch

[Donald Anderson]

On Dec 14, 2007, at 3:10 PM, P T Withington wrote:

> On 2007-12-14, at 14:13 EST, Donald Anderson wrote:
>
>> Approved with two comments:
>>
>> I'm only slightly worried about this -
>>
>>          short curval = bytes.getShort(patchloc);
>>          if (curval == 0) {             // XXXX
>>            ;                                    // XXXX
>>          } else if (lr.positive) {
>>            offset = offset - curval;
>>          }
>>
>> I think we can remove the // XXXX lines.  I'm thinking that
>> a previously stored 0 (is it possible?) would confuse this logic,
>> and also that we are depending on the ordering of the
>> two LabelReferences to get it right (first positive, second  
>> negative).
>> Probably won't ever be wrong from current code, but may be fragile if
>> used for something else...
>
> Disagree.  When the instruction is first written, it will write 0  
> offsets at the patchloc.  So, the test for curval == 0 is saying  
> "is this the first label we have encountered for this location?"   
> And if so, it will not do any arithmetic, it will simply write the  
> offset of that label into the patchloc.

My (paranoiac) worry was that when we write the offset during first  
label, the value we write would be 0.
Then when we process the second label, we'll look and see a 0, and  
believe it's the first time.
But the premise for this scenario (writing a 0 back) seems  
impossible, though I was trying to think of some
future use of this 'label arithmetic' code beyond try/catch that  
might tickle that.

> Now, when we enounter the second label, patchloc will be != 0, and  
> depending on whether the second label is positive or negative we  
> know which order to do the math in.  I will add this explanation as  
> a comment.
>
> Oh, and the reason for doing it this way is that since you made  
> `try` a subclass of `block`, you can only store positive offsets in  
> the patchloc.  (I think this is right, try is basically delimiting  
> 3 blocks).  So, to avoid any hijinx with getting the validation  
> wrong, I obey that convention.
>
> But!  That makes me realize there is still a bug here.  I had  
> better fetch curval as unsigned.

Right!

> And, there is yet _another_ bug.  Since a try could in theory be 3  
> maximal-length blocks, the code will assert out when it shouldn't.   
> Consider:  If the try body is maximal length, then the offset of  
> the label deliniating the body from catch or finally will be too  
> large to store in the bytestream.  I am not going to try to fix  
> that here.  I'm going to file a new bug.  Probably we need a better  
> mechanism than this to do things right, like storing this  
> computation state on the instruction representation rather than in  
> the byte stream.

Yes, two bytes is not enough.  But the code is conservative so I'm  
not worried.
If the sum of the three blocks is > 2^16 we're giving the developer a  
needed hint to refactor :-).

>
>> Second, a nit: I think a comment above class Assembler.LabelReference
>> would help (especially if I have to look at this again :-)).
>>
>>   // relative indicates a normal relative reference
>>   // if relative is false, this reference is part of label  
>> arithmetic,
>>   // that is, a difference of two labels:
>>   //    (label2 - label1)
>>   // this produces two references, one with positive=true and
>>   // one with positive=false.
>
> I'll add this.
>
>> On Dec 13, 2007, at 4:23 PM, P T Withington wrote:
>>
>>> Change 20071213-ptw-c by ptw at dueling-banjos.local on 2007-12-13  
>>> 16:14:13 EST
>>>    in /Users/ptw/OpenLaszlo/ringding-2
>>>    for http://svn.openlaszlo.org/openlaszlo/trunk
>>>
>>> Summary: Fix swf try/catch
>>>
>>> Bugs Fixed:
>>> LPP-5252 'try/catch for swf blows up in large programs'
>>>
>>> Technical Reviewer: dda at ddanderson.com (pending)
>>>
>>> Details:
>>>    Store the intermediate result as a relative value (i.e., store  
>>> the
>>>    branch offset to get to the first label, then subtract that from
>>>    the branch offset of the second label to get the difference that
>>>    you really want)
>>>
>>> Tests:
>>>    I can compile the LFC with try/catch now
>>>
>>> Files:
>>> M      WEB-INF/lps/server/src/org/openlaszlo/sc/Assembler.java
>>>
>>> Changeset: http://svn.openlaszlo.org/openlaszlo/patches/20071213- 
>>> ptw-c.tar
>>
>>
>> --
>


--

Don Anderson
Java/C/C++, Berkeley DB, systems consultant

Voice:  617-547-7881
Email:  dda at ddanderson.com
WWW:    http://www.ddanderson.com