Re: [PATCH][Middle-end]patch for fixing PR 86519

[Jeff Law]

On 08/23/2018 10:13 AM, Qing Zhao wrote:
> 
>> On Aug 22, 2018, at 5:01 PM, Jeff Law  wrote:
>>
>> On 08/22/2018 11:05 AM, Qing Zhao wrote:
>>>
 On Aug 22, 2018, at 10:50 AM, Rainer Orth  
 wrote:

 Hi Qing,

> From the comments you put into PR86519, for SPARC, looks like that only
> 32-bit sparc has the problem.
> sparcv9 does NOT have the same issue.
>
> I was trying to find the string to represent 32-bit sparc target, but
> haven’t found it.
>
> my guess is:   sparc32*-*-*,  is this correct?

 no, certainly not.  You need to use something like sparc*-*-* && ilp32
 to catch the 32-bit multilib in both sparc-*-* and sparcv9-*-*
 configurations.  This is similar to { i?86-*-* x86_64-*-* } && ilp32 on 
 x86.
>>>
>>> thanks for the info.
>>>

 I'm still doubtful that enumerating target after target which all fail
 the original test for unrelated reasons is the way to go, especially
 given that there are others affected besides mips and sparc.
>>>
>>> I am not sure, either.
>>>
>>> however, given the available directives provided in testing suite, what’s 
>>> the better solution
>>> to this problem?
>> We could move the test into the i386 target specific test directory.
>> It'll still get good coverage that way and it'll be naturally restricted
>> to a target where we don't have to worry about the function name we're
>> scanning for showing up in undesirable contexts.
> 
> I will do this.  is it better to add it to both i386 and aarch64 target?
If we've got verification that it's working on aarch64, then adding it
to both is fine.

jeff
> 

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Qing Zhao]

> On Aug 22, 2018, at 5:01 PM, Jeff Law  wrote:
> 
> On 08/22/2018 11:05 AM, Qing Zhao wrote:
>> 
>>> On Aug 22, 2018, at 10:50 AM, Rainer Orth  
>>> wrote:
>>> 
>>> Hi Qing,
>>> 
 From the comments you put into PR86519, for SPARC, looks like that only
 32-bit sparc has the problem.
 sparcv9 does NOT have the same issue.
 
 I was trying to find the string to represent 32-bit sparc target, but
 haven’t found it.
 
 my guess is:   sparc32*-*-*,  is this correct?
>>> 
>>> no, certainly not.  You need to use something like sparc*-*-* && ilp32
>>> to catch the 32-bit multilib in both sparc-*-* and sparcv9-*-*
>>> configurations.  This is similar to { i?86-*-* x86_64-*-* } && ilp32 on x86.
>> 
>> thanks for the info.
>> 
>>> 
>>> I'm still doubtful that enumerating target after target which all fail
>>> the original test for unrelated reasons is the way to go, especially
>>> given that there are others affected besides mips and sparc.
>> 
>> I am not sure, either.
>> 
>> however, given the available directives provided in testing suite, what’s 
>> the better solution
>> to this problem?
> We could move the test into the i386 target specific test directory.
> It'll still get good coverage that way and it'll be naturally restricted
> to a target where we don't have to worry about the function name we're
> scanning for showing up in undesirable contexts.

I will do this.  is it better to add it to both i386 and aarch64 target?

Qing
> 
> Another approach might be to scan the RTL dumps.  But if there were a
> target that didn't have direct jumps and requires a hi+lo_sum style
> sequence to load a symbolic constant it would fail.
> 
> jeff

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Jeff Law]

