[PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Kewen.Lin]

Hi,

Test case bswap64-4.c suffers the issue as its comments:

/* On some versions of dejagnu this test will fail when
   biarch testing with RUNTESTFLAGS="--target_board=unix
   '{-m64,-m32}'" due to -m32 being added on the command
   line after the dg-options -mpowerpc64.
   common/config/rs6000/rs6000-common.c:
   rs6000_handle_option disables -mpowerpc64 for -m32.  */

As tested, on test machine with dejaGnu 1.6.2, the compilation
option order looks like: -m32 ... -mpowerpc64, option
-mpowerpc64 still takes effect;  While on test machine with
dejaGnu 1.5.1, the option order looks like: -mpowerpc64 ... -m32,
option -mpowerpc64 is disabled by -m32, then the case fails.

This fix leverages the new effective target has_arch_ppc64 and
places dg-options before dg-require-effective-target (it makes
dg-options to be used for has_arch_ppc64 checking), on machine
with dejaGnu 1.6.2, the checking succeeds and the case passes;
while on machine with dejaGnu 1.5.1, the checking fails then
the case is marked as unsupported.

Tested on powerpc64-linux-gnu P7 and P8,
and powerpc64le-linux-gnu P9 and P10.

I'll push this soon if no objections.

BR,
Kewen
-----
	PR testsuite/106680

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/bswap64-4.c: Adjust with has_arch_ppc64.
---
 gcc/testsuite/gcc.target/powerpc/bswap64-4.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/testsuite/gcc.target/powerpc/bswap64-4.c b/gcc/testsuite/gcc.target/powerpc/bswap64-4.c
index 5acbb91ee38..85a7bbbd367 100644
--- a/gcc/testsuite/gcc.target/powerpc/bswap64-4.c
+++ b/gcc/testsuite/gcc.target/powerpc/bswap64-4.c
@@ -1,7 +1,8 @@
 /* { dg-do compile { target { powerpc*-*-* } } } */
 /* { dg-skip-if "" { powerpc*-*-aix* } } */
-/* { dg-options "-O2 -mpowerpc64" } */
 /* { dg-require-effective-target ilp32 } */
+/* { dg-options "-O2 -mpowerpc64" } */
+/* { dg-require-effective-target has_arch_ppc64 } */
 /* { dg-final { scan-assembler-times "lwbrx" 2 { target { ! has_arch_pwr7 } } } } */
 /* { dg-final { scan-assembler-times "stwbrx" 2 { target { ! has_arch_pwr7 } } } } */
 /* { dg-final { scan-assembler-times "ldbrx" 1 { target has_arch_pwr7 } } } */
--
2.27.0

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Peter Bergner]

On 8/31/22 4:33 AM, Kewen.Lin wrote:
> @@ -1,7 +1,8 @@
>  /* { dg-do compile { target { powerpc*-*-* } } } */
>  /* { dg-skip-if "" { powerpc*-*-aix* } } */
> -/* { dg-options "-O2 -mpowerpc64" } */
>  /* { dg-require-effective-target ilp32 } */
> +/* { dg-options "-O2 -mpowerpc64" } */
> +/* { dg-require-effective-target has_arch_ppc64 } */

With many of our recent patches moving the dg-options before any
dg-requires-effectice-target so it affects the results of the
dg-requires-effectice-target test, this looks like it's backwards
from that process.  I understand why, so I think an explicit comment
here in the test case explaining why it's after in this case.
Just so in a few years when we come back to this test case, we
won't accidentally undo this change.

The other option would be to introduce -mdejagnu-32 and -mdejagnu-64
options that operate like our -mdejagnu-cpu= options and override any
-m32/-m64 option given on the command line (eg, RUNTESTFLAGS=...).
To be honest, I'm not sure how I feel about this suggestion! :-)
Maybe we'd only use it on the test cases like this one that we know
are sensitive to be overridden???

Peter

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Kewen.Lin]

on 2022/8/31 22:13, Peter Bergner wrote:
> On 8/31/22 4:33 AM, Kewen.Lin wrote:
>> @@ -1,7 +1,8 @@
>>  /* { dg-do compile { target { powerpc*-*-* } } } */
>>  /* { dg-skip-if "" { powerpc*-*-aix* } } */
>> -/* { dg-options "-O2 -mpowerpc64" } */
>>  /* { dg-require-effective-target ilp32 } */
>> +/* { dg-options "-O2 -mpowerpc64" } */
>> +/* { dg-require-effective-target has_arch_ppc64 } */
> 
> With many of our recent patches moving the dg-options before any
> dg-requires-effectice-target so it affects the results of the
> dg-requires-effectice-target test, this looks like it's backwards
> from that process.  I understand why, so I think an explicit comment
> here in the test case explaining why it's after in this case.
> Just so in a few years when we come back to this test case, we
> won't accidentally undo this change.

Oops, the diff shows it's like "after", but it's actually still "before". :)
The dg-options is meant to be placed before the succeeding has_arch_ppc64
effective target which is supposed to use dg-options to compile.  I felt
good to let ilp32 checking go first then has_arch_ppc64, so moved dg-option
downward.

Sorry for the confusion, I should have placed the has_arch_ppc64
effective target just after the dg-options.  Anyway, it's a good idea
to add more comments in test case source!  Thanks!

