g++ pointer tracking question

g++ pointer tracking question

[Steve Ellcey]

I am looking at a problem here in our HP-UX IPF GCC compiler and I think
it is due to a bug in the shared code that could affect all platforms.
I am hoping someone could help me understand this and see if I am on the
right track.

Here is part of a C++ program:

  struct A {
     A(int arg) : ix(arg) {}
     int ix;
     int foo(int arg) { return arg + ix; }
  };
  int func(int A::*p1)
  {
     A obj(2);
     return ((&obj)->*p1);
  }

Now, when I look at the RTL generated on HP-UX IPF in 64 bit mode with
-O1 (should be the similar to Linux IPF and probably others) I see:

(insn 4 2 5 (set (reg/v/f:DI 341)
        (reg:DI 112 in0)) -1 (nil)
    (nil))

Now, I think the '/f' on register 341 means that frame_related is set
which is what is used to say that REG_POINTER is true.  I.e.  we are
claiming that 341 contains a pointer to memory.  In fact in0 and
register 341 actually just contain the offset of ix within the structure
A.  I think passing an offset (and not an address) as the argument is
the right thing to do but then we shouldn't be setting REG_POINTER to
TRUE, should we?  Can somone tell me if I passing an offset is in fact
right?  If so should REG_POINTER for reg 341 be set to TRUE?

I have no idea how to fix this but I would at least like to be sure
I understand the problem.  I can submit a GNAT if that would help and
I can continue to look into what to do but I am at a loss for how to
proceed right now.

Steve Ellcey
sje@cup.hp.com

Re: g++ pointer tracking question

[Jason Merrill]

>>>>> "Steve" == Steve Ellcey <sje@cup.hp.com> writes:

> Now, I think the '/f' on register 341 means that frame_related is set
> which is what is used to say that REG_POINTER is true.  I.e.  we are
> claiming that 341 contains a pointer to memory.  In fact in0 and
> register 341 actually just contain the offset of ix within the structure
> A.  I think passing an offset (and not an address) as the argument is
> the right thing to do but then we shouldn't be setting REG_POINTER to
> TRUE, should we?  Can somone tell me if I passing an offset is in fact
> right?  If so should REG_POINTER for reg 341 be set to TRUE?

Passing an offset is right.  REG_POINTER should not be true, but I wouldn't
expect it to break anything.  The problem is that the C++ frontend uses
POINTER_TYPE to describe pointer-to-member types; this is conceptually
wrong, but nobody has gotten around to fixing it yet.

I think your bug lies elsewhere.

Jason

Re: g++ pointer tracking question

[law]

In message <200205152056.NAA27988@hpsje.cup.hp.com>, Steve Ellcey writes:
 > I am looking at a problem here in our HP-UX IPF GCC compiler and I think
 > it is due to a bug in the shared code that could affect all platforms.
 > I am hoping someone could help me understand this and see if I am on the
 > right track.
 > 
 > Here is part of a C++ program:
 > 
 >   struct A {
 >      A(int arg) : ix(arg) {}
 >      int ix;
 >      int foo(int arg) { return arg + ix; }
 >   };
 >   int func(int A::*p1)
 >   {
 >      A obj(2);
 >      return ((&obj)->*p1);
 >   }
 > 
 > Now, when I look at the RTL generated on HP-UX IPF in 64 bit mode with
 > -O1 (should be the similar to Linux IPF and probably others) I see:
 > 
 > (insn 4 2 5 (set (reg/v/f:DI 341)
 >         (reg:DI 112 in0)) -1 (nil)
 >     (nil))
 > 
 > Now, I think the '/f' on register 341 means that frame_related is set
 > which is what is used to say that REG_POINTER is true.  I.e.  we are
 > claiming that 341 contains a pointer to memory.  In fact in0 and
 > register 341 actually just contain the offset of ix within the structure
 > A.  I think passing an offset (and not an address) as the argument is
 > the right thing to do but then we shouldn't be setting REG_POINTER to
 > TRUE, should we?  Can somone tell me if I passing an offset is in fact
 > right?  If so should REG_POINTER for reg 341 be set to TRUE?
 > 
 > I have no idea how to fix this but I would at least like to be sure
 > I understand the problem.  I can submit a GNAT if that would help and
 > I can continue to look into what to do but I am at a loss for how to
 > proceed right now.
