[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[christian_hoff at gmx dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

--- Comment #13 from Christian Hoff  ---
Hello Jonathan,

that is great news! Thank you very much for fixing this issue, I really
appreciate this.


Best regards,

   Christian

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

Jonathan Wakely  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #12 from Jonathan Wakely  ---
Should be fixed on trunk now.

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

--- Comment #11 from Jonathan Wakely  ---
Author: redi
Date: Tue Nov 27 23:25:56 2018
New Revision: 266533

URL: https://gcc.gnu.org/viewcvs?rev=266533=gcc=rev
Log:
PR libstdc++/67843 set shared_ptr lock policy at build-time

This resolves a longstanding issue where the lock policy for shared_ptr
reference counting depends on compilation options when the header is
included, so that different -march options can cause ABI changes. For
example, objects compiled with -march=armv7 will use atomics to
synchronize reference counts, and objects compiled with -march=armv5t
will use a mutex. That means the shared_ptr control block will have a
different layout in different objects, causing ODR violations and
undefined behaviour. This was the root cause of PR libstdc++/42734 as
well as PR libstdc++/67843.

The solution is to decide on the lock policy at build time, when
libstdc++ is configured. The configure script checks for the
availability of the necessary atomic built-ins for the target and fixes
that choice permanently. Different -march flags used to compile user
code will not cause changes to the lock policy. This results in an ABI
change for certain compilations, but only where there was already an ABI
incompatibility between the libstdc++.so library and objects built with
an incompatible -march option. In general, this means a more stable ABI
that isn't silently altered when -march flags make addition atomic ops
available.

To force a target to use "atomic" or "mutex" the new configure option
--with-libstdcxx-lock-policy can be used.

In order to turn ODR violations into linker errors, the uses of
shared_ptr in filesystem directory iterators have been replaced
with __shared_ptr, and explicit instantiations are declared. This
ensures that object files using those types cannot link to libstdc++
libs unless they use the same lock policy.

PR libstdc++/67843
* acinclude.m4 (GLIBCXX_ENABLE_LOCK_POLICY): Add new macro
that defines _GLIBCXX_HAVE_ATOMIC_LOCK_POLICY.
* config.h.in: Regenerate.
* configure: Regenerate.
* configure.ac: Use GLIBCXX_ENABLE_LOCK_POLICY.
* doc/xml/manual/configure.xml: Document new configure option.
* include/bits/fs_dir.h (directory_iterator): Use __shared_ptr
instead of shared_ptr.
(recursive_directory_iterator): Likewise.
(__shared_ptr<_Dir>): Add explicit instantiation declaration.
(__shared_ptr): Likewise.
* include/bits/shared_ptr_base.h (__allocate_shared, __make_shared):
Add default template argument for _Lock_policy template parameter.
* include/ext/concurrence.h (__default_lock_policy): Check macro
_GLIBCXX_HAVE_ATOMIC_LOCK_POLICY instead of checking if the current
target supports the builtins for compare-and-swap.
* src/filesystem/std-dir.cc (__shared_ptr<_Dir>): Add explicit
instantiation definition.
(__shared_ptr): Likewise.
(directory_iterator, recursive_directory_iterator): Use __make_shared
instead of make_shared.

Modified:
trunk/libstdc++-v3/ChangeLog
trunk/libstdc++-v3/acinclude.m4
trunk/libstdc++-v3/config.h.in
trunk/libstdc++-v3/configure
trunk/libstdc++-v3/configure.ac
trunk/libstdc++-v3/doc/xml/manual/configure.xml
trunk/libstdc++-v3/include/bits/fs_dir.h
trunk/libstdc++-v3/include/bits/shared_ptr_base.h
trunk/libstdc++-v3/include/ext/concurrence.h
trunk/libstdc++-v3/src/filesystem/std-dir.cc

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

Jonathan Wakely  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
   Target Milestone|--- |9.0

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

Jonathan Wakely  changed:

   What|Removed |Added

 CC||christian_hoff at gmx dot net

--- Comment #10 from Jonathan Wakely  ---
*** Bug 87666 has been marked as a duplicate of this bug. ***

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

Hans-Peter Nilsson  changed:

   What|Removed |Added

 CC||hp at gcc dot gnu.org

--- Comment #9 from Hans-Peter Nilsson  ---
(I finally found it - I must have written down the wrong number at the BoF!)

A sidenote: this is the "main" PR behind the ARM-libcstdc++-atomics BoF at the
recent GNU Tools Cauldron in Manchester:
,
where IIRC there was consensus.  I don't see it communicated neither on the
lists nor here, so I'll give it a try:

As I recall it, the level of atomics support affecting the ABI is to be fixed
to the lowest denominator reached with the default configuration, and gcc will
be changed to emit the builtin preprocessor atomicity macros to match.  This
would be the new default, overridable by configure-options to the effect of the
current behavior (and maybe compile-time gcc-options as well).  A broken
combination will yield (link-time) warnings. 

This requires gcc as well as top-level configure changes.

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

Jonathan Wakely  changed:

   What|Removed |Added

 Status|UNCONFIRMED |NEW
   Last reconfirmed||2017-06-06
 Ever confirmed|0   |1

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[clyon at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

--- Comment #8 from Christophe Lyon  ---
I've tested the patch you attached.

It needs a slight modification because it does not apply to current trunk:
_GLIBCXX_READ_MEM_BARRIER;
_GLIBCXX_WRITE_MEM_BARRIER;
have been replaced by:
__atomic_thread_fence (__ATOMIC_ACQ_REL);



I then ran the tests as I usually do (build a armv7-a toolchain, force
-march=armv5t when running make check), and the test FAILed:

/tmp/ccyPksY2.o: In function
`std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)1>::_M_release()':^M
/home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-linux-gnueabi/gcc3/arm-none-linux-gnueabi/libstdc++-v3/include/bits/shared_ptr_base.h:160:
undefined reference to `void
std::__check_lock_policy<(__gnu_cxx::_Lock_policy)1>()'^M
[]


If I don't force -march=armv5t (and use the default armv7-a), the test PASSes.

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[clyon at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

--- Comment #7 from Christophe Lyon  ---
You are probably correct: I recompiled my cross-toolchain to default to armv5t
and the test now passes.

(as opposed to armv7-a toolchain + -march=armv5t to compile the testcase).

I agree that a linker error would be good, though I don't fully understand your
patch :-)

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

--- Comment #6 from Jonathan Wakely  ---
Created attachment 36458
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36458=edit
Require consistent shared_ptr lock policy

Maybe we should solve this by forcing a linker error when an object doesn't
have the same lock policy as the library.

This patch causes the shared_ptr tests to FAIL when run with -march=i386, with
this error:

include/bits/shared_ptr_base.h:160: undefined reference to `void
std::__check_lock_policy<(__gnu_cxx::_Lock_policy)1>()

Instead of corrupting the heap at run-time the tests fail to link because the
library only has a definition for 

std::__check_lock_policy<(__gnu_cxx::_Lock_policy)2>()

The same thing should work for arm and other targets where the availability of
atomics depends on -march.

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

--- Comment #5 from Jonathan Wakely  ---
I've just realised this is probably the same issue as PR42734

If you compile for armv5 then shared_ptr uses a mutex internally, because armv5
doesn't support the necessary atomics. The library is compiled for armv7, so
its shared_ptr uses atomics.  When you pass a shared_ptr from an object
compiled with armv5 to a library compiled as armv7 they disagree on the size
and layout of the object, and all hell breaks loose.

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

--- Comment #4 from Jonathan Wakely  ---
Running this under valgrind I see 900+ errors, the first 100 seem to be from
the global strings in the libstdc++-v3/testsuite/util/testsuite_fs.h file (I
think).

All the errors are basically nonsense, reporting errors in getpid() and
mkstemp() and things that are very unlikely to be broken, so either that
executable is horribly miscompiled or valgrind is useless with it.

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[clyon at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

--- Comment #2 from Christophe Lyon  ---
I am not sure what kind of helpful information I can provide.

Can you build a cross-compiler (x86 -> armv7-a)?

This is what I do and I haven't seen the ICE you mention (I was using r228286).

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[clyon at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

--- Comment #3 from Christophe Lyon  ---
I can attach a statically linked binary, if that helps.

[Bug libstdc++/67843] experimental/filesystem/iterators/directory_iterator.cc fails on armv5t

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67843

--- Comment #1 from Jonathan Wakely  ---
I'm unable to build an armv7 toolchain, it keeps failing with an ICE, so I'll
need more information to do anything about this.