BR,
Kewen

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Segher Boessenkool]

On Thu, Sep 01, 2022 at 04:57:59PM +0800, Kewen.Lin wrote:
> on 2022/8/31 22:13, Peter Bergner wrote:
> > On 8/31/22 4:33 AM, Kewen.Lin wrote:
> >> @@ -1,7 +1,8 @@
> >>  /* { dg-do compile { target { powerpc*-*-* } } } */
> >>  /* { dg-skip-if "" { powerpc*-*-aix* } } */
> >> -/* { dg-options "-O2 -mpowerpc64" } */
> >>  /* { dg-require-effective-target ilp32 } */
> >> +/* { dg-options "-O2 -mpowerpc64" } */
> >> +/* { dg-require-effective-target has_arch_ppc64 } */
> > 
> > With many of our recent patches moving the dg-options before any
> > dg-requires-effectice-target so it affects the results of the
> > dg-requires-effectice-target test, this looks like it's backwards
> > from that process.  I understand why, so I think an explicit comment
> > here in the test case explaining why it's after in this case.
> > Just so in a few years when we come back to this test case, we
> > won't accidentally undo this change.
> 
> Oops, the diff shows it's like "after", but it's actually still "before". :)
> The dg-options is meant to be placed before the succeeding has_arch_ppc64
> effective target which is supposed to use dg-options to compile.  I felt
> good to let ilp32 checking go first then has_arch_ppc64, so moved dg-option
> downward.

These two are independent, but apparently we have a bug here, which will
make what you did malfunction in some cases -- the test will not run for
ilp32 if you have RUNTESTFLAGS {-m32,-m64}.

It should not make a difference, -mpowerpc64 and -m32 should be wholly
independent, and their order should not matter.  So the order of the
  /* { dg-require-effective-target ilp32 } */
  /* { dg-options "-O2 -mpowerpc64" } */
lines should not make a difference either.  But it does :-(


Segher

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Kewen.Lin]

Hi Segher,

on 2022/9/1 22:57, Segher Boessenkool wrote:
> On Thu, Sep 01, 2022 at 04:57:59PM +0800, Kewen.Lin wrote:
>> on 2022/8/31 22:13, Peter Bergner wrote:
>>> On 8/31/22 4:33 AM, Kewen.Lin wrote:
>>>> @@ -1,7 +1,8 @@
>>>>  /* { dg-do compile { target { powerpc*-*-* } } } */
>>>>  /* { dg-skip-if "" { powerpc*-*-aix* } } */
>>>> -/* { dg-options "-O2 -mpowerpc64" } */
>>>>  /* { dg-require-effective-target ilp32 } */
>>>> +/* { dg-options "-O2 -mpowerpc64" } */
>>>> +/* { dg-require-effective-target has_arch_ppc64 } */
>>>
>>> With many of our recent patches moving the dg-options before any
>>> dg-requires-effectice-target so it affects the results of the
>>> dg-requires-effectice-target test, this looks like it's backwards
>>> from that process.  I understand why, so I think an explicit comment
>>> here in the test case explaining why it's after in this case.
>>> Just so in a few years when we come back to this test case, we
>>> won't accidentally undo this change.
>>
>> Oops, the diff shows it's like "after", but it's actually still "before". :)
>> The dg-options is meant to be placed before the succeeding has_arch_ppc64
>> effective target which is supposed to use dg-options to compile.  I felt
>> good to let ilp32 checking go first then has_arch_ppc64, so moved dg-option
>> downward.
> 
> These two are independent, but apparently we have a bug here, which will
> make what you did malfunction in some cases -- the test will not run for
> ilp32 if you have RUNTESTFLAGS {-m32,-m64}.

Yeah, because of the bug (or call it surprised behavior), the test case can
fail for some dejaGnu version like 1.5.1 (how it places the dg-options matters).
What I proposed is to detect this kind of test environment by has_arch_ppc64,
then turn the failure into unsupported.  Then the test case can survive for
any dejaGnu versions.  But based on the discussions, I'd like to try to fix
the bug and abandon this testing fix first.

> 
> It should not make a difference, -mpowerpc64 and -m32 should be wholly
> independent, and their order should not matter.  So the order of the
>   /* { dg-require-effective-target ilp32 } */
>   /* { dg-options "-O2 -mpowerpc64" } */
> lines should not make a difference either.  But it does :-(
> 

I agree the point that the order of lines should not make a difference.  :)
But to be clarified, the order of 

  /* { dg-options "-O2 -mpowerpc64" } */

and 
  
  /* { dg-require-effective-target has_arch_ppc64 } */

matters in this proposed fix, not for the line with ilp32.

has_arch_ppc64 uses current_compiler_flags which only incorporates dg-options
which is placed before the dg-require-effective-target.  I guess it's related
to how dejaGnu parses lines and sets global variables, for this kind of case,
we have to put the expected order for now.

BR,
Kewen

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Segher Boessenkool]

On Fri, Sep 02, 2022 at 08:50:52AM +0800, Kewen.Lin wrote:
> on 2022/9/1 22:57, Segher Boessenkool wrote:
> > These two are independent, but apparently we have a bug here, which will
> > make what you did malfunction in some cases -- the test will not run for
> > ilp32 if you have RUNTESTFLAGS {-m32,-m64}.
> 
> Yeah, because of the bug (or call it surprised behavior),