On 08/22/2018 11:05 AM, Qing Zhao wrote:
> 
>> On Aug 22, 2018, at 10:50 AM, Rainer Orth  
>> wrote:
>>
>> Hi Qing,
>>
>>> From the comments you put into PR86519, for SPARC, looks like that only
>>> 32-bit sparc has the problem.
>>> sparcv9 does NOT have the same issue.
>>>
>>> I was trying to find the string to represent 32-bit sparc target, but
>>> haven’t found it.
>>>
>>> my guess is:   sparc32*-*-*,  is this correct?
>>
>> no, certainly not.  You need to use something like sparc*-*-* && ilp32
>> to catch the 32-bit multilib in both sparc-*-* and sparcv9-*-*
>> configurations.  This is similar to { i?86-*-* x86_64-*-* } && ilp32 on x86.
> 
> thanks for the info.
> 
>>
>> I'm still doubtful that enumerating target after target which all fail
>> the original test for unrelated reasons is the way to go, especially
>> given that there are others affected besides mips and sparc.
> 
> I am not sure, either.
> 
> however, given the available directives provided in testing suite, what’s the 
> better solution
> to this problem?
We could move the test into the i386 target specific test directory.
It'll still get good coverage that way and it'll be naturally restricted
to a target where we don't have to worry about the function name we're
scanning for showing up in undesirable contexts.

Another approach might be to scan the RTL dumps.  But if there were a
target that didn't have direct jumps and requires a hi+lo_sum style
sequence to load a symbolic constant it would fail.

jeff

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Qing Zhao]

> On Aug 22, 2018, at 10:50 AM, Rainer Orth  
> wrote:
> 
> Hi Qing,
> 
>> From the comments you put into PR86519, for SPARC, looks like that only
>> 32-bit sparc has the problem.
>> sparcv9 does NOT have the same issue.
>> 
>> I was trying to find the string to represent 32-bit sparc target, but
>> haven’t found it.
>> 
>> my guess is:   sparc32*-*-*,  is this correct?
> 
> no, certainly not.  You need to use something like sparc*-*-* && ilp32
> to catch the 32-bit multilib in both sparc-*-* and sparcv9-*-*
> configurations.  This is similar to { i?86-*-* x86_64-*-* } && ilp32 on x86.

thanks for the info.

> 
> I'm still doubtful that enumerating target after target which all fail
> the original test for unrelated reasons is the way to go, especially
> given that there are others affected besides mips and sparc.

I am not sure, either.

however, given the available directives provided in testing suite, what’s the 
better solution
to this problem?

thanks.

Qing
> 
>   Rainer
> 
> -- 
> -
> Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Rainer Orth]

Hi Qing,

> From the comments you put into PR86519, for SPARC, looks like that only
> 32-bit sparc has the problem.
> sparcv9 does NOT have the same issue.
>
> I was trying to find the string to represent 32-bit sparc target, but
> haven’t found it.
>
> my guess is:   sparc32*-*-*,  is this correct?

no, certainly not.  You need to use something like sparc*-*-* && ilp32
to catch the 32-bit multilib in both sparc-*-* and sparcv9-*-*
configurations.  This is similar to { i?86-*-* x86_64-*-* } && ilp32 on x86.

I'm still doubtful that enumerating target after target which all fail
the original test for unrelated reasons is the way to go, especially
given that there are others affected besides mips and sparc.

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Qing Zhao]

Hi, Rainer,

>From the comments you put into PR86519, for SPARC, looks like that only 32-bit 
>sparc has the problem.
sparcv9 does NOT have the same issue.

I was trying to find the string to represent 32-bit sparc target,   but haven’t 
found it.

my guess is:   sparc32*-*-*,  is this correct?


> On Aug 20, 2018, at 3:57 AM, Rainer Orth  
> wrote:
> 
> Hi Jeff,
> 
>>> 
>> Ugh.  :(  There's probably other targets that are going to have similar
>> properties.  I'm not really sure how to write a suitable assembler test
>> for this.
>> 
>> Maybe we just need a different search result for MIPS (and potentially
>> other targets).
> 
> we certainly do: I've reported a similar issue on sparc in PR
> testsuite/86519, but powerpc and s390x are equally affected.

do you mean that on powerpc and s390x, there is the same issue?

I tried on powerpc machines, seems no such issue. please advice.

thanks.

Qing

> 
>   Rainer
> 
> -- 
> -
> Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Qing Zhao]

