Bug 186765 – [Armv7] Linkbuffer: executableOffsetFor() fails for location 2

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Guillaume Emont]

On THUMB2, instructions can be 2 bytes long, and therefore are not guaranteed to be 4-aligned. This is the case for jump origins and targets, which means that the parameter of executableOffsetFor() can be the value 2, in which case executableOffsetFor() returns a value taken from before the start of the buffer. Since r231961, we see in unit tests cases where executableOffsetFor() is passed 2 as a parameter, which leads to the wrong offset being used and eventually a segfault as we generate a jump to an unpredictable address.

[Guillaume Emont]

Created attachment 342936 [details]
Patch

This seems to fix it. I'm unsure of how recordLinkOffsets()/executableOffsetFor() are supposed to work on thumb2, as they seem to assume that jump origins and destinations are 4-byte aligned, though I might be misunderstanding something, as it looks like this has been working like that since the introduction of thumb2 support.

[Mark Lam]

Comment on attachment 342936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342936&action=review

> Source/JavaScriptCore/assembler/LinkBuffer.h:300
> -        if (!location)
> +        if (location < sizeof(int32_t))

Why is it ever valid to pass a location of 2 and expect it to map to an offset of 0?  It looks to me like this change is trying to mask the bug instead of fixing it.  Please provide justification as why this is valid, or if this isn't the real issue, fix the real issue instead.

[Mark Lam]

