On Tue, Nov 19, 2024 at 9:55 AM Richard Biener <richard.guent...@gmail.com> wrote: > > On Sun, Nov 17, 2024 at 4:25 AM Lewis Hyatt <lhy...@gmail.com> wrote: > > > > The array gimple_ops_offset_[], which is used to find the trailing op[] > > array for a given gimple struct, is computed assuming that op[] will be > > found at sizeof(tree) bytes away from the end of the struct. This is only > > correct if the alignment requirement of a pointer is the same as the > > alignment requirement of the struct, otherwise there will be padding bytes > > that invalidate the calculation. On 64-bit platforms, this generally works > > fine because a pointer has 8-byte alignment and none of the structs make use > > of more than that. On 32-bit platforms, it also currently works fine because > > there are no 64-bit integers in the gimple structs. There are 32-bit > > platforms (e.g. sparc) on which a pointer has 4-byte alignment and a > > uint64_t has 8-byte alignment. On such platforms, adding a uint64_t to the > > gimple structs (as will take place when location_t is changed to be 64-bit) > > causes gimple_ops_offset_ to be 4 bytes too large. > > > > It would be nice to use offsetof() to compute the offset exactly, but > > offsetof() is not guaranteed to work for these types, because they use > > inheritance and so are not standard layout types. This patch attempts to > > detect the presence of tail padding by detecting when such padding is reused > > by inheritance; the padding should generally be reused for the same reason > > that offsetof() is not available, namely that all the relevant types use > > inheritance. One could envision systems on which this fix does not go far > > enough (e.g., if the ABI forbids reuse of tail padding), but it makes things > > better without affecting anything that currently works. > > > > gcc/ChangeLog: > > > > * gimple.cc (get_tail_padding_adjustment): New function. > > (DEFGSSTRUCT): Adjust the computation of gimple_ops_offset_ to be > > correct in the presence of tail padding. > > --- > > gcc/gimple.cc | 34 +++++++++++++++++++++++++++++----- > > 1 file changed, 29 insertions(+), 5 deletions(-) > > > > diff --git a/gcc/gimple.cc b/gcc/gimple.cc > > index f7b313be40e..f0a642f5b51 100644 > > --- a/gcc/gimple.cc > > +++ b/gcc/gimple.cc > > @@ -52,12 +52,36 @@ along with GCC; see the file COPYING3. If not see > > #include "ipa-modref.h" > > #include "dbgcnt.h" > > > > -/* All the tuples have their operand vector (if present) at the very bottom > > - of the structure. Therefore, the offset required to find the > > - operands vector the size of the structure minus the size of the 1 > > - element tree array at the end (see gimple_ops). */ > > +/* All the tuples have their operand vector (if present) at the very > > bottom of > > + the structure. Therefore, the offset required to find the operands > > vector is > > + the size of the structure minus the size of the 1-element tree array at > > the > > + end (see gimple_ops). An adjustment may be required if there is tail > > + padding, as may happen on a host (e.g. sparc) where a pointer has 4-byte > > + alignment while a uint64_t has 8-byte alignment. > > + > > + Unfortunately, we can't use offsetof to do this computation 100% > > + straightforwardly, because these structs use inheritance and so are not > > + standard layout types. However, the fact that they are not standard > > layout > > + types also means that tail padding will be reused in inheritance, which > > makes > > + it possible to check for the problematic case with the following logic > > + instead. If tail padding is detected, the offset should be decreased > > + accordingly. */ > > + > > +template<typename G> > > +static constexpr size_t > > +get_tail_padding_adjustment () > > +{ > > + struct padding_check : G > > + { > > + tree t; > > + }; > > + return sizeof (padding_check) == sizeof (G) ? sizeof (tree) : 0; > > +} > > + > > #define DEFGSSTRUCT(SYM, STRUCT, HAS_TREE_OP) \ > > - (HAS_TREE_OP ? sizeof (struct STRUCT) - sizeof (tree) : 0), > > + (HAS_TREE_OP \ > > + ? sizeof (STRUCT) - sizeof (tree) - get_tail_padding_adjustment<STRUCT> > > () \ > > + : 0), > > I wonder if we cannot simply use offsetof (STRUCT, ops) and some > "trick" to avoid > parsing/sanitizing this when HAS_TREE_OP is false? Maybe even a > > template <class T, bool has_offset> > constexpr size_t ops_offset () { return 0; } > > template <class T> > constexpr size_t ops_offset<T, true> () > { > return offsetof (T, ops); > /* or T x; return (char *)x.ops - (char *)x; */ > } > > ? That is I don't like the "indirect" adjustment via computing the padding. > > Richard.
Thanks, yes, I tried to start this way as well. The template works, or it's not hard to change gsstruct.def so that DEFGSSTRUCT(SYM, STRUCT, HAS_TREE_OP) is instead two different macros, one for HAS_TREE_OP==true and one for false. The issue I ran into is that: -AFAIK it's not OK to call offsetof() on a struct which is not standard layout. All the gimple types fail to be standard layout because they use inheritance. GCC provides a warning "use of offsetof is conditionally supported". We could do this anyway and suppress the warning, but I am not sure if it would be too limiting in that it may not build right with other compilers. -AFAIK the use of reinterpret_cast as in (char *)x.ops - (char *)x is not permitted in a constexpr function, so doing it that way would require the array to be initialized at runtime instead. GCC actually doesn't complain about it in the constexpr function, although I think it's not supposed to be allowed. Maybe it's not a big deal if the array becomes dynamically initialized, it would cause problems only if there are some global variables anywhere that may require it to be initialized. But I guess my thinking was that the tail padding approach is standard-conforming and works on the one platform I've found where it's necessary, and can't really do any harm to other platforms, so it's at least a strict improvement over what's there now... whereas changing it to use some implementation-defined behavior such as offsetof() on a non-POD type may have harder-to-predict implications. Happy to change it if you prefer one of the other options... -Lewis -Lewis