Re: [lmi] How to deal with this instance of UB?

[Vadim Zeitlin]

On Thu, 16 Jun 2022 20:30:09 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> Vadim--The patch below shows two ways of dealing with UB at a
GC> particular point in fdlibm; of course want to choose one way.
GC> 
GC> The first way is to use an __attribute__ that blinds UBSan to
GC> the problem. I dislike doing that, except as a last resort
GC> with a compelling reason. OTOH, this is UB only because the C
GC> standard committee says so; if (as is unlikely) I understand
GC> what C++20 says, it's no longer UB in C++ because integers are
GC> required to be two's complement;

 According to https://en.cppreference.com/w/cpp/language/operator_arithmetic
the behaviour of "a << b" is undefined for negative "a" only until C++20,
but is still undefined for negative (or too big) "b" even in C++20. I admit
I haven't bothered verifying it myself, but cppreference.com is rarely
wrong (maybe even never, although it is sometimes incomplete), so I just
tend to trust them. Moreover, compiling the same source file using the
shift operator with -std=c++17 and -std=c++20 and -fsanitize=undefined
confirms this, so at least de facto this is correct.

GC> The second way is to use casts to replace UB with defined
GC> behavior. That seems like the righteous path, and I think
GC> I've followed it correctly below, but I'd rather not commit
GC> this without your prior review.

 This is definitely not going to result in undefined behaviour, but I'm not
really sure what is the intention of the current code. I.e. is it really
expected that shifting a negative number left can result in a positive
number? OTOH I guess this doesn't really matter, as you didn't ask me to
confirm that the old code was correct, just that your change didn't change
its behaviour on a two's complement machine -- and I'm pretty sure that
this is indeed the case, as both version compile to the same assembly
instructions (shl) on x86. Of course, this doesn't prove anything in
general, but proving something about undefined behaviour is impossible by
definition, so I think this is the best we can do -- even if in theory C
compiler could do anything for the old code, in practice it produces the
same code for it as for the new version (for which it can produce nothing
else).

GC> So my questions are two:
GC>  - Do you agree that the second way is better in principle?

 Yes, of course.

GC>  - Have I implemented the second way correctly?

 I won't add to my already very wordy answer above, so let me just say:
yes, I think so.

 Sorry if you hoped for something more definitive, but I'm really just
lawyering up here, for all practical intents I'm pretty sure that your
change is correct. And, again, I'm absolutely sure it fixed the UB here.

 Thanks,
VZ

pgpqkRLNR9DPJ.pgp
Description: PGP signature