Message ID | 1433367941-119488-1-git-send-email-agraf@suse.de |
---|---|
State | New |
Headers | show |
On 3 June 2015 at 22:45, Alexander Graf <agraf@suse.de> wrote: > Hi Peter, > > This is my current patch queue for ppc. Please pull. > > Alex > > > The following changes since commit 3fc827d591679f3e262b9d1f8b34528eabfca8c0: > > target-arm: Correct check for non-EL3 (2015-06-02 13:22:29 +0100) > > are available in the git repository at: > > git://github.com/agraf/qemu.git tags/signed-ppc-for-upstream > > for you to fetch changes up to e229d3cc64420204cdb40b983ce08eae657812f9: > > softmmu: support up to 12 MMU modes (2015-06-03 23:42:13 +0200) > > ---------------------------------------------------------------- > Patch queue for ppc - 2015-06-03 > > Highlights this time around: > > - sPAPR: endian fixes, speedups, bug fixes, hotplug basics > - add default ram size capability for machines (sPAPR defaults to 512MB now) > > ---------------------------------------------------------------- Applied (the updated version), thanks. -- PMM
On 3 June 2015 at 22:45, Alexander Graf <agraf@suse.de> wrote: > Hi Peter, > > This is my current patch queue for ppc. Please pull. This is applied, but can you fix the clang sanitizer warnings, please? hw/ppc/spapr_drc.c:59:24: runtime error: left shift of negative value -1 hw/ppc/spapr_drc.c:587:19: runtime error: left shift of negative value -1 Problem looks to be in: #define DRC_INDEX_ID_MASK (~(~0 << DRC_INDEX_TYPE_SHIFT)) which is doing left shifts on a negative signed number. thanks -- PMM
On 05/06/2015 15:33, Peter Maydell wrote: > This is applied, but can you fix the clang sanitizer warnings, > please? > > hw/ppc/spapr_drc.c:59:24: runtime error: left shift of negative value -1 > hw/ppc/spapr_drc.c:587:19: runtime error: left shift of negative value -1 > > Problem looks to be in: > #define DRC_INDEX_ID_MASK (~(~0 << DRC_INDEX_TYPE_SHIFT)) > > which is doing left shifts on a negative signed number. Speaking in general, I find that this makes code worse. If you're using ~0 you probably want the value to extend with infinite ones. Using ~0u instead of ~0ull may cause problems down the line, and ~0ul is even worse because it is not 64-bit safe. Paolo
On 5 June 2015 at 15:35, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 05/06/2015 15:33, Peter Maydell wrote: >> This is applied, but can you fix the clang sanitizer warnings, >> please? >> >> hw/ppc/spapr_drc.c:59:24: runtime error: left shift of negative value -1 >> hw/ppc/spapr_drc.c:587:19: runtime error: left shift of negative value -1 >> >> Problem looks to be in: >> #define DRC_INDEX_ID_MASK (~(~0 << DRC_INDEX_TYPE_SHIFT)) >> >> which is doing left shifts on a negative signed number. > > Speaking in general, I find that this makes code worse. If you're using > ~0 you probably want the value to extend with infinite ones. > > Using ~0u instead of ~0ull may cause problems down the line, and ~0ul > is even worse because it is not 64-bit safe. I agree that C's semantics are terrible here (ideally left shift of negative values should Just Work in the 2s complement style, and right shift of negative values should be an arithmetic shift). Unfortunately we're stuck with the standard, which says this is undefined behaviour :-( -- PMM
On 05/06/2015 16:40, Peter Maydell wrote: > > Speaking in general, I find that this makes code worse. If you're using > > ~0 you probably want the value to extend with infinite ones. > > > > Using ~0u instead of ~0ull may cause problems down the line, and ~0ul > > is even worse because it is not 64-bit safe. > > I agree that C's semantics are terrible here (ideally > left shift of negative values should Just Work in the 2s > complement style, and right shift of negative values should > be an arithmetic shift). Unfortunately we're stuck with > the standard, which says this is undefined behaviour :-( But this is not something that C compiler writers can reasonably change. Can someone add a checkpatch rule that forbids shifting left U or UL constants (i.e. only ULL)? That would alleviate my concerns with these ubsan warnings. Paolo
On 5 June 2015 at 16:02, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 05/06/2015 16:40, Peter Maydell wrote: >> > Speaking in general, I find that this makes code worse. If you're using >> > ~0 you probably want the value to extend with infinite ones. >> > >> > Using ~0u instead of ~0ull may cause problems down the line, and ~0ul >> > is even worse because it is not 64-bit safe. >> >> I agree that C's semantics are terrible here (ideally >> left shift of negative values should Just Work in the 2s >> complement style, and right shift of negative values should >> be an arithmetic shift). Unfortunately we're stuck with >> the standard, which says this is undefined behaviour :-( > > But this is not something that C compiler writers can reasonably change. Right, which is why we need to change our code to not invoke undefined behaviour. (More generally, C compiler writers can agitate with the C standards bodies to get ideas like "friendly C" dialects accepted, http://blog.regehr.org/archives/1180, and they can provide them as vendor extensions too.) > Can someone add a checkpatch rule that forbids shifting left U or UL > constants (i.e. only ULL)? That would alleviate my concerns with these > ubsan warnings. ...but things like "(1U << 31)" are entirely valid. That's the reason these warnings are runtime rather than compile time in the first place... thanks -- PMM
On 05/06/2015 17:08, Peter Maydell wrote: >> > Can someone add a checkpatch rule that forbids shifting left U or UL >> > constants (i.e. only ULL)? That would alleviate my concerns with these >> > ubsan warnings. > > ...but things like "(1U << 31)" are entirely valid. They're only valid until someone does a ~ on them. I think it's reasonable to forbid them in our coding standards, if we want to fix ubsan's warning of (1 << 31). I don't think it's reasonable for compiler writers to exploit the undefinedness of (1 << 31) anyway, and if it were possible to shut up ubsan about this particular kind of undefined behavior, I would prefer it. Paolo
On 06/05/2015 07:33 AM, Peter Maydell wrote: > On 3 June 2015 at 22:45, Alexander Graf <agraf@suse.de> wrote: >> Hi Peter, >> >> This is my current patch queue for ppc. Please pull. > > This is applied, but can you fix the clang sanitizer warnings, > please? > > hw/ppc/spapr_drc.c:59:24: runtime error: left shift of negative value -1 > hw/ppc/spapr_drc.c:587:19: runtime error: left shift of negative value -1 > > Problem looks to be in: > #define DRC_INDEX_ID_MASK (~(~0 << DRC_INDEX_TYPE_SHIFT)) > > which is doing left shifts on a negative signed number. Might be as simple as using ~0U instead of ~0.
On 5 June 2015 at 16:20, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 05/06/2015 17:08, Peter Maydell wrote: >>> > Can someone add a checkpatch rule that forbids shifting left U or UL >>> > constants (i.e. only ULL)? That would alleviate my concerns with these >>> > ubsan warnings. >> >> ...but things like "(1U << 31)" are entirely valid. > > They're only valid until someone does a ~ on them. I think it's > reasonable to forbid them in our coding standards, if we want to fix > ubsan's warning of (1 << 31). > > I don't think it's reasonable for compiler writers to exploit the > undefinedness of (1 << 31) anyway, and if it were possible to shut up > ubsan about this particular kind of undefined behavior, I would prefer it. I don't think it's reasonable for compiler writers to exploit undefined behaviour either, but historically they absolutely have done. Absent a guarantee from gcc that it will never do so, I think we should avoid any UB in our code. There's also the usual rationale that it's worth fixing the borderline things that provoke complaints from lint-like tools in order that new warnings don't get lost in the pile of old uninteresting warnings. thanks -- PMM
On 05/06/2015 17:45, Peter Maydell wrote: >>> ...but things like "(1U << 31)" are entirely valid. >> >> They're only valid until someone does a ~ on them. I think it's >> reasonable to forbid them in our coding standards, if we want to fix >> ubsan's warning of (1 << 31). >> >> I don't think it's reasonable for compiler writers to exploit the >> undefinedness of (1 << 31) anyway, and if it were possible to shut up >> ubsan about this particular kind of undefined behavior, I would prefer it. > > I don't think it's reasonable for compiler writers to exploit > undefined behaviour either, but historically they absolutely > have done. Most cases of undefined behavior are rooted in "you should never do that anyway". This is not the case for bitwise operations, since they are not mathematical concepts and the representation of integers as bits is only implementation-defined. > Absent a guarantee from gcc that it will never do > so, I think we should avoid any UB in our code. The GCC manual says "GCC does not use the latitude given in C99 and C11 only to treat certain aspects of signed '<<' as undefined, but this is subject to change". It would certainly be nice if they removed the "this is subject to change" part. Paolo
On Fri, 5 Jun 2015, Paolo Bonzini wrote: > The GCC manual says "GCC does not use the latitude given in C99 and C11 > only to treat certain aspects of signed '<<' as undefined, but this is > subject to change". It would certainly be nice if they removed the > "this is subject to change" part. The correct statement would be more complicated. That is: the value returned is as documented, without that latitude being used for *optimization*, but (a) -fsanitize=undefined (and its subcase -fsanitize=shift) intends to follow exactly what the different standards specify when giving runtime errors and (b) the cases that are undefined are thereby not considered integer constant expressions (with consequent pedwarns-if-pedantic in various cases, and corner case effects on what's a null pointer constant). (The only "subject to change" would be that if there are still missing cases from the runtime detection or the not treating as integer constant expressions, then those missing cases may be fixed. I don't think it would be a good idea to add optimizations on this basis - for example, optimizations of x * 2 based on undefined overflow should not be applied to x << 1.)
On 5 June 2015 at 16:55, Paolo Bonzini <pbonzini@redhat.com> wrote: > The GCC manual says "GCC does not use the latitude given in C99 and C11 > only to treat certain aspects of signed '<<' as undefined, but this is > subject to change". It would certainly be nice if they removed the > "this is subject to change" part. Does clang provide a similar guarantee? I couldn't find one in a quick scan through the docs, but I might be looking in the wrong place. thanks -- PMM
On 5 June 2015 at 16:20, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 05/06/2015 17:08, Peter Maydell wrote: >>> > Can someone add a checkpatch rule that forbids shifting left U or UL >>> > constants (i.e. only ULL)? That would alleviate my concerns with these >>> > ubsan warnings. >> >> ...but things like "(1U << 31)" are entirely valid. > > They're only valid until someone does a ~ on them. I think it's > reasonable to forbid them in our coding standards, if we want to fix > ubsan's warning of (1 << 31). > > I don't think it's reasonable for compiler writers to exploit the > undefinedness of (1 << 31) anyway, and if it were possible to shut up > ubsan about this particular kind of undefined behavior, I would prefer it. I just checked, and it isn't possible to suppress this particular shift warning without also turning off others that we would want to retain. The ppc code is still provoking this sanitizer warning in my test runs :-( thanks -- PMM
Quoting Peter Maydell (2015-07-21 06:42:41) > On 5 June 2015 at 16:20, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 05/06/2015 17:08, Peter Maydell wrote: > >>> > Can someone add a checkpatch rule that forbids shifting left U or UL > >>> > constants (i.e. only ULL)? That would alleviate my concerns with these > >>> > ubsan warnings. > >> > >> ...but things like "(1U << 31)" are entirely valid. > > > > They're only valid until someone does a ~ on them. I think it's > > reasonable to forbid them in our coding standards, if we want to fix > > ubsan's warning of (1 << 31). > > > > I don't think it's reasonable for compiler writers to exploit the > > undefinedness of (1 << 31) anyway, and if it were possible to shut up > > ubsan about this particular kind of undefined behavior, I would prefer it. > > I just checked, and it isn't possible to suppress this particular > shift warning without also turning off others that we would want > to retain. > > The ppc code is still provoking this sanitizer warning in my > test runs :-( Was I being silly by not just doing this is the first place? -#define DRC_INDEX_ID_MASK (~(~0 << DRC_INDEX_TYPE_SHIFT)) +#define DRC_INDEX_ID_MASK ((1 << DRC_INDEX_TYPE_SHIFT) - 1) Will send a proper patch to list in a moment. > > thanks > -- PMM >