This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/2] PR libstdc++/86756 Move rest of std::filesystem to libstdc++.so


On 09/01/19 13:53 +0100, Christophe Lyon wrote:
On Wed, 9 Jan 2019 at 11:11, Jonathan Wakely <jwakely@redhat.com> wrote:

On 09/01/19 10:09 +0000, Jonathan Wakely wrote:
>On 08/01/19 11:13 +0100, Christophe Lyon wrote:
>>On Mon, 7 Jan 2019 at 15:14, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>>>
>>>On Mon, 7 Jan 2019 at 13:39, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>
>>>> On 07/01/19 09:48 +0000, Jonathan Wakely wrote:
>>>> >On 07/01/19 10:24 +0100, Christophe Lyon wrote:
>>>> >>Hi Jonathan
>>>> >>
>>>> >>On Sun, 6 Jan 2019 at 23:37, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>> >>>
>>>> >>>Move std::filesystem directory iterators and operations from
>>>> >>>libstdc++fs.a to main libstdc++ library. These components have many
>>>> >>>dependencies on OS support, which is not available on all targets. Some
>>>> >>>additional autoconf checks and conditional compilation is needed to
>>>> >>>ensure the files will build for all targets. Previously this code was
>>>> >>>not compiled without --enable-libstdcxx-filesystem-ts but the C++17
>>>> >>>components should be available for all hosted builds.
>>>> >>>
>>>> >>>The tests for these components no longer need to link to libstdc++fs.a,
>>>> >>>but are not expected to pass on all targets. To avoid numerous failures
>>>> >>>on targets which are not expected to pass the tests (due to missing OS
>>>> >>>functionality) leave the dg-require-filesystem-ts directives in place
>>>> >>>for now. This will ensure the tests only run for builds where the
>>>> >>>filesystem-ts library is built, which presumably means some level of OS
>>>> >>>support is present.
>>>> >>>
>>>> >>>
>>>> >>>Tested x86_64-linux (old/new string ABIs, 32/64 bit), x86_64-w64-mingw32.
>>>> >>>
>>>> >>>Committed to trunk.
>>>> >>>
>>>> >>
>>>> >>After this commit (r267616), I've noticed build failures for my
>>>> >>newlib-based toolchains:
>>>> >>aarch64-elf, arm-eabi:
>>>> >>
>>>> >>In file included from
>>>> >>/tmp/5241593_7.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/src/c++17/fs_ops.cc:57:
>>>> >>/tmp/5241593_7.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/src/c++17/../filesystem/ops-common.h:142:11:
>>>> >>error: '::truncate' has not been declared
>>>> >> 142 |   using ::truncate;
>>>> >>     |           ^~~~~~~~
>>>> >>/tmp/5241593_7.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/src/c++17/fs_ops.cc:
>>>> >>In function 'void std::filesystem::resize_file(const
>>>> >>std::filesystem::__cxx11::path&, uintmax_t, std::error_code&)':
>>>> >>/tmp/5241593_7.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/src/c++17/fs_ops.cc:1274:19:
>>>> >>error: 'truncate' is not a member of 'posix'
>>>> >>1274 |   else if (posix::truncate(p.c_str(), size))
>>>> >>     |                   ^~~~~~~~
>>>> >>make[5]: *** [fs_ops.lo] Error 1
>>>> >>
>>>> >>I'm not sure if there's an obvious fix? Note that I'm using a rather
>>>> >>old newlib version, if that matters.
>>>> >
>>>> >That's probably the reason, as I didn't see this in my tests with
>>>> >newlib builds.
>>>> >
>>>> >The fix is to add yet another autoconf check and guard the uses of
>>>> >truncate with a _GLIBCXX_USE_TRUNCATE macro. I'll do that now ...
>>>>
>>>>
>>>> Should be fixed with this patch, committed to trunk as r267647.
>>>>
>>>
>>>Yes, it works. Thanks!
>>>
>>
>>Hi Jonathan,
>>
>>So... this was a confirmation that the GCC build succeeded, not that
>>the tests pass :)
>>
>>And there are actually a couple new errors with my newlib-based toolchains:
>>FAIL: 27_io/filesystem/operations/all.cc (test for excess errors)
>>FAIL: 27_io/filesystem/operations/resize_file.cc (test for excess errors)
>>FAIL: 27_io/filesystem/path/generation/normal2.cc (test for excess errors)
>>which are also UNRESOLVED, because of link-time undefined reference to `chdir',
>>chmod, mkdir, pathconf and getcwd.
>
>Ah, I was assuming if <unistd.h> is present, then those basic
>functions will be present. More autoconf checks needed, I guess.
>That isn't hard to do, just tedious.
>
>>On aarch64, I'm seeing an addtional:
>>FAIL: 27_io/filesystem/path/compare/strings.cc execution test
>>because:
>>/libstdc++-v3/testsuite/27_io/filesystem/path/compare/strings.cc:39:
>>void test01(): Assertion 'p.compare(p0) == p.compare(s0)' failed.
>
>Odd, I don't know why that would be target-specific. It's probably
>just latent on other targets. I'll try to reproduce it on my aarch64
>system, but it will take a while to build current trunk.
>
>If you have time, could you please apply this patch, re-run that test

*This* patch:

--- a/libstdc++-v3/testsuite/27_io/filesystem/path/compare/strings.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/path/compare/strings.cc
@@ -36,6 +36,7 @@ test01()
     path p(s);
     VERIFY( p.compare(s) == 0 );
     VERIFY( p.compare(s.c_str()) == 0 );
+    __builtin_printf("Comparing %s as path:%d as string:%d\n", s.c_str(), p.compare(p0), p.compare(s0));
     VERIFY( p.compare(p0) == p.compare(s0) );
     VERIFY( p.compare(p0) == p.compare(s0.c_str()) );
   }




>(cd $target/libstdc++-v3 && make check
>RUNTESTFLAGS=conformance.exp=*/path/compare/strings/cc) and send me
>the output from the $target/libstdc++-v3/testsuite/libstdc++.log file?
>
>On x86_64 I get:
>
>Comparing  as path:-1 as string:-1
>Comparing / as path:-1 as string:-1
>Comparing // as path:-1 as string:-1
>Comparing /. as path:-51 as string:-51
>Comparing /./ as path:-51 as string:-51
>Comparing /a as path:-2 as string:-2
>Comparing /a/ as path:-1 as string:-1
>Comparing /a// as path:-1 as string:-1
>Comparing /a/b/c/d as path:1 as string:1
>Comparing /a//b as path:1 as string:1
>Comparing a as path:-1 as string:-1
>Comparing a/b as path:-1 as string:-1
>Comparing a/b/ as path:-1 as string:-1
>Comparing a/b/c as path:-1 as string:-1
>Comparing a/b/c.d as path:-1 as string:-1
>Comparing a/b/.. as path:-1 as string:-1
>Comparing a/b/c. as path:-1 as string:-1
>Comparing a/b/.c as path:-1 as string:-1
>PASS: 27_io/filesystem/path/compare/strings.cc execution test
>

Here is what I have on aarch64-none-elf:
Comparing  as path:-1 as string:-1^M
Comparing / as path:-1 as string:-1^M
Comparing // as path:-1 as string:-1^M
Comparing /. as path:-102 as string:-51^M
/libstdc++-v3/testsuite/27_io/filesystem/path/compare/strings.cc:40:
void test01(): Assertion 'p.compare(p0) == p.compare(s0)' failed.^M
^M
*** EXIT code 4242^M
emu: host signal 6^M
FAIL: 27_io/filesystem/path/compare/strings.cc execution test

This is a strange one. With my aarch64-unknown-linux-gnu trunk build I get:

Comparing /. as path:-51 as string:-51

Which suggests it's a newlib vs glibc difference, but this target uses
glibc (right?) and has the same FAIL:
https://gcc.gnu.org/ml/gcc-testresults/2019-01/msg02276.html

The test should be reducable to simply:

// { dg-options "-std=gnu++17" }
#include <string>
#include <string_view>

int main()
{
 std::string_view s0 = "a";
 std::string p0(s0);
 std::string p(".");
 __builtin_printf("as path:%d as string:%d\n", p.compare(p0), p.compare(s0));
}

Again, I get -51 and -51 for this. Could you test it on
aarch64-none-elf?

In terms of what the standard requires, this comparison is based on
strcmp, i.e. it only specifies a result less than zero, equal to zero,
or greater than zero. But I'd like to know why the comparisons aren't
returning the same consistent value.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]