thanks.
now, I can repeat the failure.

Qing
> On Aug 21, 2018, at 7:25 PM, Paul Hua  wrote:
> 
> On Wed, Aug 22, 2018 at 2:15 AM Qing Zhao  > wrote:
>> 
>> 
>>> On Aug 21, 2018, at 8:07 AM, Paul Hua  wrote:
>>> 
>>> Hi, Qing,
>>> 
>>> The cfarm machine gcc23 can build mips64el successful, configure with
>>> "../configure --prefix=/opt/gcc-9 MISSING=texinfo MAKEINFO=missing
>>> --target=mips64el-linux-gnu --enable-languages=c,c++
>> 
>> I got the same failure message on gcc23 as gcc22, even with the above 
>> configure:
>> 
>> /usr/bin/ld: failed to merge target specific data of file 
>> /usr/lib32/libc.a(mremap.o)
>> /usr/bin/ld: /usr/lib32/libc.a(libc-lowlevellock.o): ABI is incompatible 
>> with that of the selected emulation
>> 
>> not sure what’s the problem?
>> 
> 
> I just build all-gcc and make check.
> 
> ./configure xxx
> make all-gcc -j 2
> make check-gcc RUNTESTFLAGS="dg.exp=strcmpopt_6.c"
> 
> It's enough to reproduce the regression.
> 
> Here is a mips port patch.
> 
> diff --git a/gcc/testsuite/gcc.dg/strcmpopt_6.c
> b/gcc/testsuite/gcc.dg/strcmpopt_6.c
> index 4c6de02824f..fa0ff9d1171 100644
> --- a/gcc/testsuite/gcc.dg/strcmpopt_6.c
> +++ b/gcc/testsuite/gcc.dg/strcmpopt_6.c
> @@ -33,4 +33,5 @@ int main (void)
> 
> }
> 
> -/* { dg-final { scan-assembler-times "memcmp" 2 } } */
> +/* { dg-final { scan-assembler-times "memcmp" 2 { target { !
> mips*-*-* } } } } */
> +/* { dg-final { scan-assembler-times "memcmp" 4 { target { mips*-*-* } } } } 
> */
> 
> Paul Hua

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Paul Hua]

On Wed, Aug 22, 2018 at 2:15 AM Qing Zhao  wrote:
>
>
> > On Aug 21, 2018, at 8:07 AM, Paul Hua  wrote:
> >
> > Hi, Qing,
> >
> > The cfarm machine gcc23 can build mips64el successful, configure with
> > "../configure --prefix=/opt/gcc-9 MISSING=texinfo MAKEINFO=missing
> > --target=mips64el-linux-gnu --enable-languages=c,c++
>
> I got the same failure message on gcc23 as gcc22, even with the above 
> configure:
>
> /usr/bin/ld: failed to merge target specific data of file 
> /usr/lib32/libc.a(mremap.o)
> /usr/bin/ld: /usr/lib32/libc.a(libc-lowlevellock.o): ABI is incompatible with 
> that of the selected emulation
>
> not sure what’s the problem?
>

I just build all-gcc and make check.

./configure xxx
make all-gcc -j 2
make check-gcc RUNTESTFLAGS="dg.exp=strcmpopt_6.c"

It's enough to reproduce the regression.

Here is a mips port patch.

diff --git a/gcc/testsuite/gcc.dg/strcmpopt_6.c
b/gcc/testsuite/gcc.dg/strcmpopt_6.c
index 4c6de02824f..fa0ff9d1171 100644
--- a/gcc/testsuite/gcc.dg/strcmpopt_6.c
+++ b/gcc/testsuite/gcc.dg/strcmpopt_6.c
@@ -33,4 +33,5 @@ int main (void)

 }