No, I call it a bug.  Because that is what it is!

> the test case can
> fail for some dejaGnu version like 1.5.1 (how it places the dg-options matters).

Yes, but that is only one way to expose the problem.

The bug just should be fixed.

> But to be clarified, the order of 
> 
>   /* { dg-options "-O2 -mpowerpc64" } */
> 
> and 
>   
>   /* { dg-require-effective-target has_arch_ppc64 } */
> 
> matters in this proposed fix, not for the line with ilp32.

Of course :-)

> has_arch_ppc64 uses current_compiler_flags which only incorporates dg-options
> which is placed before the dg-require-effective-target.  I guess it's related
> to how dejaGnu parses lines and sets global variables, for this kind of case,
> we have to put the expected order for now.

Even just to avoid having to uselessly edit hundreds of testcases, it
would be better to just fix the bug!


Segher

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Kewen.Lin]

on 2022/9/3 01:36, Segher Boessenkool wrote:
> On Fri, Sep 02, 2022 at 08:50:52AM +0800, Kewen.Lin wrote:
>> on 2022/9/1 22:57, Segher Boessenkool wrote:
>>> These two are independent, but apparently we have a bug here, which will
>>> make what you did malfunction in some cases -- the test will not run for
>>> ilp32 if you have RUNTESTFLAGS {-m32,-m64}.
>>
>> Yeah, because of the bug (or call it surprised behavior),
> 
> No, I call it a bug.  Because that is what it is!
> 

OK. :)

>> the test case can
>> fail for some dejaGnu version like 1.5.1 (how it places the dg-options matters).
> 
> Yes, but that is only one way to expose the problem.
> 
> The bug just should be fixed.

Agreed.

> 
>> But to be clarified, the order of 
>>
>>   /* { dg-options "-O2 -mpowerpc64" } */
>>
>> and 
>>   
>>   /* { dg-require-effective-target has_arch_ppc64 } */
>>
>> matters in this proposed fix, not for the line with ilp32.
> 
> Of course :-)
> 
>> has_arch_ppc64 uses current_compiler_flags which only incorporates dg-options
>> which is placed before the dg-require-effective-target.  I guess it's related
>> to how dejaGnu parses lines and sets global variables, for this kind of case,
>> we have to put the expected order for now.
> 
> Even just to avoid having to uselessly edit hundreds of testcases, it
> would be better to just fix the bug!

I think "the bug" here means the "-mpower64" with "-m32/-m64" thing, not the 
dejaGnu thing mentioned above, then yes.

BR,
Kewen

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Segher Boessenkool]

On Wed, Aug 31, 2022 at 05:33:28PM +0800, Kewen.Lin wrote:
> Test case bswap64-4.c suffers the issue as its comments:
> 
> /* On some versions of dejagnu this test will fail when
>    biarch testing with RUNTESTFLAGS="--target_board=unix
>    '{-m64,-m32}'" due to -m32 being added on the command
>    line after the dg-options -mpowerpc64.
>    common/config/rs6000/rs6000-common.c:
>    rs6000_handle_option disables -mpowerpc64 for -m32.  */
> 
> As tested, on test machine with dejaGnu 1.6.2, the compilation
> option order looks like: -m32 ... -mpowerpc64, option
> -mpowerpc64 still takes effect;  While on test machine with
> dejaGnu 1.5.1, the option order looks like: -mpowerpc64 ... -m32,
> option -mpowerpc64 is disabled by -m32, then the case fails.

*Should* -mpowerpc64  be disabled by -m32?  Should *any* explicit
command line flag ever be disabled like that?  (Not talking about things
like -m32 -m64, this should be supported for convenience).

-mpowerpc64 -m32 should always mean the same as -m32 -mpowerpc64, that's
the principle of least surprise.

Where then dg-options is placed does not matter in this testcase, both
-m32 -mno-powerpc64 and -m32 -mpowerpc64 are ilp32.


Segher

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Peter Bergner]

