Issue 580241 in chromium: Win32 Clang Can't Read 64-bit Integer From Read-Only Memory-Mapped File

[chro...@googlecode.com]

Status: Untriaged
Owner: ----
CC: tha...@chromium.org
Labels: Type-Bug Pri-2 Build-Toolchain OS-Windows

New issue 580241 by bcwh...@chromium.org: Win32 Clang Can't Read 64-bit
Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

Building 32-bit Chromium under windows with GYP_DEFINES=clang=1 and running
a particular test causes a Segmentation fault.

ninja -C out\Debug base_unittests
out\Debug\base_unittests --single-process-tests
--test-launcher-retry-limit=0
--gtest_filter=FilePersistentMemoryAllocatorTest.CreationTest

Reading the 64-bit value works just fine when the memory is a normal
allocation but for this particular test, the data is actually a read-only
memory-mapped file.

My own "printf" testing shows that the uint64_t "id" variable is aligned to
an 8-byte address and is valid. I can read the two halves as 32-bit values
but reading as 64-bit fails.

This test works fine when compiled using the Visual Studio compiler.

--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[chro...@googlecode.com]

Updates:
Cc: majne...@chromium.org

Comment #7 on issue 580241 by r...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

Clang's implementation of MSVC's volatile semantics is the problem here.

shared_meta() returns a 'volatile SharedMetadata*', and then the Id()
accessor accesses a uint64_t field inside it. We try to make that volatile
uint64_t access atomic, because we try to make all volatile accesses atomic
in MSVC mode. Unfortunately, 32-bit x86 doesn't have an atomic 64-bit load
instruction, so we do a locked cmpxchg8b to get the original value.
cmpxchg8b is a read-modify-write operation, and the file is mapped read
only, so that access faults, even though it would be a no-op.

So, why are you using volatile here in the first place? We can fix Clang to
do what MSVC does (emit two loads for a uint64_t), but it probably doesn't
do what you wanted.

---

Here's a minimal example to show the issue:

$ cat t.cpp
extern "C" __int64 load_vol(volatile __int64 *p) {
return *p;
}

$ clang -Xclang -fms-volatile -S t.cpp -O1 -o - -m32 | grep -v '^\s\.'
@feat.00 = 1
_load_vol: # @load_vol
# BB#0: # %entry
pushl %ebx
pushl %esi
movl 12(%esp), %esi
xorl %eax, %eax
xorl %edx, %edx
xorl %ebx, %ebx
xorl %ecx, %ecx
lock cmpxchg8b (%esi)
popl %esi
popl %ebx
retl

[chro...@googlecode.com]

Comment #8 on issue 580241 by bcwh...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

The data structure is marked "volatile" because it may be shared with other
processes through a shared memory segment. The derived class is getting
the memory segment from a read-only file but the base class can't change
it's implementation based on that. Plus, it's possible someday that a
MemoryMappedFile will support read/write between multiple processes.

[chro...@googlecode.com]

Comment #9 on issue 580241 by majne...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

Is it being modified while being shared?
If not, then the volatile seems unnecessary.
If it is, then the modification must be synchronized with another mechanism
because 'id' cannot be loaded atomically on 32-bit x86 without a cmpxchg8b.

[chro...@googlecode.com]

Comment #10 on issue 580241 by bcwh...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

Why attempt an atomic read for volatile? A backward-compatibility thing?
I'd have thought that would only be done if I had a volatile
std::atomic<...> variable and do a .load().

Maybe I can change the structure so that, rather than the entire thing
being volatile, just the fields that get changed are individually marked
volatile. Most of those fields, including the "id" are set during
construction and considered read-only after that.

But that said, performing a write-operation during a read seems a violation
of the expectations of the programmer. Could it also cause some extra
swapping? Presumably the cmpxchg will mark the memory page as dirty.

[chro...@googlecode.com]

Comment #11 on issue 580241 by majne...@chromium.org: Win32 Clang Can't

Read 64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

Microsoft's compiler, MSVC, provides this semantic by default when
targeting x86: https://msdn.microsoft.com/en-us/library/jj204392.aspx

They have their compiler give volatile operations acquire/release semantics.
Providing those semantics requires us to make operations atomic. The only
atomic operation on x86 wide enough for your type is a cmpxchg8b which
performs a RMW.

It turns out that they silently do not provide this behavior when faced
with a sufficiently large operation.

Clang will be changed to be compatible in this respect but it invites the
question "What was did the original code want when it asked for the access
to be volatile".

If the bytes might change, then a 64-bit volatile access is awkward because
we are forced to split the operation into two operations due to the x86
ISA. If the operation is split, it is possible to observe an inconsistent
state _unless_ the program synchronizes the access in some way. However,
synchronizing the access would seem to obviate the need for volatile in the
first place...

[chro...@googlecode.com]

Comment #12 on issue 580241 by bcwh...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

It started as a code-review suggestion because that data block is shared
(and writable) across processes. Only a few of the fields are written,
though, but it never occurred to me or the reviewers that it might be
detrimental to have something marked volatile when it wasn't strictly
necessary.

I'll change the code so that the structure isn't volatile but those members
that may be modified are so marked. They're already std::atomic.

[chro...@googlecode.com]

Comment #13 on issue 580241 by tha...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

Please use base/atomicops instead of STD::atomic. See std::atomic thread on
cxx chromium.org

[chro...@googlecode.com]

Comment #14 on issue 580241 by bcwh...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

That ship has long since sailed. It was originally subtle::atomic and I
was directed to be the lucky first user of std::atomic. Switching it was a
big effort and switching it back would be bigger still, largely due to the
lack of "unsigned" in base/atomicops.

[chro...@googlecode.com]

