On 26 November 2015 at 13:04, Paolo Bonzini <[email protected]> wrote: > > > On 26/11/2015 12:28, Peter Maydell wrote: >>> But we are relying on them, and thus we should document them. Witness >>> the number of patches fixing so called "undefined" behavior. And those >>> patches are _dangerous_. >> >> Until and unless the compiler guarantees us the semantics that >> we want, then silently hoping we get something we're not getting >> is even more dangerous, and avoiding the UB is better than >> just crossing our fingers and hoping. >> >>> I can certainly remove the "as documented by the GCC manual" part and >>> the -fwrapv setting, but silencing -Wshift-negative-value and >>> documenting what we rely on should go in. >> >> I don't see much point in documenting what we rely on >> if we can't rely on it and need to stop relying on it. > > I'm having a hard, hard time believing that we can't rely on it. And if > we can rely on it, we don't need to stop relying on it.
Really my concern here is simply an ordering one: we should (1) confirm with the clang and gcc developers that what we think -fwrapv means is what they agree (and will document) as what it means (2) update our makefiles/docs/etc appropriately and also I think that (1) is the significant part of the exercise, whereas (2) is just a cleanup/tidyup that we can do afterwards. This patch is doing (2) before we've done (1). > To sum up: > > - GCC promises that signed shift of << is implementation-defined except > for overflow in constant expressions, and defines it as operating on bit > values. This was approved. For GCC, -fwrapv does not even apply except > again for constant expressions. > > - signed shift of negative values in constant expressions _are_ covered > by GCC's promise. The implementation-defined behavior of signed << > gives a meaning to all signed shifts, and all that the C standard > requires is "Each constant expression shall evaluate to a constant that > is in the range of representable values for its type" (6.6p4). > Therefore we should at least disable -Wshift-negative-value, because it > doesn't buy us anything. > > - regarding overflow, in addition to the weird -Wpedantic warning, GCC 6 > adds a new -Wshift-overflow flag which is enabled by default in C99 and > C11 modes, and which only applies to constant expressions. So the > remaining case where the compiler may change its behavior on overflow, > i.e. constant expressions, will thus be caught by our usage of -Werror > (because -Wshift-overflow is enabled by default). So, independent of > the limited scope of GCC's promise, with GCC 6 we will never have > constant expressions that overflow because of left shifts. I'm confused by all this text about constant expressions. Does -fwrapv guarantee that signed shift of << behaves as we want in all situations (including constant expressions) or doesn't it? If it doesn't do that for constant expressions I don't think we should rely on it, because it's way too confusing to have "this is OK except when it isn't OK". (And a lot of our uses of "-1 << 8" are indeed in constant expressions.) > - if a compiler actually treated signed << overflow undefined behavior, > a possible fix would be to use C89 mode (-std=gnu89), since signed << is > unspecified there rather than undefined. With C89, GCC's promise is > complete. We do use C89 with GCC <= 4.9 anyway, it makes sense to be > homogeneous across all supported compilers. "unspecified" isn't a great deal better than "undefined" really. > Now, -fwrapv was really included in my patch only to silence ubsan in > the future. I think it should, and I will work on patches to fix that. > However, an advantage of -std=gnu89 is that it silences ubsan _now_ at > least for GCC. So let's just drop -fwrapv and use -std=gnu89 instead. > This buys us time, and in the meanwhile I'll gett -fwrapv complete in GCC. > > If this is okay, I'll remove the patch, respin the pull request, and > post a new configure change to use -std=gnu89. I don't think this gains us much as a different approach, and it's still trying to do cleanup (2) before we have dealt with the main issue (1). > Yes, we haven't heard anything from clang developers. But clang does > not even document implementation-defined behavior > (https://llvm.org/bugs/show_bug.cgi?id=11272). The bug says: > >> The clang documentation should specify how clang behaves in cases >> specified to be implementation-defined in the relevant standards. >> Perhaps simply saying that our behavior is the same as GCC's would suffice? > > This is about implementation-defined rather than undefined behavior, but > I think it's enough to not care about clang developer's silence. I disagree here. I think it's important to get the clang developers to tell us what they mean by -fwrapv and what they want to guarantee about signed shifts. That's because if they decide to say "no, we don't guarantee behaviour here because the C spec says it's UB" then we need to change how we deal with these in QEMU. thanks -- PMM
