Bug 19910 - Unsigned arithmetic and %hi()/%low() operators
Summary: Unsigned arithmetic and %hi()/%low() operators
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.26
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-05 14:13 UTC by Orlando Arias
Modified: 2016-04-08 09:39 UTC (History)
3 users (show)

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


Attachments
Proposed patch (311 bytes, patch)
2016-04-05 16:13 UTC, Nick Clifton
Details | Diff
Revised patch (919 bytes, patch)
2016-04-06 09:10 UTC, Nick Clifton
Details | Diff
extended patch (1.54 KB, patch)
2016-04-06 09:33 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Orlando Arias 2016-04-05 14:13:39 UTC
Working with he sparc-leon3-linux target in gas versions 2.26.20160125
and 2.23, the following behaviour is observed:

sethi %hi(((((0x40000000U + 0x40000000U) - 160)-(8192U-160))-32U)), %fp
or %g0, %lo(((((0x40000000U +0x40000000U) - 160)-(8192U-160))-32U)), %fp

These statements fail to assemble with the error:

Error: missing ')'

However, the following assembles with no issues:

sethi %hi(((((0x40000000 + 0x40000000) - 160)-(8192-160))-32)), %fp
or %g0, %lo(((((0x40000000 +0x40000000) - 160)-(8192-160))-32)), %fp

The issue can be reduced to the following lines:

or %g0, %lo(0x40000000U +0x40000000U), %g1
or %g0, %lo((0x40000000U +0x40000000U)), %g1

The first line assembles properly, whereas the second line fails to assemble. It looks like this is a bug with the parser. I have only tested this with the sparc-leon3-linux target, but other targets may also be affected with the issue. Thank you for looking into this.

Cheers,
Orlando.
Comment 1 Nick Clifton 2016-04-05 15:50:40 UTC
Hi Orlando,

  This happens because the assembler does not support U as a suffix for integer constants.  Reduced test case:

  % cat fred.s
  sethi      %hi (((0x7FE3CU))), %fp
  sethi      %hi (((0x7FE3C))), %fp
  sethi      %hi (((0x7FE3CV))), %fp

  % as fred.s
fred.s:1: Error: missing ')'
fred.s:1: Error: missing ')'
fred.s:3: Error: missing ')'
fred.s:3: Error: missing ')'

  Obviously however the error message is bogus.  So I will look into that.

Cheers
  Nick
