Re: [PATCH] libstdc++: ifdef rtti specific function __throw_ios_failure() by __cpp_rtti

Re: [PATCH] libstdc++: ifdef rtti specific function __throw_ios_failure() by __cpp_rtti

[Jonathan Wakely]

>Hello,
>
>ran into the following when building libstdc++ without rtti support:
>
>libstdc++-v3/src/c++11/cxx11-ios_failure.cc:174:54: error: no matching 
>function for call to 'std::ios_base::failure::failure(const char*&, int&)'
>
>Attached patch does as follows:

Libstdc++ patches need to be CC'd to the libstdc++@ list as well.

>ifdef rtti specific function __throw_ios_failure() by __cpp_rtti
>
>Overloaded __throw_ios_failure(const char*, int) got introduced in
>484e936e88e5, however __ios_failure() with respective signature is only
>defined if __cpp_rtti is defined, hence should only be used within
>contexts also guarded by __cpp_rtti.

This patch is wrong. If you simply disable that function definition
for !__cpp_rtti then you'll get linker errors from fstream.tcc when
that function is called.

/usr/bin/ld: /home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so: undefined reference to `std::__throw_ios_failure(char const*, int)'

>This was done correctly for the c++98 part and probably just forgotten
>for c++11.

This has nothing to do with C++98, it's relted to the gcc4-compatible
ABI versus the cxx11 ABI.

I added a better patch to
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99077

>Thanks
>
>   mirko
>
>PS: Shouldn't this have been covered by any tests?

Nobody tests building libstdc++ with -fno-rtti because almost nobody
does that.

But we have plenty of tests, and hundreds of them fail with your patch
:-)

Re: [PATCH] libstdc++: ifdef rtti specific function __throw_ios_failure() by __cpp_rtti

[Mirko Vogt]

On 2/12/21 11:30 AM, Jonathan Wakely wrote:
> This patch is wrong.

Indeed, thanks for checking.

> If you simply disable that function definition
> for !__cpp_rtti then you'll get linker errors from fstream.tcc when
> that function is called.
> 
> /usr/bin/ld: /home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so: undefined reference to `std::__throw_ios_failure(char const*, int)'

Funny enough that doesn't occur for my use case - builds fine. However 
building with a toolchain for bare metal, potentially resulting in 
disabling e.g. fstream.

> 
>> This was done correctly for the c++98 part and probably just forgotten
>> for c++11.
> 
> This has nothing to do with C++98, it's relted to the gcc4-compatible
> ABI versus the cxx11 ABI.

Urgs, yeah, that last chunk for cxx98 expands a different macro to 
include that definition - not the rtti ifdef. Misread and wrongly took over.

> 
> I added a better patch to
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99077

Thanks!

>> PS: Shouldn't this have been covered by any tests?
> 
> Nobody tests building libstdc++ with -fno-rtti because almost nobody
> does that.

Just as info: Espressif's crosstool-ng fork (not sure about vanilla) is 
building with no-rtti for targeting their Xtensa based ESP32 SoCs. 
That's how I stumbled over this.

> 
> But we have plenty of tests, and hundreds of them fail with your patch
> :-)

Yeah, glass houses and stones.. sorry for that :)

   mirko

Re: [PATCH] libstdc++: ifdef rtti specific function __throw_ios_failure() by __cpp_rtti

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

On 12/02/21 12:31 +0100, Mirko Vogt wrote:
>On 2/12/21 11:30 AM, Jonathan Wakely wrote:
>>This patch is wrong.
>
>Indeed, thanks for checking.
>
>>If you simply disable that function definition
>>for !__cpp_rtti then you'll get linker errors from fstream.tcc when
>>that function is called.
>>
>>/usr/bin/ld: /home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so: undefined reference to `std::__throw_ios_failure(char const*, int)'
>
>Funny enough that doesn't occur for my use case - builds fine. However 
>building with a toolchain for bare metal, potentially resulting in 
>disabling e.g. fstream.
>
>>
>>>This was done correctly for the c++98 part and probably just forgotten
>>>for c++11.
>>
>>This has nothing to do with C++98, it's relted to the gcc4-compatible
>>ABI versus the cxx11 ABI.
>
>Urgs, yeah, that last chunk for cxx98 expands a different macro to 
>include that definition - not the rtti ifdef. Misread and wrongly took 
>over.
>
>>
>>I added a better patch to
>>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99077

I've committed that patch to master now (and will backport it to
gcc-10 and gcc-9 soon). Thanks for finding the bug.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 2652 bytes --]

commit 4591f7e5329dcc6ee9af2f314a050936d470ab5b
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Feb 12 10:37:56 2021

    libstdc++: Fix bootstrap with -fno-rtti [PR 99077]
    
    When libstdc++ is built without RTTI the __ios_failure type is just an
    alias for std::ios_failure, so trying to construct it from an int won't
    compile. This changes the RTTI-enabled __ios_failure type to have the
    same constructor parameters as std::ios_failure, so that the constructor
    takes the same arguments whether RTTI is enabled or not.
    
    The __throw_ios_failure function now constructs the error_code, instead
    of the __ios_failure constructor. As a drive-by fix that error_code is
    constructed with std::generic_category() not std::system_category(),
    because the int comes from errno which corresponds to the generic
    category.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/99077
            * src/c++11/cxx11-ios_failure.cc (__ios_failure(const char*, int)):
            Change int parameter to error_code, to match std::ios_failure.
            (__throw_ios_failure(const char*, int)): Construct error_code
            from int parameter.

diff --git a/libstdc++-v3/src/c++11/cxx11-ios_failure.cc b/libstdc++-v3/src/c++11/cxx11-ios_failure.cc
index e82c1aaf63b..a918ab21015 100644
--- a/libstdc++-v3/src/c++11/cxx11-ios_failure.cc
+++ b/libstdc++-v3/src/c++11/cxx11-ios_failure.cc
@@ -114,7 +114,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __ios_failure(const char* s) : failure(s)
     { __construct_ios_failure(buf, runtime_error::what()); }
 
-    __ios_failure(const char* s, int e) : failure(s, to_error_code(e))
+    __ios_failure(const char* s, const error_code& e) : failure(s, e)
     { __construct_ios_failure(buf, runtime_error::what()); }
 
     ~__ios_failure()
@@ -125,10 +125,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     // There are assertions in src/c++98/ios_failure.cc to ensure the size
     // and alignment assumptions are valid.
     alignas(runtime_error) unsigned char buf[sizeof(runtime_error)];
-
-    static error_code
-    to_error_code(int e)
-    { return e ? error_code(e, system_category()) : io_errc::stream; }
   };
 
   // Custom type info for __ios_failure.
@@ -171,7 +167,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   void
   __throw_ios_failure(const char* str __attribute__((unused)),
 		      int err __attribute__((unused)))
-  { _GLIBCXX_THROW_OR_ABORT(__ios_failure(_(str), err)); }
+  {
+    _GLIBCXX_THROW_OR_ABORT(__ios_failure(_(str),
+	  err ? error_code(err, generic_category()) : io_errc::stream));
+  }
 
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace