Re: unexpected result with -O2 solved via "volatile"
On 19/09/2021 20:17, Allin Cottrell via Gcc wrote:
> Should this perhaps be considered a bug? Below is a minimal test case
> for a type of calculation that occurs in my real code. It works as
> expected when compiled without optimization, but produces what seems
> like a wrong result when compiled with -O2, using both gcc 10.3.1
> 20210422 on Fedora and gcc 11.1.0-1 on Arch Linux. I realize there's a
> newer gcc release but it's not yet available for Arch, and looking at
> https://gcc.gnu.org/gcc-11/changes.html I didn't see anything to suggest
> that something relevant has changed.
>
>
> #include
> #include
>
> int test (int *pk, int n)
> {
> int err = 0;
>
> if (*pk > n) {
> err = 1;
> if (*pk > 2e9) {
> int k = *pk + n - INT_MAX;
>
> *pk = k;
> if (k > 0) {
> printf("Got positive revised k = %d\n", k);
> err = 0;
> } else {
> printf("k = %d tests as non-positive?!\n", k);
> }
> }
> }
>
> return err;
> }
>
> int main (void)
> {
> int k = INT_MAX - 10;
> int err;
>
> err = test(&k, 20);
> printf("main: err = %d\n", err);
>
> return 0;
> }
>
>
> What strikes me as "seems wrong" is that the "(k > 0)" branch in test()
> is not taken, although in the alternative branch it turns out that k =
> 10. This can be fixed by using the "volatile" keyword in front of the
> statement "int k = *pk + n - INT_MAX;" or by parenthesizing (n -
> INT_MAX) in that statement.
>
> I can see the case for assuming that k can't be positive if one thinks
> of the expression as (*pk + n) - INT_MAX, since (*pk + n) can't be
> greater than INT_MAX in context, being the sum of two ints. All the
> same, since gcc does in fact end up assigning the value 10 to k the
> optimization seems a risky one.
>
Your code is broken. Signed integer overflow is undefined behaviour in
C - it has no meaning, and the compiler can assume it does not happen or
that you don't care what results you get if it /does/ happen.
This gives the compiler a lot of small but useful optimisation
opportunities - it can assume a number of basic mathematical identities
apply, and can use these to simplify code.
In this particular case, it knows that "*pk + n" will result in a valid
int value between INT_MIN and INT_MAX inclusive (or that you don't care
what happens if you try to overflow), with that int value being the
mathematically correct result as if the types had unlimited sizes. It
also knows that when it takes an int "x" and subtracts INT_MAX, it again
has an int result between INT_MIN and INT_MAX that has the correct
mathematical value. It is thus a simple reasoning that, regardless of
the value of "x", the result of "k = x - INT_MAX;" must lie between
INT_MIN and 0 inclusive. The "k > 0" branch cannot be taken, and code
generation for the conditional and the branch can be skipped.
You appear to be assuming that signed integer arithmetic has two's
complement wrapping, which is not the case in C. (It's a common
misunderstanding. Another common misunderstanding is that it /should/
be wrapping, and C is a silly language for not doing so - more careful
thought and research will show that it is a /better/ language because it
makes this undefined behaviour.)
Now that you know the problem in your code (and that it is not a bug in
gcc), you should be able to find plenty of information about signed
integer arithmetic in C in whatever format you prefer (C standards,
blogs, stackoverflow, youtube, whatever suits you). There is also the
Usenet group comp.lang.c. This list is not appropriate, however, now
that you know it is nothing specific to gcc.
mvh.,
David
Re: [PING^2] Re: Fix 'hash_table::expand' to destruct stale Value objects
On Fri, Sep 17, 2021 at 5:52 PM Thomas Schwinge wrote: > > Hi! > > On 2021-09-17T15:03:18+0200, Richard Biener > wrote: > > On Fri, Sep 17, 2021 at 2:39 PM Jonathan Wakely > > wrote: > >> On Fri, 17 Sept 2021 at 13:08, Richard Biener > >> wrote: > >> > On Fri, Sep 17, 2021 at 1:17 PM Thomas Schwinge > >> > wrote: > >> > > On 2021-09-10T10:00:25+0200, I wrote: > >> > > > On 2021-09-01T19:31:19-0600, Martin Sebor via Gcc-patches > >> > > > wrote: > >> > > >> On 8/30/21 4:46 AM, Thomas Schwinge wrote: > >> > > >>> Ping -- we still need to plug the memory leak; see patch attached, > >> > > >>> [...] > > >> > > > Ping for formal approval (and review for using proper > >> > > > C++ terminology in the 'gcc/hash-table.h:hash_table::expand' source > >> > > > code > >> > > > comment that I'm adding). Patch again attached, for easy reference. > > >> > I'm happy when a C++ literate approves the main change which I quote as > >> > > >> > new ((void*) q) value_type (std::move (x)); > >> > + > >> > + /* Manually invoke destructor of original object, to > >> > counterbalance > >> > +object constructed via placement new. */ > >> > + x.~value_type (); > >> > > >> > but I had the impression that std::move already "moves away" from the > >> > source? > >> > >> It just casts the argument to an rvalue reference, which allows the > >> value_type constructor to steal its guts. > >> > >> > That said, the dance above looks iffy, there must be a nicer way to > >> > "move" > >> > an object in C++? > >> > >> The code above is doing two things: transfer the resources from x to a > >> new object at location *q, and then destroy x. > >> > >> The first part (moving its resources) has nothing to do with > >> destruction. An object still needs to be destroyed, even if its guts > >> have been moved to another object. > >> > >> The second part is destroying the object, to end its lifetime. You > >> wouldn't usually call a destructor explicitly, because it would be > >> done automatically at the end of scope for objects on the stack, or > >> done by delete when you free obejcts on the heap. This is a special > >> case where the object's lifetime is manually managed in storage that > >> is manually managed. > > ACK, and happy that I understood this correctly. > > And, thanks for providing some proper C++-esque wording, which I > summarized to replace my original source code comment. > > >> > What happens if the dtor is deleted btw? > >> > >> If the destructor is deleted you have created an unusable type that > >> cannot be stored in containers. It can only be created using new, and > >> then never destroyed. If you play stupid games, you win stupid prizes. > >> Don't do that. > > Haha! ;-) > > And, by the way, as I understood this: if the destructor is "trivial" > (which includes POD types, for example), the explicit destructor call > here is a no-op. > > >> I haven't read the rest of the patch, but the snippet above looks fine. > > > > OK, thanks for clarifying. > > > > The patch is OK then. > > Thanks, pushed to master branch > commit 89be17a1b231ade643f28fbe616d53377e069da8 > "Fix 'hash_table::expand' to destruct stale Value objects". > > Should this be backported to release branches, after a while? You'd have to cross-check the status of C++ support in our containers there. If it is a memory leak fix then yes, but as said, something older than GCC 11 needs double-checking if it is a) affected and b) has other bits already. Richard. > > Grüße > Thomas > > > - > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 > München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas > Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht > München, HRB 106955
Re: [libc-coord] Add new ABI '__memcmpeq()' to libc
On Fri, Sep 17, 2021, at 9:36 PM, James Y Knight via Libc-alpha wrote: > Glibc currently implements bcmp as an alias to memcmp -- which is valid, > but provides more than just the boolean equality semantics. There was > concern raised that modifying that might break existing binaries. However, > this concern can be easily addressed in the same way glibc typically > addresses such compatibility concerns: creating a new symbol-version for > the new bcmp implementation, with the previous bcmp symbol-version > remaining as a memcmp alias. Not speaking for anyone but myself, but IMO this tactic has proven not to preserve _enough_ backward compatibility. Old binaries keep working, but old code breaks when recompiled with new glibc. It's especially troublesome to do this for a semantic change that manifests only at runtime; the break can go undetected for quite some time and then cause catastrophe when someone finally hits the relevant edge case. zw
Harald Anlauf appointed Fortran Reviewer
I am pleased to announce that the GCC Steering Committee has appointed Harald Anlauf as a Fortran Reviewer. Please join me in congratulating Harald on his new role. Harald, please update your listing in the MAINTAINERS file. Happy hacking! David