Comment 2 Orlando Arias 2016-04-05 16:04:39 UTC
(In reply to Nick Clifton from comment #1)
Greetings:

>   This happens because the assembler does not support U as a suffix for
> integer constants.

Is there any chance that support for the suffix could be added to gas? Or is there an already existing suffix which can be used with both GCC and GAS to the same effect? I came across this issue when porting U-Boot for the LEON3 to run on a Xilinx KC705 board.

The LEON3's main memory is mapped to start at 0x40000000 and the KC705 board provides 1GB of RAM. If leaving the constants as signed, computing the end of RAM an integer overflow occurs. GCC warns of this when the constants get expanded and non-working code is generated (U-Boot crashes on the board, have yet to debug to find out why, likely though because of the undefined behaviour generated by the overflow). I proceeded to add typing to the preprocessor directives and at that point the aforementioned error came up.

>   Obviously however the error message is bogus.  So I will look into that.

This should be fine as well. I can probably make two preprocessor directives, one to be used with the assembly sources and one to be used with the C sources. It is less than ideal, but it could work (although I am not sure as to how GAS would go on to handle the sum at that point).

Thank you for looking into this.

Cheers,
Orlando.
Comment 3 Nick Clifton 2016-04-05 16:13:33 UTC
Created attachment 9154 [details]
Proposed patch

Hi Orlando,

  Please try this patch, which should improve the error message somewhat.

Cheers
  Nick
Comment 4 Orlando Arias 2016-04-05 19:50:15 UTC
(In reply to Nick Clifton from comment #3)
> Created attachment 9154 [details]
> Proposed patch
> 
> Hi Orlando,
> 
>   Please try this patch, which should improve the error message somewhat.
> 
> Cheers
>   Nick

Greetings,

The patch works as expected, making the error much more clear.

$ cat assembler.S 
	sethi %hi(0x4000U), %g1
	or %g0, %lo(0x4000U + 0x4000U), %g1
	or %g0, %lo((0x4000U + 0x4000U)), %g1

$ sparc-leon3-linux-as -c assembler.S 
assembler.S: Assembler messages:
assembler.S:3: Error: found 'U', expected: ')'

However, if U as a suffix is not supported, the assembler should catch line 2 of the testcase as invalid as well. The assembler, however, parses it incorrectly:

$ cat assembler.S 
	sethi %hi(0x4000U), %g1
	or %g0, %lo(0x4000U + 0x4000U), %g1
$ sparc-leon3-linux-as -c assembler.S
$ sparc-leon3-linux-objdump -S a.out 

a.out:     file format elf32-sparc


Disassembly of section .text:

00000000 <.text>:
   0:	03 00 00 10 	sethi  %hi(0x4000), %g1
   4:	82 10 20 00 	clr  %g1

Manually expanding the pseudoinstruction, we see that it is actually doing:

or %g0, 0, %g1

instead of throwing an error. Naturally, the immediate constant is incorrectly computed. Thank you for looking into this.

Cheers,
Orlando.
Comment 5 gingold@adacore.com 2016-04-06 07:11:19 UTC
> On 05 Apr 2016, at 18:04, orlandoarias at gmail dot com <sourceware-bugzilla@sourceware.org> wrote:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=19910
> 
> --- Comment #2 from Orlando Arias <orlandoarias at gmail dot com> ---
> (In reply to Nick Clifton from comment #1)
> Greetings:
> 
>>  This happens because the assembler does not support U as a suffix for
>> integer constants.
> 
> Is there any chance that support for the suffix could be added to gas? Or is
> there an already existing suffix which can be used with both GCC and GAS to the
> same effect? I came across this issue when porting U-Boot for the LEON3 to run
> on a Xilinx KC705 board.

You could use the preprocessor to add an U when preprocessing for C (and not
add it when preprocessing for assembly).
Comment 6 Nick Clifton 2016-04-06 09:09:52 UTC
Hi Orlando,

> Is there any chance that support for the suffix could be added to gas?

Yes.  In fact it is quite simple.  The uploaded revised patch includes this change.


> However, if U as a suffix is not supported, the assembler should catch line
> 2 of the testcase as invalid as well. The assembler, however, parses it
> incorrectly:

This was a bug in the sparc backend of gas.  The revised patch also takes care of this problem.


Please try out the new patch and let me know if it works for you.

Cheers
  Nick
Comment 7 Nick Clifton 2016-04-06 09:10:19 UTC
Created attachment 9155 [details]
Revised patch
Comment 8 Nick Clifton 2016-04-06 09:33:37 UTC
Created attachment 9156 [details]
extended patch

For completeness sake, here is an extended version of the second patch which includes a couple of additions to the SPARC testsuite for gas.  Just to make sure that the patch continues to work.
Comment 9 Orlando Arias 2016-04-06 15:28:59 UTC
(In reply to Nick Clifton from comment #8)
> Created attachment 9156 [details]
> extended patch
> 
> For completeness sake, here is an extended version of the second patch which
> includes a couple of additions to the SPARC testsuite for gas.  Just to make
> sure that the patch continues to work.

Greetings,

This is excellent. I can confirm the patch works as expected. Thank you for your time.

Cheers,
Orlando.
Comment 10 cvs-commit@gcc.gnu.org 2016-04-07 11:35:37 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit e140100a5da85568e83ffe8e77d3f5e4a59ddee8
Author: Nick Clifton <nickc@redhat.com>
Date:   Thu Apr 7 12:34:06 2016 +0100

    Allow integer contants to have a U suffix.  Improve error reporting for missing closing parentheses.
    
    	PR gas/19910
    	* config/tc-sparc.c (sparc_ip): Report an error if the expression
    	inside a %-macro could not be fully parsed.
    	* expr.c (integer_constant): Accept and ignore U suffixes to
    	integers.
    	(operand): When a missing closing parenthesis is encountered,
    	report the character that was found instead.
    	* testsuite/gas/mips/tls-ill.l: Update expected error message.
    	* testsuite/gas/sparc/pr19910-1.d: New test driver.
    	* testsuite/gas/sparc/pr19910-1.s: New test.
    	* testsuite/gas/sparc/pr19910-2.l: Expected error output.
    	* testsuite/gas/sparc/pr19910-2.s: New test.
    	* testsuite/gas/sparc/sparc.exp: Run the new tests.
Comment 11 Nick Clifton 2016-04-07 11:38:26 UTC
Patch applied.
Comment 12 Alan Modra 2016-04-08 05:23:23 UTC
sparc-aout  +FAIL: PR19910 - make sure that U suffix is accepted
sparc-coff  +FAIL: PR19910 - make sure that U suffix is accepted

.../gas/sparc/pr19910-2.s: Assembler messages:
.../gas/sparc/pr19910-2.s:2: Error: Expression inside %hi could not be parsed
.../gas/sparc/pr19910-2.s:5: Error: found 'V', expected: ')'
.../gas/sparc/pr19910-2.s:5: Error: Expression inside %lo could not be parsed
Comment 13 cvs-commit@gcc.gnu.org 2016-04-08 09:38:02 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit 992dc2c47069220ce5a94829a8d8fed3ee72a1d0
Author: Nick Clifton <nickc@redhat.com>
Date:   Fri Apr 8 10:36:49 2016 +0100

    Fix regexps in PR19910 test to cope with COFF and AOUT sparc targets.
    
    	PR target/19910
    	* testsuite/gas/sparc/pr19910-1.d: Adjust regexps to work with
    	COFF and AOUT sparc targets.
Comment 14 Nick Clifton 2016-04-08 09:39:48 UTC
Hi Alan,

> sparc-aout  +FAIL: PR19910 - make sure that U suffix is accepted
> sparc-coff  +FAIL: PR19910 - make sure that U suffix is accepted

Are we still supporting those targets ?  I had no idea. :-)

Oh well, thanks for letting me know.  I have checked in a small update to the regexps in the pr19910 test so they pass now.

Cheers
  Nick