On 8/31/22 10:24 AM, Segher Boessenkool wrote:
> Should *any* explicit command line flag ever be disabled like that?
> (Not talking about things like -m32 -m64, ...

In a general sense, I'd agree that the answer is no, but we do have
dependent options like -maltivec and -mvsx, etc., so a -mno-altivec
better disable any explicit -mvsx earlier on the command line.
Ditto for -msoft-float better disable any -maltivec and -mvsx, etc.
It's complicated...and that's a bad thing. :-(



> -mpowerpc64 -m32 should always mean the same as -m32 -mpowerpc64, that's
> the principle of least surprise.

I think I agree with this, since -mpowerpc64 doesn't mean or imply -m64.
I say "think", because I don't remember the history of why it behaves this
way and maybe there was a reason we did like this?  If there isn't a reason,
then I'm all for -m32 not overriding -mpowerpc64.

Peter

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Segher Boessenkool]

On Wed, Aug 31, 2022 at 10:48:26AM -0500, Peter Bergner wrote:
> On 8/31/22 10:24 AM, Segher Boessenkool wrote:
> > Should *any* explicit command line flag ever be disabled like that?
> > (Not talking about things like -m32 -m64, ...
> 
> In a general sense, I'd agree that the answer is no, but we do have
> dependent options like -maltivec and -mvsx, etc., so a -mno-altivec
> better disable any explicit -mvsx earlier on the command line.

Yes, but those two flags are tightly related.  -m32/-m64 and -mpowerpc64
are not, code model and instruction set selection are orthogonal.

> Ditto for -msoft-float better disable any -maltivec and -mvsx, etc.

Oh?  Why should it disable -maltivec?  -mvsx makes a little sense on
one hand, but totally none on the other either.

> It's complicated...and that's a bad thing. :-(

Yes.  And most of it is caused by us, instead of being a fact of life.

> > -mpowerpc64 -m32 should always mean the same as -m32 -mpowerpc64, that's
> > the principle of least surprise.
> 
> I think I agree with this, since -mpowerpc64 doesn't mean or imply -m64.
> I say "think", because I don't remember the history of why it behaves this
> way and maybe there was a reason we did like this?  If there isn't a reason,
> then I'm all for -m32 not overriding -mpowerpc64.

As far as I always knew it does *not* override it, so this seems like
an accident to me, not detected before  because everyone always types
-m32 -mpowerpc64 (I know I do, anyway).

I think we should just fix this and see what breaks, if anything?


Segher

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Peter Bergner]

On 8/31/22 11:05 AM, Segher Boessenkool wrote:
> On Wed, Aug 31, 2022 at 10:48:26AM -0500, Peter Bergner wrote:
>> Ditto for -msoft-float better disable any -maltivec and -mvsx, etc.
> 
> Oh?  Why should it disable -maltivec?  -mvsx makes a little sense on
> one hand, but totally none on the other either.

VSX has to be disabled, since VSX replies on the FP registers existing.

As for Altivec, I'm pretty sure there are some inherent dependencies
there, probably both in hardware and our GCC backend implementation.
I could be wrong, but my guess is things will fall over the ground
if as allow -maltivec along with -msoft-float.  Does the linux kernel
only build with -msoft-float assuming it disables altivec and vsx?
Or does it explicitly always also add -mno-altivec?



> As far as I always knew it does *not* override it, so this seems like
> an accident to me, not detected before  because everyone always types
> -m32 -mpowerpc64 (I know I do, anyway).
> 
> I think we should just fix this and see what breaks, if anything?

So in linux*.h, we have the following which came from a 2004 commit from Alan:

linux64.h:#define OS_MISSING_POWERPC64 !TARGET_64BIT

...and in rs6000.cc:rs6000_option_override_internal(), the following hunk is
basically from:

+2003-12-18  Geoffrey Keating  <geoffk@apple.com>
+
+       * config/rs6000/aix.h (OS_MISSING_POWERPC64): Define.
+       (OS_MISSING_ALTIVEC): Define.
+       * config/rs6000/darwin.h (ASM_SPEC): Be generous about supplying
+       -force_cpusubtype_ALL.
+       * config/rs6000/rs6000.c (rs6000_override_options): Rearrange
+       CPU information table; now always set all CPU-specific values.
+       Also, use Altivec and powerpc64 when chip and OS supports them.

  /* Some OSs don't support saving the high part of 64-bit registers on context
     switch.  Other OSs don't support saving Altivec registers.  On those OSs,
     we don't touch the OPTION_MASK_POWERPC64 or OPTION_MASK_ALTIVEC settings;
     if the user wants either, the user must explicitly specify them and we
     won't interfere with the user's specification.  */

  set_masks = POWERPC_MASKS;
#ifdef OS_MISSING_POWERPC64
  if (OS_MISSING_POWERPC64)
    set_masks &= ~OPTION_MASK_POWERPC64;
#endif
#ifdef OS_MISSING_ALTIVEC
  if (OS_MISSING_ALTIVEC)
    set_masks &= ~(OPTION_MASK_ALTIVEC | OPTION_MASK_VSX
                   | OTHER_VSX_VECTOR_MASKS);
#endif


...so I think there was no real reason, other than old 64-bit linux kernels didn't
save the upper register state in 32-bit mode binaries.  I believe that was "fixed"
a long time ago, so I agree we should just "fix" it and see what happens.
In this case, I think the fix is probably just to change the linux64.h define
to be "0" rather than "!TARGET_64".

Peter

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Segher Boessenkool]

On Wed, Aug 31, 2022 at 12:00:14PM -0500, Peter Bergner wrote:
> On 8/31/22 11:05 AM, Segher Boessenkool wrote:
> > On Wed, Aug 31, 2022 at 10:48:26AM -0500, Peter Bergner wrote:
> >> Ditto for -msoft-float better disable any -maltivec and -mvsx, etc.
> > 
> > Oh?  Why should it disable -maltivec?  -mvsx makes a little sense on
> > one hand, but totally none on the other either.
> 
> VSX has to be disabled, since VSX replies on the FP registers existing.

It doesn't?  On a CPU supporting VSX the FP registers are overlaid on
the VSX registers, not the other way around.

GCC says
  cc1: warning: '-mvsx' requires hardware floating point
and that's okay with me of course, but it doesn't say why it is
required.  Implementation convenience coupled with lack of a use case
is my best guess :-)

