This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR69951
- From: Ramana Radhakrishnan <ramana dot radhakrishnan at foss dot arm dot com>
- To: Richard Biener <rguenther at suse dot de>, James Greenhalgh <james dot greenhalgh at arm dot com>
- Cc: gcc-patches at gcc dot gnu dot org, Kyrylo Tkachov <kyrtka01 at arm dot com>, Richard Earnshaw <rearnsha at arm dot com>, Ramana Radhakrishnan <ramrad01 at arm dot com>, nickc at redhat dot com
- Date: Tue, 1 Mar 2016 10:48:05 +0000
- Subject: Re: [PATCH] Fix PR69951
- Authentication-results: sourceware.org; auth=none
- References: <alpine dot LSU dot 2 dot 11 dot 1602260930480 dot 31547 at t29 dot fhfr dot qr> <20160229173637 dot GA33430 at arm dot com> <alpine dot LSU dot 2 dot 11 dot 1603011017200 dot 31547 at t29 dot fhfr dot qr> <20160301095149 dot GA20934 at arm dot com> <alpine dot LSU dot 2 dot 11 dot 1603011053120 dot 31547 at t29 dot fhfr dot qr>
On 01/03/16 09:54, Richard Biener wrote:
> On Tue, 1 Mar 2016, James Greenhalgh wrote:
>
>> On Tue, Mar 01, 2016 at 10:21:27AM +0100, Richard Biener wrote:
>>> On Mon, 29 Feb 2016, James Greenhalgh wrote:
>>>
>>>> On Fri, Feb 26, 2016 at 09:32:53AM +0100, Richard Biener wrote:
>>>>>
>>>>> The following fixes PR69951, hopefully the last case of decl alias
>>>>> issues with alias analysis. This time it's points-to and the DECL_UIDs
>>>>> used in points-to sets not being canonicalized.
>>>>>
>>>>> The simplest (and cheapest) fix is to make aliases refer to the
>>>>> ultimate alias target via their DECL_PT_UID which we conveniently
>>>>> have available.
>>>>>
>>>>> Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
>>>>>
>>>>> Richard.
>>>>>
>>>>> 2016-02-26 Richard Biener <rguenther@suse.de>
>>>>>
>>>>> PR tree-optimization/69551
>>>>> * tree-ssa-structalias.c (get_constraint_for_ssa_var): When
>>>>> looking through aliases adjust DECL_PT_UID to refer to the
>>>>> ultimate alias target.
>>>>>
>>>>> * gcc.dg/torture/pr69951.c: New testcase.
>>>>
>>>> I see this new testcase failing on an ARM target as so:
>>>>
>>>> /tmp/ccChjoFc.s: Assembler messages:
>>>> /tmp/ccChjoFc.s:21: Warning: [-mwarn-syms]: Assignment makes a symbol match an ARM instruction: b
>>>>
>>>> FAIL: gcc.dg/torture/pr69951.c -O0 (test for excess errors)
>>>>
>>>> But I haven't managed to reproduce it outside of the test environment.
>>>>
>>>> The fix looks trivial, rename b to anything else you fancy (well... stay
>>>> clear of add and ldr). I'll put a fix in myself if I can manage to get
>>>> this to reproduce - though if anyone else wants to do it I won't be
>>>> offended :-).
>>>
>>> Huh, I wonder what's the use of such warning. After all 'ldr' is a valid
>>> C symbol name, too. In fact my cross arm as doesn't report this
>>> warning (binutils 2.25.0)
>>>
>>>> arm-suse-linux-gnueabi-as t.s -mwarn-syms
>>> Assembler messages:
>>> Error: unrecognized option -mwarn-syms
>>
>> Right, I've figured out the set of conditions... You need to be targeting
>> an arm-*-linux-* system to make sure that the ASM_OUTPUT_DEF definition
>> from config/arm/linux-elf.h is pulled in. This causes us to emit:
>>
>> b = a
>>
>> Rather than
>>
>> .set b,a
>>
>> Writing it as "b = a" causes the warning added to resolve binutils
>> PR18347 [1] to kick in, so you need binutils > 2.26 or to have backported
>> that patch).
>>
>> Resolving it by hacking the testcase would be one fix, but I wonder why the
>> ARM port prefers to emit "b = a" in a linux environment if .set does the
>> same thing and always avoids the warning? Maybe Ramana/Richard/Kyrill/Nick
>> remember?
>> (AArch64 does the same thing, but the AArch64 gas port doesn't
>> have the PR18347 fix).
>
> So does b = a define a macro then and the warning is to avoid you
> doing
I don't think this is a macro, b = a seems to be a way of setting the value of a to b. in the assembler. If a is an expression , then I believe the expression is resolved at assemble time - (b ends up being a symbol in the symbol table produced with the value of a) in this case the address of a. .set b, a achieves the same thing from my experiments and reading of the sources. The reason ports appear to choose not to use the .set a, b idiom is if the assembler syntax has hijacked the .set directive for something else. Thus I don't see why we use the ASM_OUTPUT_DEF form in the GNU/Linux port TBH rather than the .set form especially as we don't reuse .set for anything else in the ARM assembler port and SET_ASM_OP is defined in config/arm/aout.h.
The use of .set in the arm port of glibc for assembler code for the same purpose seems to also vindicate that kind of thought.
No reasons were given here[1], maybe Nick or Richard remember from nearly 18 years ago ;)
Therefore this seems to be an assembler bug to me in that it doesn't allow such an assignment of values, and a backend wart to me that we have ASM_OUTPUT_DEF defined for no good reason. So, a patch that removes ASM_OUTPUT_DEF from linux-elf.h seems obvious to me pending testing.
Nick , Richard - any thoughts ?
regards
Ramana
1. https://gcc.gnu.org/ml/gcc-patches/1998-10/msg00701.html
>
> ...
>
> ldr 0, 1 (or whatever correct ldr instruction)
>
> and have that ldr replaced by b?
>
> Then it's a bug to emit aliases in this form and I hope .set ldr, b
> doesn't have the same effect.
>
> Richard.
>