[Bug libstdc++/108674] New: [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[Bug libstdc++/108674] New: [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[lebedev.ri at gmail dot com]

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

            Bug ID: 108674
           Summary: [wish] *Please* silence *intentional* (non-UB!)
                    unsigned overflow in an libstdc++ header
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: lebedev.ri at gmail dot com
  Target Milestone: ---

Dear maintainer.

As everyone knows, unsigned integer overflow is well-defined in C and C++.
However, there are situations where you *know* that a particular code
should not have any overflows. To catch them, there's Integer Sanitizer
in clang (`-fsanitize=integer`). 

Unfortunately as one would expect, while some might want to have no
unsigned overflows, others may very well depend on the defined behavior.
As is the case, the GCC, and in particular libstdc++ fall into the
latter category.

I believe in the version 12, a new instance of such intentional wraparound
was introduced into libstdc++: https://godbolt.org/z/rq153fxKW
Running this on a debian machine, we get:
```
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/basic_string.h:483:51:
runtime error: unsigned integer overflow: 4 - 6 cannot be represented in type
'size_type' (aka 'unsigned long')
    #0 0x55e69e5b6818 in std::__cxx11::basic_string<char,
std::char_traits<char>, std::allocator<char>>::_S_compare(unsigned long,
unsigned long)
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/basic_string.h:483:51
    #1 0x55e69e5b6818 in std::__cxx11::basic_string<char,
std::char_traits<char>,
std::allocator<char>>::compare(std::__cxx11::basic_string<char,
std::char_traits<char>, std::allocator<char>> const&) const
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/basic_string.h:3150:10
<...>
```

I understand that there is no UB there.
I understand that you are doing this intentionally.
The problem is that it is happening in a header,
so it's effectively dictating everyone
that they should not use that sanitizer.

Silencing this kind of thing from user side is possible,
but it's somewhat cumbersome: it requires compiling with
`-fsanitize-recover=integer`, and supplying a run-time suppressions file.

On the other hand, suppressing this in-source is trivial:
https://godbolt.org/z/E7sEnvvrT
... all it would take is applying
`__attribute__((no_sanitize("unsigned-integer-overflow")))`
to `_S_compare` on line 483 in `basic_string.h`.

I have tried that locally, and it works, but it seems it needs to be
wrapped into `#if defined(__clang__)` preprocessor check:
https://godbolt.org/z/5a7ox4EWv

Forwarded from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1029970

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think this is a bug in clang in the first place for enabling
unsigned-integer-overflow at all.
I would file a bug with clang to disable unsigned-integer-overflow by default
when using -fsanitize=undefined .
GCC has already decided to never implement unsigned-integer-overflow even
because of how broken this is.

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=91547,
                   |                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=81749
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |DUPLICATE

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Exact dup of bug 97844.

*** This bug has been marked as a duplicate of bug 97844 ***

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[lebedev.ri at gmail dot com]

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

Roman Lebedev <lebedev.ri at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|DUPLICATE                   |---
             Status|RESOLVED                    |UNCONFIRMED

--- Comment #3 from Roman Lebedev <lebedev.ri at gmail dot com> ---
(In reply to Andrew Pinski from comment #1)
> I think this is a bug in clang in the first place for enabling
> unsigned-integer-overflow at all.
> I would file a bug with clang to disable unsigned-integer-overflow by
> default when using -fsanitize=undefined .
This is incorrect. 
unsigned-integer-overflow is *NOT* enabled by -fsanitize=undefined
It is enabled by -fsanitize=integer, separately.
I'm not enabling it erroneously, but very intentionally.

> GCC has already decided to never implement unsigned-integer-overflow even
> because of how broken this is.

This is quite the hot take.
I understand that the behaviours it diagnoses
are well-defined by the C and C++ standards,
and are well-used in various codebases,
however not all of those behaviors are desired by everyone.

For example, ((signed char)127)+1 is not a signed overflow,
even though you really can't tell me that the effective wraparound
is the semantics *everyone* expects there. :)

However, that is not the question here.
I really don't care whether or not you rely on the wraparound semantics
of the unsigned types in the library. I really don't.
This is only a problem because it happens in a public header.

All i'm asking is to improve the UX of the user-facing side
of the C++ standard library implementation,
and make it more usable by wider variety of the scenarios.

In fact, this is a regression.
This was not happening in libstdc++-11, or ever before.

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |DUPLICATE

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Still is exact dup of bug 97844. Does not matter if it was not happening in gcc
11 or not. Still a dup.

*** This bug has been marked as a duplicate of bug 97844 ***

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[pinskia at gcc dot gnu.org]

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

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
> This is quite the hot take.

Hot take from 5 years ago. See te other bugs I referenced and even the mailing
list emails that are referenced from there. Rather clang is the one who decided
this breaking behavior.

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[pinskia at gcc dot gnu.org]

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

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
See https://gcc.gnu.org/legacy-ml/gcc/2016-07/msg00051.html .
Sorry when I said 5 years I meant 7 years.

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[redi at gcc dot gnu.org]

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

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Roman Lebedev from comment #0)
> I believe in the version 12, a new instance of such intentional wraparound
> was introduced into libstdc++: https://godbolt.org/z/rq153fxKW

No, that code is from 2014-12-19.

> I understand that there is no UB there.
> I understand that you are doing this intentionally.
> The problem is that it is happening in a header,
> so it's effectively dictating everyone
> that they should not use that sanitizer.

Well, they shouldn't use it *and expect everybody else's code to never rely on
unsigned wraparound*. If they want to write their own code to never rely on
that guaranteed feature of the language, that's fine. They don't get to force
that choice on everybody else.


> Silencing this kind of thing from user side is possible,
> but it's somewhat cumbersome: it requires compiling with
> `-fsanitize-recover=integer`, and supplying a run-time suppressions file.
> 
> On the other hand, suppressing this in-source is trivial:
> https://godbolt.org/z/E7sEnvvrT
> ... all it would take is applying
> `__attribute__((no_sanitize("unsigned-integer-overflow")))`
> to `_S_compare` on line 483 in `basic_string.h`.
> 
> I have tried that locally, and it works, but it seems it needs to be
> wrapped into `#if defined(__clang__)` preprocessor check:
> https://godbolt.org/z/5a7ox4EWv
> 
> Forwarded from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1029970

So provide a patch then, instead of asking other people to work around this
sanitizer for you.

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[redi at gcc dot gnu.org]

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

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Roman Lebedev from comment #3)
> This is incorrect. 
> unsigned-integer-overflow is *NOT* enabled by -fsanitize=undefined
> It is enabled by -fsanitize=integer, separately.
> I'm not enabling it erroneously, but very intentionally.

But it still incorrectly claims there is undefined behaviour. Your own godbolt
link shows:

UndefinedBehaviorSanitizer: undefined-behavior ...

We've talked about this before, see PR 97844.

> All i'm asking is to improve the UX of the user-facing side
> of the C++ standard library implementation,
> and make it more usable by wider variety of the scenarios.

Please stop asking other people to work around broken tools that they don't use
themselves.

You want a workaround, propose a patch.

> In fact, this is a regression.
> This was not happening in libstdc++-11, or ever before.

The code is nearly a decade old, and you even commented on PR 97844 about it
happening in gcc 10. So I don't know where you get the idea it's new in gcc-12.

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[lebedev.ri at gmail dot com]

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

--- Comment #9 from Roman Lebedev <lebedev.ri at gmail dot com> ---
(In reply to Jonathan Wakely from comment #7)
> (In reply to Roman Lebedev from comment #0)
> > I believe in the version 12, a new instance of such intentional wraparound
> > was introduced into libstdc++: https://godbolt.org/z/rq153fxKW
> 
> No, that code is from 2014-12-19.
I agree that the _S_compare is rather old.
What i'm saying is that i have never seen this issue before v12,
so std::map implementation must have been refactored
to use the function in question, and that "introduced" the issue.

> > I understand that there is no UB there.
> > I understand that you are doing this intentionally.
> > The problem is that it is happening in a header,
> > so it's effectively dictating everyone
> > that they should not use that sanitizer.
> 
> Well, they shouldn't use it *and expect everybody else's code to never rely
> on unsigned wraparound*. If they want to write their own code to never rely
> on that guaranteed feature of the language, that's fine. They don't get to
> force that choice on everybody else.
Right, of course. As i have said, if i was compiling gcc/libstdc++ itself
with that "broken" sanitizer, closing as WONTFIX would be totally justified,
but here we are in a bit of a gray area, because while that is for sure
a part of implementation, it's rather a user-facing one.

> > Silencing this kind of thing from user side is possible,
> > but it's somewhat cumbersome: it requires compiling with
> > `-fsanitize-recover=integer`, and supplying a run-time suppressions file.
> > 
> > On the other hand, suppressing this in-source is trivial:
> > https://godbolt.org/z/E7sEnvvrT
> > ... all it would take is applying
> > `__attribute__((no_sanitize("unsigned-integer-overflow")))`
> > to `_S_compare` on line 483 in `basic_string.h`.
> > 
> > I have tried that locally, and it works, but it seems it needs to be
> > wrapped into `#if defined(__clang__)` preprocessor check:
> > https://godbolt.org/z/5a7ox4EWv
> > 
> > Forwarded from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1029970
> 
> So provide a patch then, instead of asking other people to work around this
> sanitizer for you.

:)

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[lebedev.ri at gmail dot com]

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

--- Comment #10 from Roman Lebedev <lebedev.ri at gmail dot com> ---
Created attachment 54409
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54409&action=edit
the patch

I'm not at all familiar with the GCC's preferred patch protocol,
this is the result of `git format-patch origin/master`,
with commit message mimicking the ones of the recent commits.
Please let me know what i got wrong this time.

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[redi at gcc dot gnu.org]

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

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Roman Lebedev from comment #9)
> (In reply to Jonathan Wakely from comment #7)
> > (In reply to Roman Lebedev from comment #0)
> > > I believe in the version 12, a new instance of such intentional wraparound
> > > was introduced into libstdc++: https://godbolt.org/z/rq153fxKW
> > 
> > No, that code is from 2014-12-19.
> I agree that the _S_compare is rather old.
> What i'm saying is that i have never seen this issue before v12,
> so std::map implementation must have been refactored
> to use the function in question, and that "introduced" the issue.

Again no. A map of strings has always compared the strings, and that has always
used that function. Your godbolt example has exactly the same behaviour with
GCC 7 (I didn't bother testing anything older):

tmp$ cat wrap.cc
#include <string>
#include <map>

enum class BitOrder {
  LSB,   /* Memory order */
  MSB,   /* Input is added to stack byte by byte, and output is lifted
                     from top */
  MSB16, /* Same as above, but 16 bits at the time */
  MSB32, /* Same as above, but 32 bits at the time */
};

const std::map<std::string, BitOrder, std::less<>> order2enum = {
    {"plain", BitOrder::LSB},
    {"jpeg", BitOrder::MSB},
    {"jpeg16", BitOrder::MSB16},
    {"jpeg32", BitOrder::MSB32},
};

int main() {
  return order2enum.find("plain") == order2enum.end();
}

tmp$ clang++ --gcc-toolchain=$HOME/gcc/7 wrap.cc -O3
-fsanitize=unsigned-integer-overflow 
tmp$ ./a.out
/home/jwakely/gcc/7/lib/gcc/x86_64-pc-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/basic_string.h:392:51:
runtime error: unsigned integer overflow: 4 - 6 cannot be represented in type
'unsigned long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
/home/jwakely/gcc/7/lib/gcc/x86_64-pc-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/basic_string.h:392:51
in

[Bug libstdc++/108674] [wish] *Please* silence *intentional* (non-UB!) unsigned overflow in an libstdc++ header

[redi at gcc dot gnu.org]

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

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Roman Lebedev from comment #10)
> I'm not at all familiar with the GCC's preferred patch protocol,

See https://gcc.gnu.org/contribute.html#patches

> this is the result of `git format-patch origin/master`,

That format is great.

> with commit message mimicking the ones of the recent commits.
> Please let me know what i got wrong this time.

The commit message should be in the form "libstdc++: ..."

I would just use "libstdc++: silence clang's Integer Sanitizer [PR108674]".

The no_sanitize attribute needs to be __no_sanitize__ instead, because this is
a valid program:

#define no_sanitize !!1!1
#include <string>
int main() { }


Otherwise this looks reasonable. The
__no_sanitize__("unsigned-integer-overflow") attribute seems to be supported by
all non-ancient versions of Clang.

Also, please note that the Signed-off-by: tag has the meaning described at
https://gcc.gnu.org/dco.html so please be sure that's what you intend when
using it :-)

Please make the changes above and post to the gcc-patches list, CCing the
libstdc++ list, thanks!