-/* { dg-final { scan-assembler-times "memcmp" 2 } } */
+/* { dg-final { scan-assembler-times "memcmp" 2 { target { !
mips*-*-* } } } } */
+/* { dg-final { scan-assembler-times "memcmp" 4 { target { mips*-*-* } } } } */

Paul Hua

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Paul Hua]

Hi, Qing,

The cfarm machine gcc23 can build mips64el successful, configure with
"../configure --prefix=/opt/gcc-9 MISSING=texinfo MAKEINFO=missing
--target=mips64el-linux-gnu --enable-languages=c,c++
On Tue, Aug 21, 2018 at 7:02 AM Qing Zhao  wrote:
>
> Hi, Paul,
>
> I was trying to repeat this issue on a mips machine today, but failed…
>
> the only mips machines I can access are those in gcc compile farm, I chose 
> gcc22, but failed to build GCC on this machine.
>
> do you know any other machine in gcc compile farm that can repeat this issue?
>
> thanks a lot.
>
> Qing
> > On Aug 17, 2018, at 10:43 PM, Paul Hua  wrote:
> >
> > Hi Qing:
> >
> >>
> >> the change has been committed as:
> >> https://gcc.gnu.org/viewcvs/gcc?view=revision=263563 
> >> 
> >>
> >> Qing
> >>
> >
> > The strcmpopt_6.c test still fails on mips64el target.
> >
> > gcc.dg/strcmpopt_6.c: memcmp found 4 times
> > FAIL: gcc.dg/strcmpopt_6.c scan-assembler-times memcmp 2
> >
> >
> > The mips asm output have ".reloc" info.
> >
> > -
> >ld  $5,%got_page(.LC0)($28)
> >ld  $25,%call16(memcmp)($28)
> >li  $6,3# 0x3
> >sd  $31,8($sp)
> >.reloc  1f,R_MIPS_JALR,memcmp
> > 1:  jalr$25
> >daddiu  $5,$5,%got_ofst(.LC0)
> > 
> >
> > scan-assembler find "4" times.
> >
> > Thanks
> > Paul Hua
>

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Qing Zhao]

Hi, Paul,

I was trying to repeat this issue on a mips machine today, but failed…

the only mips machines I can access are those in gcc compile farm, I chose 
gcc22, but failed to build GCC on this machine.

do you know any other machine in gcc compile farm that can repeat this issue?

thanks a lot.

Qing
> On Aug 17, 2018, at 10:43 PM, Paul Hua  wrote:
> 
> Hi Qing:
> 
>> 
>> the change has been committed as:
>> https://gcc.gnu.org/viewcvs/gcc?view=revision=263563 
>> 
>> 
>> Qing
>> 
> 
> The strcmpopt_6.c test still fails on mips64el target.
> 
> gcc.dg/strcmpopt_6.c: memcmp found 4 times
> FAIL: gcc.dg/strcmpopt_6.c scan-assembler-times memcmp 2
> 
> 
> The mips asm output have ".reloc" info.
> 
> -
>ld  $5,%got_page(.LC0)($28)
>ld  $25,%call16(memcmp)($28)
>li  $6,3# 0x3
>sd  $31,8($sp)
>.reloc  1f,R_MIPS_JALR,memcmp
> 1:  jalr$25
>daddiu  $5,$5,%got_ofst(.LC0)
> 
> 
> scan-assembler find "4" times.
> 
> Thanks
> Paul Hua

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Qing Zhao]

Hi, Rainer,

thanks a lot to report the issues with mips and sparc platform.

Yes, looks like even on the assembly level, the string scanning still not 
reliable on different platforms.

I agree with Jeff’s suggestion to apply different search result for different 
platforms.

I will update the testcase with this approach soon.