(In reply to Guillaume Emont from comment #0)
> On THUMB2, instructions can be 2 bytes long, and therefore are not
> guaranteed to be 4-aligned. This is the case for jump origins and targets,
> which means that the parameter of executableOffsetFor() can be the value 2,
> in which case executableOffsetFor() returns a value taken from before the
> start of the buffer. 

Please explain how an offset of 2 translates to "a value taken from before the start of the buffer".  Can you point to some specific code where this happens?

> Since r231961, we see in unit tests cases where
> executableOffsetFor() is passed 2 as a parameter, which leads to the wrong
> offset being used and eventually a segfault as we generate a jump to an
> unpredictable address.

A location of 2 should be valid for ARMv7.

(In reply to Guillaume Emont from comment #1)
> Created attachment 342936 [details]
> Patch
> 
> This seems to fix it. I'm unsure of how
> recordLinkOffsets()/executableOffsetFor() are supposed to work on thumb2, as
> they seem to assume that jump origins and destinations are 4-byte aligned,
> though I might be misunderstanding something, as it looks like this has been
> working like that since the introduction of thumb2 support.

You're possibly getting closer to the real issue here.

[Mark Lam]

(In reply to Mark Lam from comment #3)
> (In reply to Guillaume Emont from comment #0)
> > On THUMB2, instructions can be 2 bytes long, and therefore are not
> > guaranteed to be 4-aligned. This is the case for jump origins and targets,
> > which means that the parameter of executableOffsetFor() can be the value 2,
> > in which case executableOffsetFor() returns a value taken from before the
> > start of the buffer. 
> 
> Please explain how an offset of 2 translates to "a value taken from before
> the start of the buffer".  Can you point to some specific code where this
> happens?

I see what you meant here.  The code at the end of executableOffsetFor() is where this can happen if the location is 2.  However, that doesn't mean the change you made is valid.

2 things you should do:
1. Find out how the location value of 2 came to be, and determine if it's a valid value or not.
2. If a location of 2 is valid, then the code in executableOffsetFor() and recordLinkOffsets() might need a different implementation for ARMv7.

The answers to the above depends on how the branch compaction feature (guarded by ENABLE(BRANCH_COMPACTION)) works.

[Guillaume Emont]

(In reply to Mark Lam from comment #3)
> (In reply to Guillaume Emont from comment #0)
> > On THUMB2, instructions can be 2 bytes long, and therefore are not
> > guaranteed to be 4-aligned. This is the case for jump origins and targets,
> > which means that the parameter of executableOffsetFor() can be the value 2,
> > in which case executableOffsetFor() returns a value taken from before the
> > start of the buffer. 
> 
> Please explain how an offset of 2 translates to "a value taken from before
> the start of the buffer".  Can you point to some specific code where this
> happens?

In executableOffsetFor() we do:

  return bitwise_cast<int32_t*>(m_assemblerStorage.buffer())[location / sizeof(int32_t) - 1]

If location is 2, our index (location / sizeof(int32_t) - 1) is -1.

> 
> > Since r231961, we see in unit tests cases where
> > executableOffsetFor() is passed 2 as a parameter, which leads to the wrong
> > offset being used and eventually a segfault as we generate a jump to an
> > unpredictable address.
> 
> A location of 2 should be valid for ARMv7.
> 
> (In reply to Guillaume Emont from comment #1)
> > Created attachment 342936 [details]
> > Patch
> > 
> > This seems to fix it. I'm unsure of how
> > recordLinkOffsets()/executableOffsetFor() are supposed to work on thumb2, as
> > they seem to assume that jump origins and destinations are 4-byte aligned,
> > though I might be misunderstanding something, as it looks like this has been
> > working like that since the introduction of thumb2 support.
> 
> You're possibly getting closer to the real issue here.

That's what I was half guessing, though I'm surprised because it's been like that for so long. I guess we got lucky. I see two ideas on how to fix this, and am unsure of which one is best yet:
 - force alignment of jumps and labels to 4 bytes on CPU(ARM_THUMB2). That might partly negate the benefits of jump compaction.
or
 - change the way recordLinkOffsets()/executableOffsetFor() work. The issue is that we want the offset table to fit where the code was, like it is currently (avoiding the use of extra memory), we would have to either store the offsets on 16 bits (not sure it's enough), or find a clever way to store them, if one exists, that can be proved to fit in the amount of memory available. The alternative would be to add an extra structure (hashtable?) to keep track of these offsets. Either way would likely make things slower for arm64, so we might want to have different code paths, e.g. depending on sizeof(InstructionType).

Any thoughts on that?

[Guillaume Emont]

(In reply to Mark Lam from comment #4)
> (In reply to Mark Lam from comment #3)
> > (In reply to Guillaume Emont from comment #0)
> > > On THUMB2, instructions can be 2 bytes long, and therefore are not
> > > guaranteed to be 4-aligned. This is the case for jump origins and targets,
> > > which means that the parameter of executableOffsetFor() can be the value 2,
> > > in which case executableOffsetFor() returns a value taken from before the
> > > start of the buffer. 
> > 
> > Please explain how an offset of 2 translates to "a value taken from before
> > the start of the buffer".  Can you point to some specific code where this
> > happens?
> 
> I see what you meant here.  The code at the end of executableOffsetFor() is
> where this can happen if the location is 2.  However, that doesn't mean the
> change you made is valid.
> 
> 2 things you should do:
> 1. Find out how the location value of 2 came to be, and determine if it's a
> valid value or not.

In LinkBuffer::copyCompactAndLinkCode() we have:

            if (jumpsToLink[i].to() >= jumpsToLink[i].from())
                target = codeOutData + jumpsToLink[i].to() - offset; // Compensate for what we have collapsed so far
            else
                target = codeOutData + jumpsToLink[i].to() - executableOffsetFor(jumpsToLink[i].to());

In that case, if the code we're linking is something like:
  2 byte instruction
  label:
  more instructions
  jmp label

We would have a .to() == 2. Indeed we see this with a few unit tests since the
InstanceOf inline cache has been added. I think that these values are valid, as
it makes sense to be able to branch to the second instruction.



> 2. If a location of 2 is valid, then the code in executableOffsetFor() and
> recordLinkOffsets() might need a different implementation for ARMv7.

Yes, it looks like that's where we're heading.
> 
> The answers to the above depends on how the branch compaction feature
> (guarded by ENABLE(BRANCH_COMPACTION)) works.

[Michael Saboff]

Comment on attachment 342936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342936&action=review

r=me

>> Source/JavaScriptCore/assembler/LinkBuffer.h:300
>> +        if (location < sizeof(int32_t))
> 
> Why is it ever valid to pass a location of 2 and expect it to map to an offset of 0?  It looks to me like this change is trying to mask the bug instead of fixing it.  Please provide justification as why this is valid, or if this isn't the real issue, fix the real issue instead.

I'm fine with this change.  Since the buffer records the offsets prior to the current location and the offsets are stored as int32_t values, this works.  You might want to add a comment that no compaction can happen before this point.

[Mark Lam]

Comment on attachment 342936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342936&action=review

>>> Source/JavaScriptCore/assembler/LinkBuffer.h:300
>>> +        if (location < sizeof(int32_t))
>> 
>> Why is it ever valid to pass a location of 2 and expect it to map to an offset of 0?  It looks to me like this change is trying to mask the bug instead of fixing it.  Please provide justification as why this is valid, or if this isn't the real issue, fix the real issue instead.
> 
> I'm fine with this change.  Since the buffer records the offsets prior to the current location and the offsets are stored as int32_t values, this works.  You might want to add a comment that no compaction can happen before this point.

For the record, I withdraw my objection after speaking with Michael and looking at the code.  Please add the comment that the reason this works is because at (offset < sizeof(int32_t)), no compaction could have happened before this point as the assembler could not have placed a branch instruction within this space that required some compaction.  Hence, 0 is the correct result.

[Guillaume Emont]

(In reply to Michael Saboff from comment #7)
> Comment on attachment 342936 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342936&action=review
> 
> r=me
> 
> >> Source/JavaScriptCore/assembler/LinkBuffer.h:300
> >> +        if (location < sizeof(int32_t))
> > 
> > Why is it ever valid to pass a location of 2 and expect it to map to an offset of 0?  It looks to me like this change is trying to mask the bug instead of fixing it.  Please provide justification as why this is valid, or if this isn't the real issue, fix the real issue instead.
> 
> I'm fine with this change.  Since the buffer records the offsets prior to
> the current location and the offsets are stored as int32_t values, this
> works.  You might want to add a comment that no compaction can happen before
> this point.


I'm wondering though, how can we be sure that executableOffsetFor() is always correct, since the offsets stored have a granularity of 4 bytes, but instruction addresses have a granularity of 2? Like, what if we have:

  ...
  jmp label1 ; 2-byte instruction
  label2:
  jmp label3
  ...

then could we have executableOffsetFor(label2) return the wrong offset? And if not, why?

[Mark Lam]

(In reply to Guillaume Emont from comment #9)
> (In reply to Michael Saboff from comment #7)
> > Comment on attachment 342936 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=342936&action=review
> > 
> > r=me
> > 
> > >> Source/JavaScriptCore/assembler/LinkBuffer.h:300
> > >> +        if (location < sizeof(int32_t))
> > > 
> > > Why is it ever valid to pass a location of 2 and expect it to map to an offset of 0?  It looks to me like this change is trying to mask the bug instead of fixing it.  Please provide justification as why this is valid, or if this isn't the real issue, fix the real issue instead.
> > 
> > I'm fine with this change.  Since the buffer records the offsets prior to
> > the current location and the offsets are stored as int32_t values, this
> > works.  You might want to add a comment that no compaction can happen before
> > this point.
> 
> 
> I'm wondering though, how can we be sure that executableOffsetFor() is
> always correct, since the offsets stored have a granularity of 4 bytes, but
> instruction addresses have a granularity of 2? Like, what if we have:
> 
>   ...
>   jmp label1 ; 2-byte instruction
>   label2:
>   jmp label3
>   ...
> 
> then could we have executableOffsetFor(label2) return the wrong offset? And
> if not, why?

Because it's storing the offset in the old (before compaction) buffer, which at minimum would have allocated more than 1 instruction per branch.

[Igalia-pontevedra EWS]

Comment on attachment 342936 [details]
Patch

Attachment 342936 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.webkit.org/results/8239453

New failing tests:
fast/files/file-constructor.html

[Igalia-pontevedra EWS]

Created attachment 343007 [details]
Archive of layout-test-results from ltilve-gtk-wk2-ews for gtk-wk2

The attached test failures were seen while running run-webkit-tests on the gtk-wk2-ews.
Bot: ltilve-gtk-wk2-ews  Port: gtk-wk2  Platform: Linux-4.16.0-0.bpo.1-amd64-x86_64-with-debian-9.4

[Guillaume Emont]

Created attachment 343041 [details]
Patch

Updated patch for landing that adds the required comment and adds a static_cast to make clang happy.

[Guillaume Emont]

(In reply to Igalia-pontevedra EWS from comment #11)
> Comment on attachment 342936 [details]
> Patch
> 
> Attachment 342936 [details] did not pass gtk-wk2-ews (gtk-wk2):
> Output: http://webkit-queues.webkit.org/results/8239453
> 
> New failing tests:
> fast/files/file-constructor.html

The issues raised by the gtk-wk2-ews should be issues with the bot, since the  code modified only gets compiled on arm and arm64.

[Guillaume Emont]

Created attachment 343148 [details]
Patch

Real patch for landing (added review)

[EWS Watchlist]

Comment on attachment 343148 [details]
Patch

Attachment 343148 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8263727

New failing tests:
http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm

[EWS Watchlist]

Created attachment 343152 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Guillaume Emont]

(In reply to Build Bot from comment #16)
> Comment on attachment 343148 [details]
> Patch
> 
> Attachment 343148 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: https://webkit-queues.webkit.org/results/8263727
> 
> New failing tests:
> http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm

The modified code is guarded by ENABLE(BRANCH_COMPACTION), which is only true on ARM_THUMB2 and ARM64, so I think only the output from the ios and ios-sim EWS bots is relevant.

[WebKit Commit Bot]

Comment on attachment 343148 [details]
Patch

Clearing flags on attachment: 343148

Committed r233015: <https://trac.webkit.org/changeset/233015>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41293679>