Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2256113002:
Workaround for gcc array bound check issue (Closed)
|
Created:
4 years, 4 months ago by JaideepBajwa Modified:
4 years, 4 months ago CC:
v8-reviews_googlegroups.com, Hannes Payer (out of office), ulan, JoranSiu, john.yan, michael_dawson Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionWorkaround for gcc array bound check issue
V8 doesn't build on Ubuntu 16.04 (with GCC 5.3). Seems to be
a known regression on newer GCC version. It emits incorrect
"error: array subscript is above array bounds" message. Adding
explicit array bound check fixes the issue.
R=hablich@chromium.org
BUG=
Committed: https://crrev.com/757ea240f4208cf954d1e70cb1505b44fbb1f2d8
Cr-Commit-Position: refs/heads/master@{#38721}
Patch Set 1 #
Total comments: 2
Patch Set 2 : moved array bound check to MaskCell #Messages
Total messages: 15 (5 generated)
PTAL
Description was changed from ========== Workaround for gcc array bound check issue V8 doesn't build on Ubuntu 16.04 (with GCC 5.3). Seems to be a known regression on newer GCC version. It emits incorrect "error: array subscript is above array bounds" message. Adding explicit array bound check fixes the issue. R=hablich@chromium.org BUG= ========== to ========== Workaround for gcc array bound check issue V8 doesn't build on Ubuntu 16.04 (with GCC 5.3). Seems to be a known regression on newer GCC version. It emits incorrect "error: array subscript is above array bounds" message. Adding explicit array bound check fixes the issue. R=hablich@chromium.org BUG= ==========
hablich@chromium.org changed reviewers: + hpayer@chromium.org, mlippautz@chromium.org
On 2016/08/18 04:10:29, JaideepBajwa wrote: > PTAL added mlippautz and hpayer who are better qualified to review this CL.
Can you refer to a gcc bug? Just want to make sure that we don't miss something here. https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h File src/heap/slot-set.h (right): https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h#newcode77 src/heap/slot-set.h:77: MaskCell(start_bucket, start_cell, start_mask | end_mask); I would guess that the access in MaskCell is the problem, right? Can we move the condition in there? Maybe something like if (bucket < kBuckets) { // masking } else { // Explain the gcc bug. UNREACHABLE(); } https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h#newcode104 src/heap/slot-set.h:104: if (current_bucket == kBuckets || (current_bucket < kBuckets && If you do above, then just add a comment that refers to MaskCell.
On 2016/08/18 09:19:20, Michael Lippautz wrote: > Can you refer to a gcc bug? Just want to make sure that we don't miss something > here. > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h > File src/heap/slot-set.h (right): > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h#newcode77 > src/heap/slot-set.h:77: MaskCell(start_bucket, start_cell, start_mask | > end_mask); > I would guess that the access in MaskCell is the problem, right? > > Can we move the condition in there? Maybe something like > if (bucket < kBuckets) { > // masking > } else { > // Explain the gcc bug. > UNREACHABLE(); > } > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h#newcode104 > src/heap/slot-set.h:104: if (current_bucket == kBuckets || (current_bucket < > kBuckets && > If you do above, then just add a comment that refers to MaskCell. Thank you, I've updated as suggested. gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59124
On 2016/08/18 13:03:38, JaideepBajwa wrote: > On 2016/08/18 09:19:20, Michael Lippautz wrote: > > Can you refer to a gcc bug? Just want to make sure that we don't miss > something > > here. > > > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h > > File src/heap/slot-set.h (right): > > > > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h#newcode77 > > src/heap/slot-set.h:77: MaskCell(start_bucket, start_cell, start_mask | > > end_mask); > > I would guess that the access in MaskCell is the problem, right? > > > > Can we move the condition in there? Maybe something like > > if (bucket < kBuckets) { > > // masking > > } else { > > // Explain the gcc bug. > > UNREACHABLE(); > > } > > > > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h#newcode104 > > src/heap/slot-set.h:104: if (current_bucket == kBuckets || (current_bucket < > > kBuckets && > > If you do above, then just add a comment that refers to MaskCell. > > Thank you, I've updated as suggested. > gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59124 Did you check that it indeed compiles? In PS2 you also removed one workaround.
On 2016/08/18 13:38:06, Michael Lippautz wrote: > On 2016/08/18 13:03:38, JaideepBajwa wrote: > > On 2016/08/18 09:19:20, Michael Lippautz wrote: > > > Can you refer to a gcc bug? Just want to make sure that we don't miss > > something > > > here. > > > > > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h > > > File src/heap/slot-set.h (right): > > > > > > > > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h#newcode77 > > > src/heap/slot-set.h:77: MaskCell(start_bucket, start_cell, start_mask | > > > end_mask); > > > I would guess that the access in MaskCell is the problem, right? > > > > > > Can we move the condition in there? Maybe something like > > > if (bucket < kBuckets) { > > > // masking > > > } else { > > > // Explain the gcc bug. > > > UNREACHABLE(); > > > } > > > > > > > > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h#newcode104 > > > src/heap/slot-set.h:104: if (current_bucket == kBuckets || (current_bucket < > > > kBuckets && > > > If you do above, then just add a comment that refers to MaskCell. > > > > Thank you, I've updated as suggested. > > gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59124 > > Did you check that it indeed compiles? In PS2 you also removed one workaround. Yes it builds cleanly on ubuntu 16.04 with gcc5.3. After adding the bound check in MaskCell, gcc didn't complain for the array access on line 103.
On 2016/08/18 13:46:21, JaideepBajwa wrote: > On 2016/08/18 13:38:06, Michael Lippautz wrote: > > On 2016/08/18 13:03:38, JaideepBajwa wrote: > > > On 2016/08/18 09:19:20, Michael Lippautz wrote: > > > > Can you refer to a gcc bug? Just want to make sure that we don't miss > > > something > > > > here. > > > > > > > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h > > > > File src/heap/slot-set.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h#newcode77 > > > > src/heap/slot-set.h:77: MaskCell(start_bucket, start_cell, start_mask | > > > > end_mask); > > > > I would guess that the access in MaskCell is the problem, right? > > > > > > > > Can we move the condition in there? Maybe something like > > > > if (bucket < kBuckets) { > > > > // masking > > > > } else { > > > > // Explain the gcc bug. > > > > UNREACHABLE(); > > > > } > > > > > > > > > > > > > > https://codereview.chromium.org/2256113002/diff/1/src/heap/slot-set.h#newcode104 > > > > src/heap/slot-set.h:104: if (current_bucket == kBuckets || (current_bucket > < > > > > kBuckets && > > > > If you do above, then just add a comment that refers to MaskCell. > > > > > > Thank you, I've updated as suggested. > > > gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59124 > > > > Did you check that it indeed compiles? In PS2 you also removed one workaround. > > Yes it builds cleanly on ubuntu 16.04 with gcc5.3. > After adding the bound check in MaskCell, gcc didn't complain for the array > access on line 103. lgtm then. Thanks!
The CQ bit was checked by bjaideep@ca.ibm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Workaround for gcc array bound check issue V8 doesn't build on Ubuntu 16.04 (with GCC 5.3). Seems to be a known regression on newer GCC version. It emits incorrect "error: array subscript is above array bounds" message. Adding explicit array bound check fixes the issue. R=hablich@chromium.org BUG= ========== to ========== Workaround for gcc array bound check issue V8 doesn't build on Ubuntu 16.04 (with GCC 5.3). Seems to be a known regression on newer GCC version. It emits incorrect "error: array subscript is above array bounds" message. Adding explicit array bound check fixes the issue. R=hablich@chromium.org BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Workaround for gcc array bound check issue V8 doesn't build on Ubuntu 16.04 (with GCC 5.3). Seems to be a known regression on newer GCC version. It emits incorrect "error: array subscript is above array bounds" message. Adding explicit array bound check fixes the issue. R=hablich@chromium.org BUG= ========== to ========== Workaround for gcc array bound check issue V8 doesn't build on Ubuntu 16.04 (with GCC 5.3). Seems to be a known regression on newer GCC version. It emits incorrect "error: array subscript is above array bounds" message. Adding explicit array bound check fixes the issue. R=hablich@chromium.org BUG= Committed: https://crrev.com/757ea240f4208cf954d1e70cb1505b44fbb1f2d8 Cr-Commit-Position: refs/heads/master@{#38721} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/757ea240f4208cf954d1e70cb1505b44fbb1f2d8 Cr-Commit-Position: refs/heads/master@{#38721} |
