Re: Problem building libstdc++ for the avr target

[Jonathan Wakely <jwakely@redhat.com>]

On 07/02/21 14:55 +0100, Vladimir V wrote:
>Hello,
>
>I finally managed to test building for AVR with Keith's patch and
>unistd.h removed (to enforce usage of stabs in filesystem).
>
>I identified a couple of minor build issues that are obvious from the
>attached patch, Could you please have a look? They work with AVR target
>and doesn't seem to cause regressions for x64 build.
>
>Thank you.
>
>пт, 22 янв. 2021 г. в 15:46, Vladimir V <vv.os.swe@gmail.com>:
>
>> Thank you for the detailed clarification.
>>
>> My primary idea was to reduce the build dependencies between hosted
>> libstdc++
>> and target libc if some components can not be supported or never will be
>> used.
>> Also my general vision is that if something doesn't work it shouldn't be
>> available
>> in the environment. But I perfectly understand the rationale you explained
>> and requirements applied by standard.
>>
>> So I think not much can be done here at this point and further work should
>> be on the avr-libc side.
>>
>> Thank you.
>>
>> пт, 8 янв. 2021 г. в 19:21, Jonathan Wakely <jwakely@redhat.com>:
>>
>>> On 04/01/21 12:28 +0100, Vladimir V wrote:
>>> >Hello and Happy New Year!
>>> >
>>> >Getting back to the discussion we had about scalable libstdc++.
>>> >For sure it will be very helpful for embedded targets. There are huge
>>> >components with
>>> >well defined boundaries that are unlikely to be often used in minimalist
>>> >settings but require missing platform support.
>>> >One particular example I'm looking at right now is 'filesystem'. Although
>>> >it resorts to some
>>> >default stubs if corresponding API is not present, I think it would be
>>> >correct to remove it from the library
>>> >completely if it can not/should not be used on the target.
>>> >In case of avr-libc it is worse as the unistd is partially present but
>>> not
>>> >sufficient to satisfy all filesystem
>>> >dependencies.
>>> >
>>> >It looks like that after the filesystem support was implemented for c++17
>>> >standard there is no option to remove
>>> >it from the build (unlike for the filesystem-ts). Please correct me if I
>>> am
>>> >wrong but If it is like that I would like to make this configurable.
>>>
>>> You're right.
>>>
>>> My thinking was that the C++ standard requires the <filesystem> header
>>> to be present for a hosted implementation. It doesn't necessarily
>>> require the contents to *do* much, so it's conforming for them to
>>> exist but always report errors.
>>>
>>> >Would it be acceptable?
>>>
>>> I'm unsure.
>>>
>>> Currently we support "hosted" and "freestanding", but not really
>>> anything more fine-grained. The point of the configure switch
>>> --disable-libstdcxx-filesystem-ts was not really to allow people to
>>> build without it, but more to enable it for targets where it wasn't on
>>> by default because it hadn't been tested yet.
>>>
>>> I've recently been trying to move away from having missing features,
>>> and make things available unconditionally, but to use useless stubs or
>>> return errors for targets that can't support it. For example, GCC 11
>>> always defines the std::thread type, even if the target can't actually
>>> use it to create new threads. The idea is to always provide a complete
>>> implementation, not have the feature set vary by target (I see a lot
>>> of confusion on places like StackOverflow, where the answer is "yeah
>>> sorry, that header is empty on your OS").
>>>
>>> What you're suggesting would lead to a more modular libstdc++ where
>>> you can enable/disable components, similar to what newlib does. If we
>>> did that for more than just the std::filesystem library things would
>>> get complicated (it increases the maintenance and testing burden, and
>>> we need to be careful not to create dependencies on anything from a
>>> conditionally-present feature).
>>>
>>> Would your suggestion actually result in smaller/faster binaries for
>>> AVR? Or just remove features that aren't fully functional if you
>>> attempt to use them? If having those features in the library doesn't
>>> actually cause bigger/slower binaries, then I don't really see a
>>> problem with them being present.
>>>
>>> Tangentially, the direction the C++ standard seems to be going is to
>>> grow the feature set of freestanding. That would allow more things to
>>> be used in freestanding environments, rather than the very limited set
>>> of features currently guaranteed to be available. That doesn't really
>>> help you though, as you want a hosted env with things like basic I/O,
>>> but not the full set of features usually present for hosted.
>>>
>>>
>>>
>>>

From ac970e6a376def43c510f9091d342a014f23fe8f Mon Sep 17 00:00:00 2001
>From: Vladimir Vishnevsky <vv.os.swe@gmail.com>
>Date: Sat, 6 Feb 2021 12:25:24 +0100
>Subject: [PATCH] libstdc++: Fix build failure for targets without unistd.h
>
>The patch fixes build issues occuring if build parameter
>"--enable-cstdio=stdio_pure" is specified and no unistd.h is
>present in the environment.
>
>2021-02-07 Vladimir Vishnevsky <vv.os.swe@gmail.com>
>
>libstdc++-v3/ChangeLog:
>        Fix build failure for targets without unistd.h.
>        * include/ext/stdio_sync_filebuf.h: Unused <unistd.h> include removed.
>        * src/c++17/fs_ops.cc: Proper namespace specified for locally defined
>        mode_t.
>---
> libstdc++-v3/include/ext/stdio_sync_filebuf.h | 1 -
> libstdc++-v3/src/c++17/fs_ops.cc              | 2 +-
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
>diff --git a/libstdc++-v3/include/ext/stdio_sync_filebuf.h b/libstdc++-v3/include/ext/stdio_sync_filebuf.h
>index 178b471957a..90765e55831 100644
>--- a/libstdc++-v3/include/ext/stdio_sync_filebuf.h
>+++ b/libstdc++-v3/include/ext/stdio_sync_filebuf.h
>@@ -32,7 +32,6 @@
> #pragma GCC system_header
>
> #include <streambuf>
>-#include <unistd.h>

I don't see why this file includes <unistd.h>, it doesn't *seem* to
need it. Even if it's needed, the correct way to include it is:

#ifdef _GLIBCXX_HAVE_UNISTD_H
# include <unistd.h>
#endif

So we should do that at least, even if we don't remove it.


> #include <cstdio>
> #include <bits/c++io.h>  // For __c_file
> #include <bits/move.h>   // For __exchange
>diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc
>index 72755c98a5a..04a559ab1d6 100644
>--- a/libstdc++-v3/src/c++17/fs_ops.cc
>+++ b/libstdc++-v3/src/c++17/fs_ops.cc
>@@ -1130,7 +1130,7 @@ fs::permissions(const path& p, perms prms, perm_options opts,
> #else
>   if (nofollow && is_symlink(st))
>     ec = std::make_error_code(std::errc::not_supported);
>-  else if (posix::chmod(p.c_str(), static_cast<mode_t>(prms)))
>+  else if (posix::chmod(p.c_str(), static_cast<posix::mode_t>(prms)))
>     err = errno;
> #endif

Yes, this is correct, thanks.