| Message ID | Zely50Q5nSR8eXev@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | sccvn: Avoid UB in ao_ref_init_from_vn_reference [PR105533] | expand |
On Thu, 7 Mar 2024, Jakub Jelinek wrote: > Hi! > > When compiling libgcc or on e.g. > int a[64]; > int p; > > void > foo (void) > { > int s = 1; > while (p) > { > s -= 11; > a[s] != 0; > } > } > sccvn invokes UB in the compiler as detected by ubsan: > ../../gcc/poly-int.h:1089:5: runtime error: left shift of negative value -40 > The problem is that we still use C++11..C++17 as the implementation language > and in those C++ versions shifting negative values left is UB (well defined > since C++20) and above in > offset += op->off << LOG2_BITS_PER_UNIT; > op->off is poly_int64 with -40 value (in libgcc with -8). > I understand the offset_int << LOG2_BITS_PER_UNIT shifts but it is then well > defined during underlying implementation which is done on the uhwi limbs, > but for poly_int64 we use > offset += pop->off * BITS_PER_UNIT; > a few lines earlier and I think that is both more readable in what it > actually does and triggers UB only if there would be signed multiply > overflow. In the end, the compiler will treat them the same at least at the > RTL level (at least, if not and they aren't the same cost, it should). > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. > 2024-03-07 Jakub Jelinek <jakub@redhat.com> > > PR middle-end/105533 > * tree-ssa-sccvn.cc (ao_ref_init_from_vn_reference) <case ARRAY_REF>: > Multiple op->off by BITS_PER_UNIT instead of shifting it left by > LOG2_BITS_PER_UNIT. > > --- gcc/tree-ssa-sccvn.cc.jj 2024-02-28 22:57:18.318658827 +0100 > +++ gcc/tree-ssa-sccvn.cc 2024-03-06 14:52:16.819229719 +0100 > @@ -1221,7 +1221,7 @@ ao_ref_init_from_vn_reference (ao_ref *r > if (maybe_eq (op->off, -1)) > max_size = -1; > else > - offset += op->off << LOG2_BITS_PER_UNIT; > + offset += op->off * BITS_PER_UNIT; > break; > > case REALPART_EXPR: > > > Jakub > >
--- gcc/tree-ssa-sccvn.cc.jj 2024-02-28 22:57:18.318658827 +0100 +++ gcc/tree-ssa-sccvn.cc 2024-03-06 14:52:16.819229719 +0100 @@ -1221,7 +1221,7 @@ ao_ref_init_from_vn_reference (ao_ref *r if (maybe_eq (op->off, -1)) max_size = -1; else - offset += op->off << LOG2_BITS_PER_UNIT; + offset += op->off * BITS_PER_UNIT; break; case REALPART_EXPR:
Hi! When compiling libgcc or on e.g. int a[64]; int p; void foo (void) { int s = 1; while (p) { s -= 11; a[s] != 0; } } sccvn invokes UB in the compiler as detected by ubsan: ../../gcc/poly-int.h:1089:5: runtime error: left shift of negative value -40 The problem is that we still use C++11..C++17 as the implementation language and in those C++ versions shifting negative values left is UB (well defined since C++20) and above in offset += op->off << LOG2_BITS_PER_UNIT; op->off is poly_int64 with -40 value (in libgcc with -8). I understand the offset_int << LOG2_BITS_PER_UNIT shifts but it is then well defined during underlying implementation which is done on the uhwi limbs, but for poly_int64 we use offset += pop->off * BITS_PER_UNIT; a few lines earlier and I think that is both more readable in what it actually does and triggers UB only if there would be signed multiply overflow. In the end, the compiler will treat them the same at least at the RTL level (at least, if not and they aren't the same cost, it should). Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2024-03-07 Jakub Jelinek <jakub@redhat.com> PR middle-end/105533 * tree-ssa-sccvn.cc (ao_ref_init_from_vn_reference) <case ARRAY_REF>: Multiple op->off by BITS_PER_UNIT instead of shifting it left by LOG2_BITS_PER_UNIT. Jakub