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

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

[Qing Zhao]

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

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  <qing.zhao@oracle.com>
+
+       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 <qing.zhao@oracle.com>
+
+       PR testsuite/86519
+       * gcc.dg/strcmpopt_6.c: Scan the assembly file instead of
+       the .expand file.
+


[-- Attachment #2: PR86519.patch --]
[-- Type: application/octet-stream, Size: 1827 bytes --]

From 596e7164df0d84748dc6bf6a3cf39e5f85ab8331 Mon Sep 17 00:00:00 2001
From: qing zhao <qing.zhao@oracle.com>
Date: Tue, 7 Aug 2018 09:52:14 -0700
Subject: [PATCH] fix PR86519

---
 gcc/builtins.c                     | 7 ++++++-
 gcc/testsuite/gcc.dg/strcmpopt_6.c | 6 +++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 39611de..d1d49f5 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -4480,11 +4480,16 @@ expand_builtin_memcmp (tree exp, rtx target, bool result_eq)
 				  /*objsize=*/NULL_TREE);
     }
 
+  /* If the specified length exceeds the size of either object, 
+     call the function.  */
+  if (!no_overflow)
+    return NULL_RTX;
+
   /* Due to the performance benefit, always inline the calls first
      when result_eq is false.  */
   rtx result = NULL_RTX;
 
-  if (!result_eq && fcode != BUILT_IN_BCMP && no_overflow)
+  if (!result_eq && fcode != BUILT_IN_BCMP)
     {
       result = inline_expand_builtin_string_cmp (exp, target);
       if (result)
diff --git a/gcc/testsuite/gcc.dg/strcmpopt_6.c b/gcc/testsuite/gcc.dg/strcmpopt_6.c
index 964b9e4..4c6de02 100644
--- a/gcc/testsuite/gcc.dg/strcmpopt_6.c
+++ b/gcc/testsuite/gcc.dg/strcmpopt_6.c
@@ -1,7 +1,7 @@
 /* When the specified length exceeds one of the arguments of the call to memcmp, 
    the call to memcmp should NOT be inlined.  */
-/* { dg-do run } */
-/* { dg-options "-O2 -fdump-rtl-expand -Wno-stringop-overflow" } */
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wno-stringop-overflow" } */
 
 typedef struct { char s[8]; int x; } S;
 
@@ -33,4 +33,4 @@ int main (void)
 
 }
 
-/* { dg-final { scan-rtl-dump-times "__builtin_memcmp" 6 "expand" } } */
+/* { dg-final { scan-assembler-times "memcmp" 2 } } */
-- 
1.9.1

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

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  <qing.zhao@oracle.com>
> +
> +       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 <qing.zhao@oracle.com>
> +
> +       PR testsuite/86519
> +       * gcc.dg/strcmpopt_6.c: Scan the assembly file instead of
> +       the .expand file.
OK.
jeff

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

[Qing Zhao]

> On Aug 14, 2018, at 11:25 PM, Jeff Law <law@redhat.com> 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  <qing.zhao@oracle.com>
>> +
>> +       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 <qing.zhao@oracle.com>
>> +
>> +       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&revision=263563 <https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=263563>

Qing

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&revision=263563 <https://gcc.gnu.org/viewcvs/gcc?view=revision&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

[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&revision=263563 <https://gcc.gnu.org/viewcvs/gcc?view=revision&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

[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&revision=263563
>>> <https://gcc.gnu.org/viewcvs/gcc?view=revision&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

[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 <ro@CeBiTec.Uni-Bielefeld.DE> 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&revision=263563
>>>> <https://gcc.gnu.org/viewcvs/gcc?view=revision&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

[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 <paul.hua.gm@gmail.com> wrote:
> 
> Hi Qing:
> 
>> 
>> the change has been committed as:
>> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=263563 <https://gcc.gnu.org/viewcvs/gcc?view=revision&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

[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 <qing.zhao@oracle.com> 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 <paul.hua.gm@gmail.com> wrote:
> >
> > Hi Qing:
> >
> >>
> >> the change has been committed as:
> >> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=263563 <https://gcc.gnu.org/viewcvs/gcc?view=revision&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

[Paul Hua]

On Wed, Aug 22, 2018 at 2:15 AM Qing Zhao <qing.zhao@oracle.com> wrote:
>
>
> > On Aug 21, 2018, at 8:07 AM, Paul Hua <paul.hua.gm@gmail.com> 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

[Qing Zhao]

thanks.
now, I can repeat the failure.

Qing
> On Aug 21, 2018, at 7:25 PM, Paul Hua <paul.hua.gm@gmail.com> wrote:
> 
> On Wed, Aug 22, 2018 at 2:15 AM Qing Zhao <qing.zhao@oracle.com <mailto:qing.zhao@oracle.com>> wrote:
>> 
>> 
>>> On Aug 21, 2018, at 8:07 AM, Paul Hua <paul.hua.gm@gmail.com> 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

[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 <ro@CeBiTec.Uni-Bielefeld.DE> 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

[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]

> On Aug 22, 2018, at 10:50 AM, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> 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

[Jeff Law]

On 08/22/2018 11:05 AM, Qing Zhao wrote:
> 
>> On Aug 22, 2018, at 10:50 AM, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> 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 5:01 PM, Jeff Law <law@redhat.com> wrote:
> 
> On 08/22/2018 11:05 AM, Qing Zhao wrote:
>> 
>>> On Aug 22, 2018, at 10:50 AM, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> 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/23/2018 10:13 AM, Qing Zhao wrote:
> 
>> On Aug 22, 2018, at 5:01 PM, Jeff Law <law@redhat.com> wrote:
>>
>> On 08/22/2018 11:05 AM, Qing Zhao wrote:
>>>
>>>> On Aug 22, 2018, at 10:50 AM, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> 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
>