| Message ID | or34ogjc6g.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | [libstdc++,testsuite] avoid arbitrary errno codes | expand |
On Thu, 11 Jul 2024 at 14:23, Alexandre Oliva <oliva@adacore.com> wrote: > > > Passing an arbitrary error number to strerror* functions doesn't seem > to be portable; 19_diagnostics/system_error/cons-1.cc is hitting > runtime errors in the block that attempts to instantiate a > std:;system_error for error number 95. The range of errno macros > defined on this target doesn't reach 95. The C standard is clear: "Typically, the values for errnum come from errno, but strerror shall map any value of type int to a message. " And std::system_error doesn't limit the values that can be passed to it either. So I'd prefer to keep testing with arbitrary int values, because that *should* work. > > I'm changing the test to try to use a couple of select error codes, > falling back to a lower error number if neither are present. > Hopefully this doesn't change the nature of what is being tested for. I think it does. > > Regstrapped on x86_64-linux-gnu, also tested with gcc-13 targeting > aarch64. Ok to install? > > > for libstdc++-v3/ChangeLog > > * testsuite/19_diagnostics/system_error/cons-1.cc: Use lower > error numbers, preferring some macro-defined ones. > --- > .../19_diagnostics/system_error/cons-1.cc | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/libstdc++-v3/testsuite/19_diagnostics/system_error/cons-1.cc b/libstdc++-v3/testsuite/19_diagnostics/system_error/cons-1.cc > index 16aa960b2ee28..e227c67542411 100644 > --- a/libstdc++-v3/testsuite/19_diagnostics/system_error/cons-1.cc > +++ b/libstdc++-v3/testsuite/19_diagnostics/system_error/cons-1.cc > @@ -37,8 +37,18 @@ int main() > > // 2 > { > - std::system_error err2(95, std::system_category(), s); > - VERIFY( err2.code() == std::error_code(95, std::system_category()) ); > + int eno; > +#if defined EOPNOTSUPP > + eno = EOPNOTSUPP; > +#elif defined ENOSYS > + eno = ENOSYS; > +#else > + // strerror (used to combine with the given message) may fail if > + // the error number is out of range for the system. > + eno = 42; > +#endif > + std::system_error err2(eno, std::system_category(), s); > + VERIFY( err2.code() == std::error_code(eno, std::system_category()) ); > VERIFY( std::string((err2.what(), s)).find(s) != std::string::npos ); > } > > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > More tolerance and less prejudice are key for inclusion and diversity > Excluding neuro-others for not behaving ""normal"" is *not* inclusive >
On Jul 11, 2024, Jonathan Wakely <jwakely@redhat.com> wrote: > And std::system_error doesn't limit the values that can be passed to > it either. So I'd prefer to keep testing with arbitrary int values, > because that *should* work. Fair enough, thanks. I've seen portability documentation suggesting that strerror returns NULL on some systems, if the error code is unknown, and it didn't seem to me that libstdc++ was prepared to deal with that. I'm not sure this system, on which I saw the failure, returns NULL or fails in some other way, but it's clear its behavior is not compliant with the relevant standards. It makes sense to test for this condition, though IMHO that makes it more of a libc test than a libstdc++ test, compared with the patched version I proposed. Not that I object to that :-) I just hadn't understood the goal. Patch withdrawn.
On Thu, 11 Jul 2024 at 17:23, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jul 11, 2024, Jonathan Wakely <jwakely@redhat.com> wrote: > > > And std::system_error doesn't limit the values that can be passed to > > it either. So I'd prefer to keep testing with arbitrary int values, > > because that *should* work. > > Fair enough, thanks. I've seen portability documentation suggesting > that strerror returns NULL on some systems, if the error code is > unknown, and it didn't seem to me that libstdc++ was prepared to deal > with that. I'm not sure this system, on which I saw the failure, > returns NULL or fails in some other way, but it's clear its behavior is > not compliant with the relevant standards. It makes sense to test for > this condition, though IMHO that makes it more of a libc test than a > libstdc++ test, There's no requirement that system_error uses strerror, that's just an implementation detail. So I think it makes sense to test at the user visible API level that system_error works with arbitrary values, even though that ends up depending on whether libc strerror also supports them. > compared with the patched version I proposed. Not that > I object to that :-) I just hadn't understood the goal. > > Patch withdrawn. > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > More tolerance and less prejudice are key for inclusion and diversity > Excluding neuro-others for not behaving ""normal"" is *not* inclusive >
On Jul 11, 2024, Jonathan Wakely <jwakely@redhat.com> wrote: > There's no requirement that system_error uses strerror, that's just an > implementation detail. *nod*. I meant it was more of a libc test in the case that relied on strerror. But I fully agree that testing the C++ standard API makes perfect sense. It's just that maybe we want workarounds for cases in which we implement in terms of libc, but libc doesn't live up to the standard. Tolerating NULL returns from strerror would be an easy one; do we want that? Checking strerror acceptable ranges (that don't trigger runtime errors) before calling it, and taking an alternate path when needed, that would be harder to do, and IMHO of dubious value.
On Fri, 12 Jul 2024 at 09:27, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jul 11, 2024, Jonathan Wakely <jwakely@redhat.com> wrote: > > > There's no requirement that system_error uses strerror, that's just an > > implementation detail. > > *nod*. I meant it was more of a libc test in the case that relied on > strerror. But I fully agree that testing the C++ standard API makes > perfect sense. It's just that maybe we want workarounds for cases in > which we implement in terms of libc, but libc doesn't live up to the > standard. Tolerating NULL returns from strerror would be an easy one; > do we want that? Checking strerror acceptable ranges (that don't > trigger runtime errors) before calling it, and taking an alternate path > when needed, that would be harder to do, and IMHO of dubious value. Yes, handling a null result from strerror seems sensible if that's the reality.
On Fri, 12 Jul 2024 at 10:27, Jonathan Wakely <jwakely@redhat.com> wrote: > > On Fri, 12 Jul 2024 at 09:27, Alexandre Oliva <oliva@adacore.com> wrote: > > > > On Jul 11, 2024, Jonathan Wakely <jwakely@redhat.com> wrote: > > > > > There's no requirement that system_error uses strerror, that's just an > > > implementation detail. > > > > *nod*. I meant it was more of a libc test in the case that relied on > > strerror. But I fully agree that testing the C++ standard API makes > > perfect sense. It's just that maybe we want workarounds for cases in > > which we implement in terms of libc, but libc doesn't live up to the > > standard. Tolerating NULL returns from strerror would be an easy one; > > do we want that? Checking strerror acceptable ranges (that don't > > trigger runtime errors) before calling it, and taking an alternate path > > when needed, that would be harder to do, and IMHO of dubious value. > > Yes, handling a null result from strerror seems sensible if that's the reality. Does this fix it for your target? --- a/libstdc++-v3/src/c++11/system_error.cc +++ b/libstdc++-v3/src/c++11/system_error.cc @@ -110,7 +110,11 @@ namespace #else string strerror_string(int err) { - return strerror(err); // XXX Not thread-safe. + auto str = strerror(err); // XXX Not thread-safe. + if (str) [[__likely__]] + return str; + // strerror should not return NULL, but some implementations do. + return "Unknown error"; } #endif
On Jul 12, 2024, Jonathan Wakely <jwakely@redhat.com> wrote: > On Fri, 12 Jul 2024 at 10:27, Jonathan Wakely <jwakely@redhat.com> wrote: >> >> On Fri, 12 Jul 2024 at 09:27, Alexandre Oliva <oliva@adacore.com> wrote: >> > >> > On Jul 11, 2024, Jonathan Wakely <jwakely@redhat.com> wrote: >> > >> > > There's no requirement that system_error uses strerror, that's just an >> > > implementation detail. >> > >> > *nod*. I meant it was more of a libc test in the case that relied on >> > strerror. But I fully agree that testing the C++ standard API makes >> > perfect sense. It's just that maybe we want workarounds for cases in >> > which we implement in terms of libc, but libc doesn't live up to the >> > standard. Tolerating NULL returns from strerror would be an easy one; >> > do we want that? Checking strerror acceptable ranges (that don't >> > trigger runtime errors) before calling it, and taking an alternate path >> > when needed, that would be harder to do, and IMHO of dubious value. >> >> Yes, handling a null result from strerror seems sensible if that's the reality. > Does this fix it for your target? IIRC calling strerror(95) was enough to trigger a runtime failure, regardless of system_error, so the problem on this specific target was not that strerror returned NULL. But the change you proposed will help other implementations that do, according to the [GNU/]Linux man-pages. Thanks!
diff --git a/libstdc++-v3/testsuite/19_diagnostics/system_error/cons-1.cc b/libstdc++-v3/testsuite/19_diagnostics/system_error/cons-1.cc index 16aa960b2ee28..e227c67542411 100644 --- a/libstdc++-v3/testsuite/19_diagnostics/system_error/cons-1.cc +++ b/libstdc++-v3/testsuite/19_diagnostics/system_error/cons-1.cc @@ -37,8 +37,18 @@ int main() // 2 { - std::system_error err2(95, std::system_category(), s); - VERIFY( err2.code() == std::error_code(95, std::system_category()) ); + int eno; +#if defined EOPNOTSUPP + eno = EOPNOTSUPP; +#elif defined ENOSYS + eno = ENOSYS; +#else + // strerror (used to combine with the given message) may fail if + // the error number is out of range for the system. + eno = 42; +#endif + std::system_error err2(eno, std::system_category(), s); + VERIFY( err2.code() == std::error_code(eno, std::system_category()) ); VERIFY( std::string((err2.what(), s)).find(s) != std::string::npos ); }