OTOH VMX and hard float are completely orthogonal (the VMX FP things do
not even use the fpscr for example).

> As for Altivec, I'm pretty sure there are some inherent dependencies
> there, probably both in hardware and our GCC backend implementation.
> I could be wrong, but my guess is things will fall over the ground
> if as allow -maltivec along with -msoft-float.  Does the linux kernel
> only build with -msoft-float assuming it disables altivec and vsx?
> Or does it explicitly always also add -mno-altivec?

No.  Instead, it just works!

Try this:
===
typedef float vf __attribute__((vector_size(16)));
vf f(float x)
{
	x *= 42;
	return (vf){x, x, x, x};
}
===
with -maltivec -msoft-float.  It does not use the FPRs, and it does use
the VMX registers and VMX instructions.

> So in linux*.h, we have the following which came from a 2004 commit from Alan:
> 
> linux64.h:#define OS_MISSING_POWERPC64 !TARGET_64BIT

That macro returns 1 on OSes that do not properly support -mpowerpc64.

> ...so I think there was no real reason, other than old 64-bit linux kernels didn't
> save the upper register state in 32-bit mode binaries.

And it is still not saved in 32-bit user mode (setjmp/longjmp and
getcontext/setcontext).  Most bigger programs will fail, and most
smaller programs (including everything in the GCC testsuite) work fine.
But we should not enable -mpowerpc64 on 32-bit Linux by default.


Segher

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Peter Bergner]

On 8/31/22 2:28 PM, Segher Boessenkool wrote:
> On Wed, Aug 31, 2022 at 12:00:14PM -0500, Peter Bergner wrote:
> No.  Instead, it just works!
> 
> Try this:
> ===
> typedef float vf __attribute__((vector_size(16)));
> vf f(float x)
> {
> 	x *= 42;
> 	return (vf){x, x, x, x};
> }
> ===
> with -maltivec -msoft-float.  It does not use the FPRs, and it does use
> the VMX registers and VMX instructions.

Well color me surprised!



>> linux64.h:#define OS_MISSING_POWERPC64 !TARGET_64BIT
> 
> That macro returns 1 on OSes that do not properly support -mpowerpc64.

Right, but haven't the 64-bit Linux kernels been fixed forever to always
save/restore the full 64-bit hardware registers on a context switch/signal?
If not, them this whole thing is moot and the current behavior of disabling
-mpower64 if we use -m32 later on the command line is the correct thing to do.


> But we should not enable -mpowerpc64 on 32-bit Linux by default.

I didn't imply we should do that.  I was only agreeing with you that
we should try not disabling an explicit -mpowerpc64 when -m32 is used
later on the command line.

I only meant to say is that the code in rs6000_option_override_internal() is what
seems to remove the OPTION_MASK_POWERPC64 from our cpu mask when -m32 is used
later on the command line... and that is controlled by OS_MISSING_POWERPC64.
Changing OS_MISSING_POWERPC64 as I mentioned would not add OPTION_MASK_POWERPC64
to our cpu masks when -m32 is used.


Peter

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Segher Boessenkool]

On Wed, Aug 31, 2022 at 02:53:07PM -0500, Peter Bergner wrote:
> On 8/31/22 2:28 PM, Segher Boessenkool wrote:
> > On Wed, Aug 31, 2022 at 12:00:14PM -0500, Peter Bergner wrote:
> Right, but haven't the 64-bit Linux kernels been fixed forever to always
> save/restore the full 64-bit hardware registers on a context switch/signal?

The kernel has.  But there are user space things (glibc) that haven't
been fixed, and those are default as well.

> If not, them this whole thing is moot and the current behavior of disabling
> -mpower64 if we use -m32 later on the command line is the correct thing to do.

The kernel has no way of disabling -mpowerpc64.  The instructions are
valid on any CPU that supports them, whether SF=1 or SF=0.

> > But we should not enable -mpowerpc64 on 32-bit Linux by default.
> 
> I didn't imply we should do that.  I was only agreeing with you that
> we should try not disabling an explicit -mpowerpc64 when -m32 is used
> later on the command line.

Okay, glad we agree :-)

> I only meant to say is that the code in rs6000_option_override_internal() is what
> seems to remove the OPTION_MASK_POWERPC64 from our cpu mask when -m32 is used
> later on the command line... and that is controlled by OS_MISSING_POWERPC64.

This is the default mask only here.

> Changing OS_MISSING_POWERPC64 as I mentioned would not add OPTION_MASK_POWERPC64
> to our cpu masks when -m32 is used.

So you say this is where the bug is?


Segher

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Peter Bergner]

On 8/31/22 4:07 PM, Segher Boessenkool wrote:
> On Wed, Aug 31, 2022 at 02:53:07PM -0500, Peter Bergner wrote:
>> Changing OS_MISSING_POWERPC64 as I mentioned would not add OPTION_MASK_POWERPC64
>> to our cpu masks when -m32 is used.
> 
> So you say this is where the bug is?

For linux64.h which is what I think the powerpc64-linux build will use,
we have:

linux64.h:#define OS_MISSING_POWERPC64 !TARGET_64BIT

