Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1619983004:
Restrict volatile section of block header. (Closed)
DescriptionRestrict volatile section of block header.
The clang compiler creates read-modify-write code for volatile uint64_t
on 32-bit windows which causes segv when inside a read-only memory-
mapped file. By reducing the "volatile" footprint, this should no
longer be a problem.
BUG=580241
TBR=asvitkine
Committed: https://crrev.com/c03fc0ae7b182e1f51ba81f0a818087454937c51
Cr-Commit-Position: refs/heads/master@{#373704}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 25 (10 generated)
bcwhite@chromium.org changed reviewers: + glider@chromium.org
Glider... Touching atomic stuff here by reducing what is volatile and how it is declared such. Anything I missed?
glider@chromium.org changed reviewers: + dvyukov@chromium.org
+dvyukov Calling for help, because I'm not really familiar with volatiles behavior on Windows. Also, I don't see any volatile uint64_t in the change.
On 2016/01/22 10:05:13, Alexander Potapenko wrote: > +dvyukov > > Calling for help, because I'm not really familiar with volatiles behavior on > Windows. > Also, I don't see any volatile uint64_t in the change. The uint64_t that is causing the problem doesn't need to be volatile. It's set during initial creation of the allocator and after that is read-only. It just got marked "volatile" because the entire structure was set so.
bcwhite@chromium.org changed reviewers: + mk@chromium.org
+mk who found the issue with clang
Ping?
LGTM I guess Is there a bug on clang? Why does't it use MMX/SSE on 386 for 64-bit loads?...
On 2016/02/02 16:05:36, Dmitry Vyukov wrote: > LGTM I guess > > Is there a bug on clang? Why does't it use MMX/SSE on 386 for 64-bit loads?... You can read the attached bug for details. Clang-team says they've fixed it.
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1619983004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1619983004/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Restrict volatile section of block header. The clang compiler creates read-modify-write code for volatile uint64_t on 32-bit windows which causes segv when inside a read-only memory- mapped file. By reducing the "volatile" footprint, this should no longer be a problem. BUG=580241 ========== to ========== Restrict volatile section of block header. The clang compiler creates read-modify-write code for volatile uint64_t on 32-bit windows which causes segv when inside a read-only memory- mapped file. By reducing the "volatile" footprint, this should no longer be a problem. BUG=580241 TBR=asvitkine ==========
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1619983004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1619983004/1
Message was sent while issue was closed.
Description was changed from ========== Restrict volatile section of block header. The clang compiler creates read-modify-write code for volatile uint64_t on 32-bit windows which causes segv when inside a read-only memory- mapped file. By reducing the "volatile" footprint, this should no longer be a problem. BUG=580241 TBR=asvitkine ========== to ========== Restrict volatile section of block header. The clang compiler creates read-modify-write code for volatile uint64_t on 32-bit windows which causes segv when inside a read-only memory- mapped file. By reducing the "volatile" footprint, this should no longer be a problem. BUG=580241 TBR=asvitkine ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Restrict volatile section of block header. The clang compiler creates read-modify-write code for volatile uint64_t on 32-bit windows which causes segv when inside a read-only memory- mapped file. By reducing the "volatile" footprint, this should no longer be a problem. BUG=580241 TBR=asvitkine ========== to ========== Restrict volatile section of block header. The clang compiler creates read-modify-write code for volatile uint64_t on 32-bit windows which causes segv when inside a read-only memory- mapped file. By reducing the "volatile" footprint, this should no longer be a problem. BUG=580241 TBR=asvitkine Committed: https://crrev.com/c03fc0ae7b182e1f51ba81f0a818087454937c51 Cr-Commit-Position: refs/heads/master@{#373704} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/c03fc0ae7b182e1f51ba81f0a818087454937c51 Cr-Commit-Position: refs/heads/master@{#373704}
Message was sent while issue was closed.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
Message was sent while issue was closed.
lgtm I guess Is the plan to revert this to the previous state after we have the new clang? (is that already in?) https://codereview.chromium.org/1619983004/diff/1/base/metrics/persistent_mem... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/1619983004/diff/1/base/metrics/persistent_mem... base/metrics/persistent_memory_allocator.h:254: const_cast<const char*>(mem_base_)); I don't understand why there's a const_cast here?
Message was sent while issue was closed.
> Is the plan to revert this to the previous state after we have the new clang? > (is that already in?) No need. This is better anyway as it allows the compiler to optimize access to those fields that are write-once (during construction) but still be safe for those that are updated in parallel. https://codereview.chromium.org/1619983004/diff/1/base/metrics/persistent_mem... > base/metrics/persistent_memory_allocator.h:254: const_cast<const > char*>(mem_base_)); > I don't understand why there's a const_cast here? I'll take another look. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
