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