Doing the macro expansion by hand into:

  set_masks = POWERPC_MASKS;
#ifdef OS_MISSING_POWERPC64
  if (OS_MISSING_POWERPC64)
    set_masks &= ~OPTION_MASK_POWERPC64;
#endif


...gives us:

  set_masks = POWERPC_MASKS;
  if (!TARGET_64BIT)
    set_masks &= ~OPTION_MASK_POWERPC64;

So if we handled a -mpowerpc64 earlier on the command line and added
OPTION_MASK_POWERPC64 to our cpu mask, then a following -m32 use will
remove it here.

So I mentioned doing:

linux64.h:
- #define OS_MISSING_POWERPC64 !TARGET_64BIT
+ #define OS_MISSING_POWERPC64 0

...which disables the above code only for powerpc64-linux builds and doesn't
affect AIX, Darwin, BSD, etc. or a powerpc-linux build.


> The kernel has.  But there are user space things (glibc) that haven't
> been fixed, and those are default as well.

Sure, but someone who is using -m32 -mpowerpc64 should know that and
relying on a 32-bit glibc to save/restore the full 64-bit registers
is a user error in my book.  If you're using -m32 -mpower64, you
better know what you are doing and the limitations you have to live under.



Peter

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Segher Boessenkool]

On Wed, Aug 31, 2022 at 04:38:02PM -0500, Peter Bergner wrote:
> On 8/31/22 4:07 PM, Segher Boessenkool wrote:
> > On Wed, Aug 31, 2022 at 02:53:07PM -0500, Peter Bergner wrote:
> >> Changing OS_MISSING_POWERPC64 as I mentioned would not add OPTION_MASK_POWERPC64
> >> to our cpu masks when -m32 is used.
> > 
> > So you say this is where the bug is?
> 
> For linux64.h which is what I think the powerpc64-linux build will use,
> we have:
> 
> linux64.h:#define OS_MISSING_POWERPC64 !TARGET_64BIT
> 
> Doing the macro expansion by hand into:
> 
>   set_masks = POWERPC_MASKS;
> #ifdef OS_MISSING_POWERPC64
>   if (OS_MISSING_POWERPC64)
>     set_masks &= ~OPTION_MASK_POWERPC64;
> #endif
> 
> 
> ...gives us:
> 
>   set_masks = POWERPC_MASKS;
>   if (!TARGET_64BIT)
>     set_masks &= ~OPTION_MASK_POWERPC64;
> 
> So if we handled a -mpowerpc64 earlier on the command line and added
> OPTION_MASK_POWERPC64 to our cpu mask, then a following -m32 use will
> remove it here.
> 
> So I mentioned doing:
> 
> linux64.h:
> - #define OS_MISSING_POWERPC64 !TARGET_64BIT
> + #define OS_MISSING_POWERPC64 0
> 
> ...which disables the above code only for powerpc64-linux builds and doesn't
> affect AIX, Darwin, BSD, etc. or a powerpc-linux build.

But it is incorrect as well.  Instead, we should look if -mpowerpc64 is
enabled explicitly, and not change it if so.

> > The kernel has.  But there are user space things (glibc) that haven't
> > been fixed, and those are default as well.
> 
> Sure, but someone who is using -m32 -mpowerpc64 should know that and
> relying on a 32-bit glibc to save/restore the full 64-bit registers
> is a user error in my book.  If you're using -m32 -mpower64, you
> better know what you are doing and the limitations you have to live under.

Of course.  But -mpowerpc64 can never be any kind of default for 32-bit
Linux.  Not indirectly either.


Segher

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Peter Bergner]

On 8/31/22 4:49 PM, Segher Boessenkool wrote:
> But it is incorrect as well.  Instead, we should look if -mpowerpc64 is
> enabled explicitly, and not change it if so.

Sure, I agree with checking for explicit use.  That said, I'll let
someone else work on this.

Peter

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Kewen.Lin]

Hi Segher and Peter,

Thanks a lot for your insightful comments on this.

I just read through all discussions and plan to give a
try as replied below.

on 2022/8/31 23:24, Segher Boessenkool wrote:
> On Wed, Aug 31, 2022 at 05:33:28PM +0800, Kewen.Lin wrote:
>> Test case bswap64-4.c suffers the issue as its comments:
>>
>> /* On some versions of dejagnu this test will fail when
>>    biarch testing with RUNTESTFLAGS="--target_board=unix
>>    '{-m64,-m32}'" due to -m32 being added on the command
>>    line after the dg-options -mpowerpc64.
>>    common/config/rs6000/rs6000-common.c:
>>    rs6000_handle_option disables -mpowerpc64 for -m32.  */
>>
>> As tested, on test machine with dejaGnu 1.6.2, the compilation
>> option order looks like: -m32 ... -mpowerpc64, option
>> -mpowerpc64 still takes effect;  While on test machine with
>> dejaGnu 1.5.1, the option order looks like: -mpowerpc64 ... -m32,
>> option -mpowerpc64 is disabled by -m32, then the case fails.
> 
> *Should* -mpowerpc64  be disabled by -m32?  

I think the reason to disable -mpowerpc64 at -m32 is that we have
-mpowerpc64 explicitly specified at -m64 (equivalent behavior).

