[Webkit-unassigned] [Bug 186765] [Armv7] Linkbuffer: executableOffsetFor() fails for location 2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=186765

--- Comment #5 from Guillaume Emont <guijemont at igalia.com> ---
(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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180618/baccd580/attachment.html>