Qing
> On Aug 20, 2018, at 3:57 AM, Rainer Orth  
> wrote:
> 
> Hi Jeff,
> 
>> On 08/17/2018 09:43 PM, Paul Hua wrote:
>>> Hi Qing:
>>> 
 
 the change has been committed as:
 https://gcc.gnu.org/viewcvs/gcc?view=revision=263563
 
 
 Qing
 
>>> 
>>> The strcmpopt_6.c test still fails on mips64el target.
>>> 
>>> gcc.dg/strcmpopt_6.c: memcmp found 4 times
>>> FAIL: gcc.dg/strcmpopt_6.c scan-assembler-times memcmp 2
>>> 
>>> 
>>> The mips asm output have ".reloc" info.
>>> 
>>> -
>>>ld  $5,%got_page(.LC0)($28)
>>>ld  $25,%call16(memcmp)($28)
>>>li  $6,3# 0x3
>>>sd  $31,8($sp)
>>>.reloc  1f,R_MIPS_JALR,memcmp
>>> 1:  jalr$25
>>>daddiu  $5,$5,%got_ofst(.LC0)
>>> 
>>> 
>>> scan-assembler find "4" times.
>> Ugh.  :(  There's probably other targets that are going to have similar
>> properties.  I'm not really sure how to write a suitable assembler test
>> for this.
>> 
>> Maybe we just need a different search result for MIPS (and potentially
>> other targets).
> 
> we certainly do: I've reported a similar issue on sparc in PR
> testsuite/86519, but powerpc and s390x are equally affected.
> 
>   Rainer
> 
> -- 
> -
> Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Rainer Orth]

Hi Jeff,

> On 08/17/2018 09:43 PM, Paul Hua wrote:
>> Hi Qing:
>> 
>>>
>>> the change has been committed as:
>>> https://gcc.gnu.org/viewcvs/gcc?view=revision=263563
>>> 
>>>
>>> Qing
>>>
>> 
>> The strcmpopt_6.c test still fails on mips64el target.
>> 
>> gcc.dg/strcmpopt_6.c: memcmp found 4 times
>> FAIL: gcc.dg/strcmpopt_6.c scan-assembler-times memcmp 2
>> 
>> 
>> The mips asm output have ".reloc" info.
>> 
>> -
>> ld  $5,%got_page(.LC0)($28)
>> ld  $25,%call16(memcmp)($28)
>> li  $6,3# 0x3
>> sd  $31,8($sp)
>> .reloc  1f,R_MIPS_JALR,memcmp
>> 1:  jalr$25
>> daddiu  $5,$5,%got_ofst(.LC0)
>> 
>> 
>> scan-assembler find "4" times.
> Ugh.  :(  There's probably other targets that are going to have similar
> properties.  I'm not really sure how to write a suitable assembler test
> for this.
>
> Maybe we just need a different search result for MIPS (and potentially
> other targets).

we certainly do: I've reported a similar issue on sparc in PR
testsuite/86519, but powerpc and s390x are equally affected.

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Jeff Law]

On 08/17/2018 09:43 PM, Paul Hua wrote:
> Hi Qing:
> 
>>
>> the change has been committed as:
>> https://gcc.gnu.org/viewcvs/gcc?view=revision=263563 
>> 
>>
>> Qing
>>
> 
> The strcmpopt_6.c test still fails on mips64el target.
> 
> gcc.dg/strcmpopt_6.c: memcmp found 4 times
> FAIL: gcc.dg/strcmpopt_6.c scan-assembler-times memcmp 2
> 
> 
> The mips asm output have ".reloc" info.
> 
> -
> ld  $5,%got_page(.LC0)($28)
> ld  $25,%call16(memcmp)($28)
> li  $6,3# 0x3
> sd  $31,8($sp)
> .reloc  1f,R_MIPS_JALR,memcmp
> 1:  jalr$25
> daddiu  $5,$5,%got_ofst(.LC0)
> 
> 
> scan-assembler find "4" times.
Ugh.  :(  There's probably other targets that are going to have similar
properties.  I'm not really sure how to write a suitable assembler test
for this.

Maybe we just need a different search result for MIPS (and potentially
other targets).

jeff

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Paul Hua]