In the current implementation, when -m64 is specified, we set the
bit OPTION_MASK_POWERPC64 in both opts and opts_set.  Since we
set OPTION_MASK_POWERPC64 in opts_set for -m64, when we find the
OPTION_MASK_POWERPC64 is ON in opts_set, we don't know if there
is one actual cmd-line option -mpowerpc64 or just -m64.

Assuming there is -m32 given after -m64 in cmd-line option, it's
also unclear how OPTION_MASK_POWERPC64 in opts_set is set, so
to keep conservative it has to disable -mpowerpc64 to ensure
the options like "-m64 -m32" not to have OPTION_MASK_POWERPC64
ON, just like what we have when just specifying "-m32".

Without any explicit -mpowerpc64 (and -mno-), I think we all agree
that -m64 should set OPTION_MASK_POWERPC64 in opts, conversely -m32
should unset OPTION_MASK_POWERPC64 in opts.

To make -m32/-m64 and -mpowerpc64 orthogonal, IMHO we should not
set bit OPTION_MASK_POWERPC64 in opts_set for -m64.  I'm not sure
if there is some particular reason why we set OPTION_MASK_POWERPC64
in opts_set, I hope no. :)  One possible reason I can imagine is
that we want to get the cmd-line options "-mno-powerpc64 -m64" not
raise error, but I think having it to error makes more senses.

So if no objections I'm going to give it a shot like:

```
Iff -mpowerpc64 (or -mno-powerpc64) is specified, the bit
OPTION_MASK_POWERPC64 in opts_set is set.  Either -m64 and -m32
will leave OPTION_MASK_POWERPC64 in opts alone, it only honors
the specified option, and we will raise error for "-m64" +
"-mno-powerpc64" (either order).

When no explicit -mpowerpc64 (or -mno-powerpc64) is provided,
for -m64, set bit OPTION_MASK_POWERPC64 in opts; while for -m32,
unset bit OPTION_MASK_POWERPC64 in opts.  Both will not touch
OPTION_MASK_POWERPC64 in opts_set.
```

btw, I guess the option compatibility isn't an blocking issue
here, right?

BR,
Kewen

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Segher Boessenkool]

On Thu, Sep 01, 2022 at 05:05:44PM +0800, Kewen.Lin wrote:
> > On Wed, Aug 31, 2022 at 05:33:28PM +0800, Kewen.Lin wrote:
> > *Should* -mpowerpc64  be disabled by -m32?  
> 
> I think the reason to disable -mpowerpc64 at -m32 is that we have
> -mpowerpc64 explicitly specified at -m64 (equivalent behavior).

*Im*plicitly.  Explicit means the user has it on the command line.

> In the current implementation, when -m64 is specified, we set the
> bit OPTION_MASK_POWERPC64 in both opts and opts_set.  Since we
> set OPTION_MASK_POWERPC64 in opts_set for -m64, when we find the
> OPTION_MASK_POWERPC64 is ON in opts_set, we don't know if there
> is one actual cmd-line option -mpowerpc64 or just -m64.

Yes.  That is what _explicit is for :-)

> Without any explicit -mpowerpc64 (and -mno-), I think we all agree
> that -m64 should set OPTION_MASK_POWERPC64 in opts, conversely -m32
> should unset OPTION_MASK_POWERPC64 in opts.

The latter only for OSes that do not handle -mpowerpc64 correctly.

> To make -m32/-m64 and -mpowerpc64 orthogonal, IMHO we should not
> set bit OPTION_MASK_POWERPC64 in opts_set for -m64.

No.  Instead, we should not touch it if the user has explicitly set it
or unset it.  Just like with all other flags :-)

> I'm not sure
> if there is some particular reason why we set OPTION_MASK_POWERPC64
> in opts_set, I hope no. :)  One possible reason I can imagine is
> that we want to get the cmd-line options "-mno-powerpc64 -m64" not
> raise error, but I think having it to error makes more senses.

Agreed.

> btw, I guess the option compatibility isn't an blocking issue
> here, right?

We have survived for years with the status quo, nothing changed recently
that makes it more urgent to fix this.


Segher

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Kewen.Lin]

Hi Segher,

on 2022/9/1 23:04, Segher Boessenkool wrote:
> On Thu, Sep 01, 2022 at 05:05:44PM +0800, Kewen.Lin wrote:
>>> On Wed, Aug 31, 2022 at 05:33:28PM +0800, Kewen.Lin wrote:
>>> *Should* -mpowerpc64  be disabled by -m32?  
>>
>> I think the reason to disable -mpowerpc64 at -m32 is that we have
>> -mpowerpc64 explicitly specified at -m64 (equivalent behavior).
> 
> *Im*plicitly.  Explicit means the user has it on the command line.
> 

aha, let me reword it. :)  ... is that when -m64 is specified we make
it act like -mpowerpc64 is specified explicitly too even if user doesn't
actually specify -mpowerpc64.