Comment #15 on issue 580241 by bcwh...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

Moving "volatile" markers...
https://codereview.chromium.org/1619983004/

[chro...@googlecode.com]

Comment #16 on issue 580241 by tha...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

Who recommended std::atomic? Can you tell them to not recommend it? Please
see the cxx@chromium thread. We don't want to use std::atomic atm.

(majnemer: Sounds like cl.exe doesn't have acquire/release semantics for
64-bit ints in 32-bit mode, right? So why must we?)

[chro...@googlecode.com]

Comment #17 on issue 580241 by bcwh...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

https://codereview.chromium.org/1410213004/
Comments #86 and #88 from JF and #89 from you.

[chro...@googlecode.com]

Comment #18 on issue 580241 by majne...@google.com: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

thakis: we do not need this behavior, I've committed to fixing this earlier.

[chro...@googlecode.com]

Comment #19 on issue 580241 by bcwh...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

> However, synchronizing the access would seem to obviate the need for
> volatile in the first place...

I had a recent discussion on this as well. See comments #86, #88, and
onwards on https://codereview.chromium.org/1410213004/ ...

But basically... (quoting JF)

C++11 atomics can also be volatile, which tells the compiler among other
things
"there can be external modification which you are not aware of" (it also
means
the compiler must respect ordering of volatiles between each other,
allowing you
to use them for signals and such). In the case of shared memory (in the same
process, or between different processes) it means the compiler can't do
usual
atomic optimization such as those in http://wg21.link/n4455. Without
volatile
the compiler can prove that a variable is use atomically only in certain
places,
and assume no other accesses occur as "as-if" the atomic uses. Realistically
compilers today don't do much of this, but they will in the future and the
bugs
it'll create will be wonderful to track down.

volatile doesn't provide no-tear guarantees, nor write ordering guarantees.
It
merely tells the compiler that it can't elide or duplicate accesses, and
can't
reorder volatile accesses w.r.t. each other (it can reorder non-volatile
w.r.t.
volatile). To get no-tear and happens-before you need atomic, so in the
case of
the ill-defined shared memory you need volatile+atomic.

Furthermore, for cross-process shared memory you need lock-free atomics! In
the
same process you're guaranteed address-free even when not lock-free, but
across
process you have no guarantee that non-lock-free operations work properly
(they
in fact do not on implementations I'm aware of). As an added
recommendation, I'd
have a check that the types you're using in shared memory have
CHECK(var.is_lock_free()). Practically speaking, this means you can't use
64-bit
values because they can tear on some ARM and MIPS CPUs.

[chro...@googlecode.com]

Comment #20 on issue 580241 by bcwh...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

> However, synchronizing the access would seem to obviate the need for
> volatile in the first place...

I had a recent discussion on this as well. See comments #86, #88, and
onwards on https://codereview.chromium.org/1410213004/ ...

But basically... (quoting JF)

C++11 atomics can also be volatile, which tells the compiler among other
things
"there can be external modification which you are not aware of" (it also
means
the compiler must respect ordering of volatiles between each other,
allowing you
to use them for signals and such). In the case of shared memory (in the same
process, or between different processes) it means the compiler can't do
usual
atomic optimization such as those in http://wg21.link/n4455. Without
volatile
the compiler can prove that a variable is use atomically only in certain
places,
and assume no other accesses occur as "as-if" the atomic uses. Realistically
compilers today don't do much of this, but they will in the future and the
bugs
it'll create will be wonderful to track down.

[chro...@googlecode.com]

Comment #21 on issue 580241 by majne...@chromium.org: Win32 Clang Can't

Read 64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

bcwhite: JF is giving you most of the story but not all of it. The
compiler cannot ensure that volatile loads will not be split, that is
impossible in the general case because doing so is predicated on the CPU
having a load wide enough. volatile *will* ensure that the load will not
be merged but I fail to see how that would be relevant in this situation.
As I said before, any sort of synchronization obviates the need for
volatile unless you are doing MMIO.

[chro...@googlecode.com]

Comment #22 on issue 580241 by bcwh...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

It's not relevant to _this_ situation (i.e. shearing of a 64-bit read) but
is relevant to atomics in general. If the atomic is accessible by
something outside the scope of the compiler then it must be
marked "volatile" so that the compiler doesn't make optimizations on the
assumption that it controls all.

[chro...@googlecode.com]

Comment #23 on issue 580241 by majne...@chromium.org: Win32 Clang Can't

Read 64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

The clang side of this has been fixed with r258506.

[chro...@googlecode.com]

Updates:
Status: Fixed

Comment #28 on issue 580241 by tha...@chromium.org: Win32 Clang Can't Read

64-bit Integer From Read-Only Memory-Mapped File
https://code.google.com/p/chromium/issues/detail?id=580241

(No comment was entered for this change.)

[chro...@googlecode.com]

Comment #29 on issue 580241 by bugd...@chromium.org: Win32 Clang Can't

Read 64-bit Integer From Read-Only Memory-Mapped File

https://code.google.com/p/chromium/issues/detail?id=580241#c29

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src.git/+/c03fc0ae7b182e1f51ba81f0a818087454937c51

commit c03fc0ae7b182e1f51ba81f0a818087454937c51
Author: bcwhite <bcw...@chromium.org>
Date: Fri Feb 05 01:18:03 2016

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

Review URL: https://codereview.chromium.org/1619983004

Cr-Commit-Position: refs/heads/master@{#373704}

[modify]
http://crrev.com/c03fc0ae7b182e1f51ba81f0a818087454937c51/base/metrics/persistent_memory_allocator.cc
[modify]
http://crrev.com/c03fc0ae7b182e1f51ba81f0a818087454937c51/base/metrics/persistent_memory_allocator.h