[patch] gcc/testsuite/gcc.c-torture/compile/pr27528.c

[patch] gcc/testsuite/gcc.c-torture/compile/pr27528.c

[Hui-May Chang]

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

We, at Apple, found pr27528.c failed on x86 MacOS as asm_operand_ok  
routine performs the following test,

          case 'i':
           if (CONSTANT_P (op) && (! flag_pic ||  
LEGITIMATE_PIC_OPERAND_P (op)))
             result = 1;
           break;

We felt asm's aren't expected to figure out the necessary sequence for  
pic code and would like to propose adding -fno-pic option for the test.

The following patch has been tested on ppc and x86 MacOS.

gcc/testsuite/ChangeLog:
         * gcc.c-torture/compile/pr27528.c: Add -fno-pic option.



[-- Attachment #2: radar-patch.pr27528.txt --]
[-- Type: text/plain, Size: 350 bytes --]

Index: pr27528.c
===================================================================
--- pr27528.c	(revision 120526)
+++ pr27528.c	(working copy)
@@ -1,3 +1,4 @@
+/* { dg-options "-fno-pic" } */
 /* Check that constant constraints like "i", "n" and "s" can be used in
    cases where the operand is an initializer constant.  */
 int x[2] = { 1, 2 };

[-- Attachment #3: Type: text/plain, Size: 35 bytes --]


OK?

Hui-May Chang
Apple Computer

Re: [patch] gcc/testsuite/gcc.c-torture/compile/pr27528.c

[Andrew Pinski]

On Sat, 2007-01-06 at 13:56 -0800, Hui-May Chang wrote:
> 
> gcc/testsuite/ChangeLog:
>          * gcc.c-torture/compile/pr27528.c: Add -fno-pic option.


I don't think this is the correct fix.  The "i" constraint should accept
this, even if we are in PIC mode.

What the "i" constraint should do for these asms is just give them the
asm label and nothing more.  This is the reason behind this bug report
and testcase originally in fact.

Thanks,
Andrew Pinski

Re: [patch] gcc/testsuite/gcc.c-torture/compile/pr27528.c

[Mike Stump]

On Jan 6, 2007, at 3:12 PM, Andrew Pinski wrote:
> On Sat, 2007-01-06 at 13:56 -0800, Hui-May Chang wrote:
>>
>> gcc/testsuite/ChangeLog:
>>          * gcc.c-torture/compile/pr27528.c: Add -fno-pic option.
>
> I don't think this is the correct fix.  The "i" constraint should  
> accept
> this, even if we are in PIC mode.

Could you elaborate some?  Is it because it is really a legitimate  
pic operand?  If so, could you discuss just why it is.  How does one  
encode a LABEL_DECL in x86 land's addressing modes when the  
instruction doesn't take a pc-relative address but you want read only  
code for pic code?

> What the "i" constraint should do for these asms

For just these asm, or for all uses of the "i" constraint?

> is just give them the asm label and nothing more.

And how do you expect that to be encoded in an instruction that  
doesn't have pc-relative addressing for pic mode when the desire is  
to be able to not have dynamic reloc fixups for the code so that it  
can be read only and shared?

> This is the reason behind this bug report and testcase originally  
> in fact.

And what PR was that exactly?  PR27528?  I didn't see any mention of - 
fPIC anywhere in that PR.

Re: [patch] gcc/testsuite/gcc.c-torture/compile/pr27528.c

[Andrew Pinski]

On 1/8/07, Mike Stump <mrs@apple.com> wrote:
> On Jan 6, 2007, at 3:12 PM, Andrew Pinski wrote:
> > On Sat, 2007-01-06 at 13:56 -0800, Hui-May Chang wrote:
> >>
> >> gcc/testsuite/ChangeLog:
> >>          * gcc.c-torture/compile/pr27528.c: Add -fno-pic option.
> >
> > I don't think this is the correct fix.  The "i" constraint should
> > accept
> > this, even if we are in PIC mode.
>
> Could you elaborate some?  Is it because it is really a legitimate
> pic operand?  If so, could you discuss just why it is.  How does one
> encode a LABEL_DECL in x86 land's addressing modes when the
> instruction doesn't take a pc-relative address but you want read only
> code for pic code?

What I am trying to say is that:

int f(void)
{
 static int a;
  asm("#%0" : : "i" (&a));
}
Should always work.  "i" does not need a legitimate pic operand.  And
PIC is irrelevant here because of what the documentation says about
the "i" constraint.

Lets look at the documentation for the "i" constraint:
`i'
    An immediate integer operand (one with constant value) is allowed.
This includes symbolic constants whose values will be known only at
assembly time or later.


Since the label will always be known at assembly time,  it should not
be rejected.

> > What the "i" constraint should do for these asms
>
> For just these asm, or for all uses of the "i" constraint?

Look at the documentation that I reference above.  "i" should work
with any address of a global variables.


> > is just give them the asm label and nothing more.
>
> And how do you expect that to be encoded in an instruction that
> doesn't have pc-relative addressing for pic mode when the desire is
> to be able to not have dynamic reloc fixups for the code so that it
> can be read only and shared?


But that is not real question here.  Look at what the documentation
says, this is a constant at assemble time.

> And what PR was that exactly?  PR27528?  I didn't see any mention of -
> fPIC anywhere in that PR.

Right, but that is because it was referencing in general behavior of
"i", just the original bug showed up on powerpc-linux with the section
anchors.  PIC is just another case of the bug.

Re: [patch] gcc/testsuite/gcc.c-torture/compile/pr27528.c

[Mike Stump]

On Jan 6, 2007, at 1:56 PM, Hui-May Chang wrote:
> We, at Apple, found pr27528.c failed on x86 MacOS as asm_operand_ok  
> routine performs the following test,
>
>          case 'i':
>           if (CONSTANT_P (op) && (! flag_pic ||  
> LEGITIMATE_PIC_OPERAND_P (op)))
>             result = 1;
>           break;
>
> We felt asm's aren't expected to figure out the necessary sequence  
> for pic code and would like to propose adding -fno-pic option for  
> the test.
>
> The following patch has been tested on ppc and x86 MacOS.
>
> gcc/testsuite/ChangeLog:
>         * gcc.c-torture/compile/pr27528.c: Add -fno-pic option.

> OK?

Unless someone knows of a reason why that would be a bad change, I'm  
thinking of approving this patch later this week.  I think this  
version is better than a .x file a la testsuite/gcc.c-torture/execute/ 
960312-1.x because it covers all targets that might want to default  
to pic codegen.  I'd really like to see others chime in on this one.   
Ian?  Jan?

Re: [patch] gcc/testsuite/gcc.c-torture/compile/pr27528.c

[Andrew Pinski]

> 
> On Jan 6, 2007, at 1:56 PM, Hui-May Chang wrote:
> > We, at Apple, found pr27528.c failed on x86 MacOS as asm_operand_ok  
> > routine performs the following test,
> >
> >          case 'i':
> >           if (CONSTANT_P (op) && (! flag_pic ||  
> > LEGITIMATE_PIC_OPERAND_P (op)))
> >             result = 1;
> >           break;
> >
> > We felt asm's aren't expected to figure out the necessary sequence  
> > for pic code and would like to propose adding -fno-pic option for  
> > the test.
> >
> > The following patch has been tested on ppc and x86 MacOS.
> >
> > gcc/testsuite/ChangeLog:
> >         * gcc.c-torture/compile/pr27528.c: Add -fno-pic option.
> 
> > OK?
> 
> Unless someone knows of a reason why that would be a bad change, I'm  
> thinking of approving this patch later this week.  I think this  
> version is better than a .x file a la testsuite/gcc.c-torture/execute/ 
> 960312-1.x because it covers all targets that might want to default  
> to pic codegen.  I'd really like to see others chime in on this one.   
> Ian?  Jan?
> 

Re: [patch] gcc/testsuite/gcc.c-torture/compile/pr27528.c

[Ian Lance Taylor]

Mike Stump <mrs@apple.com> writes:

> On Jan 6, 2007, at 1:56 PM, Hui-May Chang wrote:
> > We, at Apple, found pr27528.c failed on x86 MacOS as asm_operand_ok
> > routine performs the following test,
> >
> >          case 'i':
> >           if (CONSTANT_P (op) && (! flag_pic ||
> > LEGITIMATE_PIC_OPERAND_P (op)))
> >             result = 1;
> >           break;
> >
> > We felt asm's aren't expected to figure out the necessary sequence
> > for pic code and would like to propose adding -fno-pic option for
> > the test.
> >
> > The following patch has been tested on ppc and x86 MacOS.
> >
> > gcc/testsuite/ChangeLog:
> >         * gcc.c-torture/compile/pr27528.c: Add -fno-pic option.
> 
> > OK?
> 
> Unless someone knows of a reason why that would be a bad change, I'm
> thinking of approving this patch later this week.  I think this
> version is better than a .x file a la testsuite/gcc.c-torture/execute/
> 960312-1.x because it covers all targets that might want to default
> to pic codegen.  I'd really like to see others chime in on this one.
> Ian?  Jan?

The original patch to the testsuite looks reasonable to me.  I agree
that in PIC mode the address of a global variable is not a constant,
so it does not make sense to require a "i" constraint to accept it.

But I can't approve the patch as I am not a testsuite maintainer.

Ian

Re: [patch] gcc/testsuite/gcc.c-torture/compile/pr27528.c

[Mike Stump]

On Jan 8, 2007, at 11:45 AM, Andrew Pinski wrote:
> Should always work.  "i" does not need a legitimate pic operand.

> And PIC is irrelevant here because of what the documentation says  
> about
> the "i" constraint.

Ah, ok, I see where you are coming from.  You think that the  
documentation is complete, accurate and fully describes this and the  
compiler should be changed to match it.  Now, one last question, why  
do you think the documentation is accurate and complete?

Anyway, I'd be happy to accept your position, if you submit a change  
to recog.c to match your world view and have it accepted.  Oh, and  
while you're at it, be sure to `fix' the g constraint to match as  
well, as it has the exact same `bug' in it, which is where rth copied  
the code from.  Oh, also, be sure to fix the documentation for  
LEGITIMATE_PIC_OPERAND_P as it would then be wrong as well.

My take, existing code follows existing behavior of the compiler and  
that existing code currently correctly handles "ri" on x86 by  
reloading things that can't be handled as an operand due to picness  
into a register.  If the recog change was put in, it would break this  
type of existing code.  I don't see the advantages of randomly  
breaking existing code.

Re: [patch] gcc/testsuite/gcc.c-torture/compile/pr27528.c

[Dale Johannesen]

On Jan 8, 2007, at 3:15 PM, Mike Stump wrote:

> On Jan 8, 2007, at 11:45 AM, Andrew Pinski wrote:
>> Should always work.  "i" does not need a legitimate pic operand.
>
>> And PIC is irrelevant here because of what the documentation says  
>> about
>> the "i" constraint.
>
> Ah, ok, I see where you are coming from.  You think that the  
> documentation is complete, accurate and fully describes this and  
> the compiler should be changed to match it.  Now, one last  
> question, why do you think the documentation is accurate and complete?

The history of the doc may be relevant.  For a long time it read:

> This includes symbolic constants whose values will be known only at
> assembly time.

which does not include labels, as they get relocated at link time, or  
later in the case of most pic code.  The "or later" was added here:
http://gcc.gnu.org/ml/gcc-patches/2004-03/msg00846.html
as a rider to a m68k-specific code-changing patch.  No justification  
is given for the doc change.   It's not obvious what the intent was,  
but I doubt  that trying to handle values that aren't known until  
runtime with "i" is a reasonable thing to do.

Re: [patch] gcc/testsuite/gcc.c-torture/compile/pr27528.c

[Mike Stump]

On Jan 6, 2007, at 1:56 PM, Hui-May Chang wrote:
> gcc/testsuite/ChangeLog:
>         * gcc.c-torture/compile/pr27528.c: Add -fno-pic option.

> OK?

Ok.