I would agree -- setting REG_POINTER on an offset within a structure
is a bad thing to do.  In fact, it can result in incorrect code on
the PA (we use REG_POINTER to distinguish between things like base
and unscaled index when generating ldbx instructions -- getting them
reversed can result in a segfault due to the way implicit space
register selection works on the PA).

jeff

Re: g++ pointer tracking question

[Jason Merrill]

>>>>> "Steve" == Steve Ellcey <sje@cup.hp.com> writes:

> While the change you suggested does fix my problem it looks like it
> breaks other things.

Yep, there are bits in the C++ frontend that expect POINTER_TYPE_P to be
true for pointer to member types.  Seems that the invasive change (dropping
the POINTER_TYPE and just using OFFSET_TYPE) is the right way to go.

Jason

Re: g++ pointer tracking question

[Steve Ellcey]

Jason,

While the change you suggested does fix my problem it looks like it
breaks other things.  I kicked off a C & C++ test suite run and got a
few new C++ failures like g++.jason/pmem3.C where g++ is now saying:

pmem3.C: In function `int main()':
pmem3.C:9: cannot convert `apm' from type `int A::*' to type `int B::*'
pmem3.C:10: cannot convert `apm' from type `int A::*' to type `int B::*'

I never got this before.

Steve Ellcey
sje@cup.hp.com

Re: g++ pointer tracking question

[Steve Ellcey]

> Does this fix the problem?

Yes, my code correctly compiles with this change.  On IA64 HP-UX, in
ILP32 mode, it does not do an optimization that I want because the
pointer that the offset is being added to is not marked as a pointer but
that may be a bug in my own IA64 specific code or it could be a bug in
the more generic POINTERS_EXTEND_UNSIGNED code, I am not sure.  When I
generate LP64 code everything seems marked correctly as to whether it is
a pointer or not.  At any rate, while I miss an opportunity for a
platform specific optimization, I do not generate any bad code, so I
like the change and think it is better then my local change which was to
just turn off the optimization entirely when compiling C++.  This will
only turn off the optimization in certain cases where it is known that
it cannot do it safely.

Steve Ellcey
sje@cup.hp.com

Re: g++ pointer tracking question

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 28 bytes --]

Does this fix the problem?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1997 bytes --]

*** calls.c.~1~	Fri May 10 14:22:49 2002
--- calls.c	Sat May 18 16:07:09 2002
*************** expand_call (exp, target, ignore)
*** 3086,3092 ****
  	      rtx temp = gen_reg_rtx (GET_MODE (valreg));
  
  	      /* Mark the return value as a pointer if needed.  */
! 	      if (TREE_CODE (TREE_TYPE (exp)) == POINTER_TYPE)
  		mark_reg_pointer (temp,
  				  TYPE_ALIGN (TREE_TYPE (TREE_TYPE (exp))));
  
--- 3086,3092 ----
  	      rtx temp = gen_reg_rtx (GET_MODE (valreg));
  
  	      /* Mark the return value as a pointer if needed.  */
! 	      if (POINTER_TYPE_P (TREE_TYPE (exp)))
  		mark_reg_pointer (temp,
  				  TYPE_ALIGN (TREE_TYPE (TREE_TYPE (exp))));
  
*************** expand_call (exp, target, ignore)
*** 3118,3124 ****
  	  rtx last, insns;
  
  	  /* The return value from a malloc-like function is a pointer.  */
! 	  if (TREE_CODE (TREE_TYPE (exp)) == POINTER_TYPE)
  	    mark_reg_pointer (temp, BIGGEST_ALIGNMENT);
  
  	  emit_move_insn (temp, valreg);
--- 3118,3124 ----
  	  rtx last, insns;
  
  	  /* The return value from a malloc-like function is a pointer.  */
! 	  if (POINTER_TYPE_P (TREE_TYPE (exp)))
  	    mark_reg_pointer (temp, BIGGEST_ALIGNMENT);
  
  	  emit_move_insn (temp, valreg);
*** tree.h.~1~	Thu May 16 05:51:52 2002
--- tree.h	Sat May 18 16:06:47 2002
*************** extern void tree_class_check_failed PARA
*** 418,424 ****
     reference type.  (It should be renamed to INDIRECT_TYPE_P.)  */
  
  #define POINTER_TYPE_P(TYPE) \
!   (TREE_CODE (TYPE) == POINTER_TYPE || TREE_CODE (TYPE) == REFERENCE_TYPE)
  
  /* Nonzero if TYPE represents a bounded pointer or bounded reference type.  */
  
--- 418,426 ----
     reference type.  (It should be renamed to INDIRECT_TYPE_P.)  */
  
  #define POINTER_TYPE_P(TYPE) \
!   ((TREE_CODE (TYPE) == POINTER_TYPE \
!     && TREE_CODE (TREE_TYPE (TYPE)) != OFFSET_TYPE) \
!    || TREE_CODE (TYPE) == REFERENCE_TYPE)
  
  /* Nonzero if TYPE represents a bounded pointer or bounded reference type.  */
  

Re: g++ pointer tracking question

[Steve Ellcey]

Well, I thought I could do a simple workaround for IA64 but it is worse
then I thought.  I believed that the two values being added together
were both being marked as REG_POINTER but in fact the offset is being
marked as a REG_POINTER and the real pointer is not marked as a
REG_POINTER so I can't just bail out on the optimization when
REG_POINTER is set for both values being added together.  I don't know
how to fix this so I have filed it in GNAT (c++/6685) and will turn off
the machine level optimization for C++ on HP-UX IA64 that is going wrong
when this information is wrong.

Steve Ellcey
sje@cup.hp.com

Re: g++ pointer tracking question

[John David Anglin]

> Passing an offset is right.  REG_POINTER should not be true, but I wouldn't
> expect it to break anything.  The problem is that the C++ frontend uses

On the PA, it could result in the wrong register being selected as
the basereg for an index insn.  The basereg is used to select the
space register for the operation.  If this is selected incorrectly,
a segmentation fault could occur or incorrect data could be loaded.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

Re: g++ pointer tracking question

[Steve Ellcey]

> Passing an offset is right.  REG_POINTER should not be true, but I wouldn't
> expect it to break anything.  The problem is that the C++ frontend uses
> POINTER_TYPE to describe pointer-to-member types; this is conceptually
> wrong, but nobody has gotten around to fixing it yet.
> 
> I think your bug lies elsewhere.

No, I think this is it.  While this works OK in 64 bit mode and may not
cause problems for other platforms, in 32 bit mode (HP-UX IA64) we
really need to know if something is a pointer or not for one of our
(platform specific) optimizations.

Details for the curious:

IA64 extends 32 pointers to 64 bits using an instruction known as
"addp4", which can take two arguments, a 32 bit pointer and an offset
(constant integer or another register), and it gives you a result by
adding the two together and then copying bits 30 and 31 of the pointer
argument to bits 61 and 62.  The bits copied are only from the register
that is supposed to contain a pointer, not the register that contains an
offset.

If you do it in two steps; add the two numbers, then addp4 the result it
doesn't matter which value is an address and which is an offset.  You
just add them together and then do an addp4 with a constant offset of 0.
If you want to combine the two steps into a single instruction it
matters very much that the register you think has a pointer in it really
does.

So my problem is I have two 32 bit numbers, both show up as pointers and
I just happen to luck out and pick the wrong one when I try to use one
as the pointer register in my addp4 instruction.

Maybe I can modify ia64.md to say that the addp4_plus_[12] instructions
that combine an add and an addp4 into one instruction cannot be used if
both arguments show up as pointers.  I don't think I am up to fixing the
C++ front-end.

Steve Ellcey
sje@cup.hp.com