Summary: | [Armv7] Linkbuffer: executableOffsetFor() fails for location 2 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Guillaume Emont <guijemont> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bugs-noreply, calvaris, commit-queue, ews-bot+gtk-1, ews-bot+gtk-2, ews-watchlist, keith_miller, ltilve+ews, mark.lam, msaboff, rniwa, saam, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Other | ||||||||||||||
OS: | Linux | ||||||||||||||
Attachments: |
|
Description
Guillaume Emont
2018-06-18 07:39:36 PDT
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.
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. (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. (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. (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? (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. 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. 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. (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? (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. 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 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
Created attachment 343041 [details]
Patch
Updated patch for landing that adds the required comment and adds a static_cast to make clang happy.
(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. Created attachment 343148 [details]
Patch
Real patch for landing (added review)
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 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
(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. Comment on attachment 343148 [details] Patch Clearing flags on attachment: 343148 Committed r233015: <https://trac.webkit.org/changeset/233015> All reviewed patches have been landed. Closing bug. |