>> In the current implementation, when -m64 is specified, we set the
>> bit OPTION_MASK_POWERPC64 in both opts and opts_set.  Since we
>> set OPTION_MASK_POWERPC64 in opts_set for -m64, when we find the
>> OPTION_MASK_POWERPC64 is ON in opts_set, we don't know if there
>> is one actual cmd-line option -mpowerpc64 or just -m64.
> 
> Yes.  That is what _explicit is for :-)
> 
>> Without any explicit -mpowerpc64 (and -mno-), I think we all agree
>> that -m64 should set OPTION_MASK_POWERPC64 in opts, conversely -m32
>> should unset OPTION_MASK_POWERPC64 in opts.
> 
> The latter only for OSes that do not handle -mpowerpc64 correctly.

I think it's the same for the OSes that handle -mpowerpc64 correctly.

Note that it's for the context without any explicit -mpowerpc64 (and
-mno-), assuming we don't "unset OPTION_MASK_POWERPC64 in opts" for
-m32, then the command line "-m64 -m32" would not be the same as
"-m32", since the previous "-m64" sets OPTION_MASK_POWERPC64 in opts
and it's still kept, it's unexpected.

> 
>> To make -m32/-m64 and -mpowerpc64 orthogonal, IMHO we should not
>> set bit OPTION_MASK_POWERPC64 in opts_set for -m64.
> 
> No.  Instead, we should not touch it if the user has explicitly set it
> or unset it.  Just like with all other flags :-)

I may miss something, but I think what we said here is consistent.
"should not set bit OPTION_MASK_POWERPC64 in opts_set" means we should
not make it act as -mpowerpc64 is specified explicitly, (once we won't
do the "unexpected" thing for -m64, then no reason to unset it for -m32
conversely, so explicit set/unset -mpowerpc64 is independent of -m32/-m64). 

BR,
Kewen

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Segher Boessenkool]

On Fri, Sep 02, 2022 at 08:51:01AM +0800, Kewen.Lin wrote:
> on 2022/9/1 23:04, Segher Boessenkool wrote:
> > On Thu, Sep 01, 2022 at 05:05:44PM +0800, Kewen.Lin wrote:
> >> Without any explicit -mpowerpc64 (and -mno-), I think we all agree
> >> that -m64 should set OPTION_MASK_POWERPC64 in opts, conversely -m32
> >> should unset OPTION_MASK_POWERPC64 in opts.
> > 
> > The latter only for OSes that do not handle -mpowerpc64 correctly.
> 
> I think it's the same for the OSes that handle -mpowerpc64 correctly.

No.  -m32 should not set or unset POWERPC64.  The two options are
independent.

-m64 on the other hand forces POWERPC64 to on.  -m64 -mno-powerpc64 is
invalid (and we do indeed error on that).  But we do allow
  -m32 -mno-powerpc64 -m64
(silently enabling it again), urgh.

> 
> Note that it's for the context without any explicit -mpowerpc64 (and
> -mno-), assuming we don't "unset OPTION_MASK_POWERPC64 in opts" for
> -m32, then the command line "-m64 -m32" would not be the same as
> "-m32", since the previous "-m64" sets OPTION_MASK_POWERPC64 in opts
> and it's still kept, it's unexpected.

No.  -m64 -m32 does not set POWERPC64!  Or it shouldn't, in any case :-(


Segher

Re: [PATCH] rs6000/test: Fix bswap64-4.c with has_arch_ppc64 [PR106680]

[Kewen.Lin]

on 2022/9/3 01:44, Segher Boessenkool wrote:
> On Fri, Sep 02, 2022 at 08:51:01AM +0800, Kewen.Lin wrote:
>> on 2022/9/1 23:04, Segher Boessenkool wrote:
>>> On Thu, Sep 01, 2022 at 05:05:44PM +0800, Kewen.Lin wrote:
>>>> Without any explicit -mpowerpc64 (and -mno-), I think we all agree
>>>> that -m64 should set OPTION_MASK_POWERPC64 in opts, conversely -m32
>>>> should unset OPTION_MASK_POWERPC64 in opts.
>>>
>>> The latter only for OSes that do not handle -mpowerpc64 correctly.
>>
>> I think it's the same for the OSes that handle -mpowerpc64 correctly.
> 
> No.  -m32 should not set or unset POWERPC64.  The two options are
> independent.
> 
> -m64 on the other hand forces POWERPC64 to on.  -m64 -mno-powerpc64 is
> invalid (and we do indeed error on that).  But we do allow
>   -m32 -mno-powerpc64 -m64
> (silently enabling it again), urgh.

I just realized the discussion here depends on how we implemented it,
I can understand what you mean now.  Yes, if we implemented it like the
other option supports in rs6000_option_override_internal, we only get
-m32 or -m64 eventually, we don't need to do anything for -m32 but need
to forces POWERPC64 for -m64 if it's not set in opts_set.  The current
implementation by setting and unsetting in command line option handling
is bad, it makes us have to set/unset on the way.

> 
>>
>> Note that it's for the context without any explicit -mpowerpc64 (and
>> -mno-), assuming we don't "unset OPTION_MASK_POWERPC64 in opts" for
>> -m32, then the command line "-m64 -m32" would not be the same as
>> "-m32", since the previous "-m64" sets OPTION_MASK_POWERPC64 in opts
>> and it's still kept, it's unexpected.
> 
> No.  -m64 -m32 does not set POWERPC64!  Or it shouldn't, in any case :-(
> 

Yeah, (... does not set/unset), thanks again for your clarification. :)

BR,
Kewen