Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1899783002:
32-bit linux: Force 16-byte stack alignment. (Closed)
Description32-bit linux: Force 16-byte stack alignment.
clang assumes 16-byte stack alignment, but incoming stack alignment isn't
always guaranteed to be that way. It looks like v8 was lucky to not hit
this so far.
See https://crbug.com/418554 -- this makes v8's standalone config match
Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414
Maybe it's possible to change the caller of OnEntryHook() to guarantee
the right alignment, but matching Chromium's build flags here seems like
a good idea in general.
BUG=v8:4928
LOG=n
Committed: https://crrev.com/3afb3324941625559635380ef98a2ee73e370a0a
Cr-Commit-Position: refs/heads/master@{#35597}
Committed: https://crrev.com/5459468dd6c9501d95e4d32b51682d75c437883b
Cr-Commit-Position: refs/heads/master@{#35662}
Patch Set 1 #
Messages
Total messages: 21 (10 generated)
thakis@chromium.org changed reviewers: + machenbach@chromium.org
lgtm! thanks a lot for the fix!
Description was changed from ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=4928 LOG=n ========== to ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=v8:4928 LOG=n ==========
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1899783002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899783002/1
Message was sent while issue was closed.
Description was changed from ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=v8:4928 LOG=n ========== to ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=v8:4928 LOG=n ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=v8:4928 LOG=n ========== to ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=v8:4928 LOG=n Committed: https://crrev.com/3afb3324941625559635380ef98a2ee73e370a0a Cr-Commit-Position: refs/heads/master@{#35597} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/3afb3324941625559635380ef98a2ee73e370a0a Cr-Commit-Position: refs/heads/master@{#35597}
Message was sent while issue was closed.
rnk@chromium.org changed reviewers: + rnk@chromium.org
Message was sent while issue was closed.
I'm not sure you really want these flags on by default in V8, they hurt performance and tend to trip over compiler bugs (https://llvm.org/bugs/show_bug.cgi?id=16830). Maybe it's more important for V8's upstream gyp build to be in sync with Chrome's, though. The only time you should see a misaligned stack is on old Linux distros or when V8's own code isn't ABI-compliant.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1895783004/ by adamk@chromium.org. The reason for reverting is: Broke InterpreterCreateArguments test on Linux nosnap debug: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap%20-%2....
Message was sent while issue was closed.
Description was changed from ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=v8:4928 LOG=n Committed: https://crrev.com/3afb3324941625559635380ef98a2ee73e370a0a Cr-Commit-Position: refs/heads/master@{#35597} ========== to ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=v8:4928 LOG=n Committed: https://crrev.com/3afb3324941625559635380ef98a2ee73e370a0a Cr-Commit-Position: refs/heads/master@{#35597} ==========
Trying to reland this. This time we'll disable the tests if they fail...
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1899783002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899783002/1
Message was sent while issue was closed.
Description was changed from ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=v8:4928 LOG=n Committed: https://crrev.com/3afb3324941625559635380ef98a2ee73e370a0a Cr-Commit-Position: refs/heads/master@{#35597} ========== to ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=v8:4928 LOG=n Committed: https://crrev.com/3afb3324941625559635380ef98a2ee73e370a0a Cr-Commit-Position: refs/heads/master@{#35597} ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=v8:4928 LOG=n Committed: https://crrev.com/3afb3324941625559635380ef98a2ee73e370a0a Cr-Commit-Position: refs/heads/master@{#35597} ========== to ========== 32-bit linux: Force 16-byte stack alignment. clang assumes 16-byte stack alignment, but incoming stack alignment isn't always guaranteed to be that way. It looks like v8 was lucky to not hit this so far. See https://crbug.com/418554 -- this makes v8's standalone config match Chromium. See also https://llvm.org/bugs/show_bug.cgi?id=21414 Maybe it's possible to change the caller of OnEntryHook() to guarantee the right alignment, but matching Chromium's build flags here seems like a good idea in general. BUG=v8:4928 LOG=n Committed: https://crrev.com/3afb3324941625559635380ef98a2ee73e370a0a Cr-Commit-Position: refs/heads/master@{#35597} Committed: https://crrev.com/5459468dd6c9501d95e4d32b51682d75c437883b Cr-Commit-Position: refs/heads/master@{#35662} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/5459468dd6c9501d95e4d32b51682d75c437883b Cr-Commit-Position: refs/heads/master@{#35662} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
