Bug 18347 - On ARM, "LDR =something" with missing destination register is silently ignored
Summary: On ARM, "LDR =something" with missing destination register is silently ignored
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.25
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-28 17:01 UTC by Solra Bizna
Modified: 2015-05-08 16:36 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Proposed patch (867 bytes, patch)
2015-04-29 17:26 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Solra Bizna 2015-04-28 17:01:45 UTC
A simple test program that demonstrates this problem:

    MOV r1, r0
    LDR =garbage // no destination register
    MOV r2, r3

Assembles without any error or warning, and disassembles as:

00000000 <.text>:
   0:   e1a01000        mov     r1, r0
   4:   e1a02003        mov     r2, r3

The effect is the same if the "garbage" symbol is actually defined. I made this mistake while writing an IO routine, and had to look through the disassembly to figure out why the register had the wrong value.

GAS should throw an error when it encounters such an instruction, since there is no sensible way to interpret it.
Comment 1 Nick Clifton 2015-04-29 17:26:56 UTC
Created attachment 8290 [details]
Proposed patch
Comment 2 Nick Clifton 2015-04-29 17:30:31 UTC
Hi Solra,

  This was an interesting one.  The problem is that "LDR = garbage" is a valid GAS operation - it sets a symbol called "LDR" to the value of an (undefined in this case) symbol called "garbage".  Hence there was no warning message, and no LDR instruction in the output.

  Please could you try out the uploaded patch as a possible fix for this problem ?  It makes the ARM port of GAS issue a warning message when the user attempts to create a symbol with the same name as an ARM instruction.

  One thing that I am not sure about is whether we need a command line option to suppress this warning, in the case that the user really does want to create a symbol that matches an instruction.

Cheers
  Nick
Comment 3 Solra Bizna 2015-04-29 19:31:12 UTC
Ah! That explains what's happening.

Just tried the patch. It works great.

As far as adding some way to suppress the warning... Instruction set extensions mean that an acceptable symbol one day will cause a warning tomorrow. Having some way to suppress the warning would be good. If it's possible, I'd suggest keeping the warning on definitions of the form "X=Y", and having no warning on the rather more explicit (but equivalent, right?) ".set X, Y". If the user really wants a symbol with that kind of name, they oughtn't mind being explicit about it.
Comment 4 Sourceware Commits 2015-04-30 10:19:49 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=8b2d793ce5ee03336d6c1d1f30b8d296cbe443de

commit 8b2d793ce5ee03336d6c1d1f30b8d296cbe443de
Author: Nick Clifton <nickc@redhat.com>
Date:   Thu Apr 30 11:17:55 2015 +0100

    GAS ARM: Warn if the user creates a symbol with the same name as an instruction.
    
    	PR gas/18347
    gas	* config/tc-arm.c (md_undefined_symbol): Issue a warning message
    	(if enabled) when the user creates a symbol with the same name as
    	an ARM instruction.
    	(flag_warn_syms): New static variable.
    	(arm_opts): Add mwarn-syms and mno-warn-syms.
    	* doc/c-arm.texi (ARM Options): Document the -m[no-]warn-syms
    	options.
    
    tests	* gas/arm/pr18347.s: New file: Test case.
    	* gas/arm/pr18347.l: New file: Expected assembler output.
    	* gas/arm/pr18347.d: New file: Test driver.
Comment 5 Nick Clifton 2015-04-30 10:22:34 UTC
Hi Solra,

  OK - I have enhanced the patch to add a new command line option to disable the warning (-mno-warn-sym).  I have also added a testcase and some documentation.  I did not make the check specific to the X=Y form as this would involve modifying generic code, and I wanted to keep this patch in the ARM backend.  If this kind of problem turns up with other targets then I may reconsider this decision.

Cheers
  Nick
Comment 6 Vidya Praveen 2015-05-06 15:21:23 UTC
Like I mentioned in the mailing list*, this warning should be restricted to <mnemonic> = <value> scenarios (and perhaps in future other possible similar scenarios where an instruction name can be misunderstood for a symbol name).

* https://sourceware.org/ml/binutils/2015-05/msg00035.html
Comment 7 Sourceware Commits 2015-05-08 16:29:45 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ae8714c2712ef9a179cfa9158a289bd400c0ad97

commit ae8714c2712ef9a179cfa9158a289bd400c0ad97
Author: Nick Clifton <nickc@redhat.com>
Date:   Fri May 8 17:28:26 2015 +0100

    Change ARM symbol name verification code so that it only triggers when the form "name = val" is used.
    
    	PR gas/18347
    	* config/tc-arm.h (TC_EQUAL_IN_INSN): Define.
    	* config/tc-arm.c (arm_tc_equal_in_insn): New function.  Move
    	the symbol name checking code to here from...
    	(md_undefined_symbo): ... here.
Comment 8 Nick Clifton 2015-05-08 16:33:53 UTC
Right.  I have found a way to restrict the test so that it only triggers when the "name = val" form is used, and without having to modify generic code.

I have updated the assembler testcase so that it includes several other varieties of symbol assignment using ARM instruction names, in order to make sure that these assignments do not trigger the warning.

I have also run the ARM simulator testsuite and verified that this update fixes the problems with the label names used in some of the tests there.

Cheers
  Nick

This time it is right, it will work, and no one will have to get nailed to anything.
Comment 9 Vidya Praveen 2015-05-08 16:36:03 UTC
Thanks Nick.