On Tue, Nov 19, 2024 at 4:37 PM Lewis Hyatt <lhy...@gmail.com> wrote:
>
> 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.
We need to support non-GCC when building stage1 so we need something
strictly conforming here.
> 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...
Unless some very C++ savy person comes around your patch is OK then.
It should work for the case the padding is exactly sizeof(tree) (in theory
it could be half a tree for 8 byte but 4 byte aligned pointers and some other
struct element forcing 8 byte alignment)
Richard.
>
> -Lewis
>
>
> -Lewis