Hi Qing:

>
> the change has been committed as:
> https://gcc.gnu.org/viewcvs/gcc?view=revision=263563 
> 
>
> Qing
>

The strcmpopt_6.c test still fails on mips64el target.

gcc.dg/strcmpopt_6.c: memcmp found 4 times
FAIL: gcc.dg/strcmpopt_6.c scan-assembler-times memcmp 2


The mips asm output have ".reloc" info.

-
ld  $5,%got_page(.LC0)($28)
ld  $25,%call16(memcmp)($28)
li  $6,3# 0x3
sd  $31,8($sp)
.reloc  1f,R_MIPS_JALR,memcmp
1:  jalr$25
daddiu  $5,$5,%got_ofst(.LC0)


scan-assembler find "4" times.

Thanks
Paul Hua

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Qing Zhao]

> On Aug 14, 2018, at 11:25 PM, Jeff Law  wrote:
> 
> On 08/14/2018 08:57 AM, Qing Zhao wrote:
>> Hi,
>> 
>> PR 86519:New test case gcc.dg/strcmpopt_6.c fails with its introduction in 
>> r262636.
>> 
>> gcc/ChangeLog:
>> 
>> +2018-08-14  Qing Zhao  
>> +
>> +   PR testsuite/86519
>> +   * builtins.c (expand_builtin_memcmp): Do not expand the call
>> +   when overflow is detected.
>> +
>> gcc/testsuite/ChangeLog:
>> 
>> +2018-08-14  Qing Zhao 
>> +
>> +   PR testsuite/86519
>> +   * gcc.dg/strcmpopt_6.c: Scan the assembly file instead of
>> +   the .expand file.
> OK.

thanks.

the change has been committed as:
https://gcc.gnu.org/viewcvs/gcc?view=revision=263563 


Qing

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Jeff Law]

On 08/14/2018 08:57 AM, Qing Zhao wrote:
> Hi,
> 
> PR 86519:New test case gcc.dg/strcmpopt_6.c fails with its introduction in 
> r262636.
> 
> ***the root cause is:
> 
> for the following call to memcmp:   __builtin_memcmp (s->s, "a", 3);
> the specified length 3 is larger than the length of "a", it's clearly an 
> out-of-bound access. This new testing case is try to claim that,
> For such out-of-bound access, we should NOT expand this call at all. The new 
> added in-lining expansion was prohibited under
> such situation, However, the expansion to hardware compare insn (old code) is 
> NOT prohibited under such situation. 
> on powerPC, the above call to memcmp is expanded to hardware compare insn. 
> therefore, the testing case failed.
> 
> ***in addition to the above major issue, there is also one minor issue with 
> the new testing case itself:
> 
> dg-final { scan-rtl-dump-times "__builtin_memcmp" 6 "expand” }
> this is trying to scan the dumped .expand file to match the string 
> “__builtin_memcmp” exactly 6 times. however, the # of times that
> the string “__builtin_memcmp” appears in the .expand file varies on different 
> target or optimization level, in order to avoid such
> instability, instead of scanning the .expand file to match the string 
> “__builtin_memcmp”,  scanning the final assembly file to match
> the string “memcmp”.
> 
> please review the attached simple patch.
> 
> thanks.
> 
> Qing
> 
> gcc/ChangeLog:
> 
> +2018-08-14  Qing Zhao  
> +
> +   PR testsuite/86519
> +   * builtins.c (expand_builtin_memcmp): Do not expand the call
> +   when overflow is detected.
> +
> gcc/testsuite/ChangeLog:
> 
> +2018-08-14  Qing Zhao 
> +
> +   PR testsuite/86519
> +   * gcc.dg/strcmpopt_6.c: Scan the assembly file instead of
> +   the .expand file.
OK.
jeff