[PATCH] Don't define ARCH_cris for BFD64

[PATCH] Don't define ARCH_cris for BFD64

[Luis Machado]

I believe it is a mistake to define ARCH_cris when BFD64 is defined. It is
a 32-bit architecture, so should be placed outside of the BFD64 block.
---
 opcodes/disassemble.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/opcodes/disassemble.c b/opcodes/disassemble.c
index bd1b90b3956..5a472ce5349 100644
--- a/opcodes/disassemble.c
+++ b/opcodes/disassemble.c
@@ -28,7 +28,6 @@
 #define ARCH_aarch64
 #define ARCH_alpha
 #define ARCH_bpf
-#define ARCH_cris
 #define ARCH_ia64
 #define ARCH_loongarch
 #define ARCH_mips
@@ -43,6 +42,7 @@
 #define ARCH_avr
 #define ARCH_bfin
 #define ARCH_cr16
+#define ARCH_cris
 #define ARCH_crx
 #define ARCH_csky
 #define ARCH_d10v
-- 
2.25.1

Re: [PATCH] Don't define ARCH_cris for BFD64

[Alan Modra]

On Wed, May 04, 2022 at 08:56:28AM +0100, Luis Machado via Binutils wrote:
> I believe it is a mistake to define ARCH_cris when BFD64 is defined. It is
> a 32-bit architecture, so should be placed outside of the BFD64 block.

No.  cris may be 32-bit but the bfd support requires a 64-bit bfd.
See config.bfd.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Don't define ARCH_cris for BFD64

[Luis Machado]

On 5/4/22 09:08, Alan Modra wrote:
> On Wed, May 04, 2022 at 08:56:28AM +0100, Luis Machado via Binutils wrote:
>> I believe it is a mistake to define ARCH_cris when BFD64 is defined. It is
>> a 32-bit architecture, so should be placed outside of the BFD64 block.
> 
> No.  cris may be 32-bit but the bfd support requires a 64-bit bfd.
> See config.bfd.
> 

Interesting, I missed that. So I suppose GDB will need to move these 
cris files from ALL_TARGET_OBS to ALL_64_TARGET_OBS.

I see opcodes/Makefile.am also puts the cris files into 
TARGET32_LIBOPCODES_CFILES. Is that a mistake?

Re: [PATCH] Don't define ARCH_cris for BFD64

[Alan Modra]

On Wed, May 04, 2022 at 09:15:43AM +0100, Luis Machado wrote:
> On 5/4/22 09:08, Alan Modra wrote:
> > On Wed, May 04, 2022 at 08:56:28AM +0100, Luis Machado via Binutils wrote:
> > > I believe it is a mistake to define ARCH_cris when BFD64 is defined. It is
> > > a 32-bit architecture, so should be placed outside of the BFD64 block.
> > 
> > No.  cris may be 32-bit but the bfd support requires a 64-bit bfd.
> > See config.bfd.
> > 
> 
> Interesting, I missed that. So I suppose GDB will need to move these cris
> files from ALL_TARGET_OBS to ALL_64_TARGET_OBS.
> 
> I see opcodes/Makefile.am also puts the cris files into
> TARGET32_LIBOPCODES_CFILES. Is that a mistake?

Hmm, maybe config.bfd is wrong.  I see cris files in bfd/Makefile.am
BFD32_BACKENDS.  Hans-Peter, do you know what is going on here?

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Don't define ARCH_cris for BFD64

[Hans-Peter Nilsson]

> From: Alan Modra <amodra@gmail.com>
> Date: Wed, 4 May 2022 10:39:58 +0200

> On Wed, May 04, 2022 at 09:15:43AM +0100, Luis Machado wrote:
> > On 5/4/22 09:08, Alan Modra wrote:
> > > On Wed, May 04, 2022 at 08:56:28AM +0100, Luis Machado via Binutils wrote:
> > > > I believe it is a mistake to define ARCH_cris when BFD64 is defined. It is
> > > > a 32-bit architecture, so should be placed outside of the BFD64 block.
> > > 
> > > No.  cris may be 32-bit but the bfd support requires a 64-bit bfd.
> > > See config.bfd.
> > > 
> > 
> > Interesting, I missed that. So I suppose GDB will need to move these cris
> > files from ALL_TARGET_OBS to ALL_64_TARGET_OBS.
> > 
> > I see opcodes/Makefile.am also puts the cris files into
> > TARGET32_LIBOPCODES_CFILES. Is that a mistake?
> 
> Hmm, maybe config.bfd is wrong.  I see cris files in bfd/Makefile.am
> BFD32_BACKENDS.  Hans-Peter, do you know what is going on here?

I didn't remember that I had changed anything 32/64-related
in that area, so I had to consult git blame.  The most
recent change I did was in 2017, so whatever 32/64-changes
is probably related to 56fbd041853a "Fix gas/22304 by
forcing a 64-bit bfd for cris*-*" (TL;DR: expressions
overflow).  That commit *could* be slightly wrong; perhaps
it shouldn't be wrapped in #ifdef BFD64?

Adding CRIS files to BFD32_BACKENDS happened when the port
was added, back in 2000.  If it's wrong for a 32-bit target
to be there, then I don't want to be right! :)

brgds, H-P

Re: [PATCH] Don't define ARCH_cris for BFD64

[Alan Modra]

On Wed, May 04, 2022 at 04:37:03PM +0200, Hans-Peter Nilsson wrote:
> > From: Alan Modra <amodra@gmail.com>
> > Date: Wed, 4 May 2022 10:39:58 +0200
> 
> > On Wed, May 04, 2022 at 09:15:43AM +0100, Luis Machado wrote:
> > > On 5/4/22 09:08, Alan Modra wrote:
> > > > On Wed, May 04, 2022 at 08:56:28AM +0100, Luis Machado via Binutils wrote:
> > > > > I believe it is a mistake to define ARCH_cris when BFD64 is defined. It is
> > > > > a 32-bit architecture, so should be placed outside of the BFD64 block.
> > > > 
> > > > No.  cris may be 32-bit but the bfd support requires a 64-bit bfd.
> > > > See config.bfd.
> > > > 
> > > 
> > > Interesting, I missed that. So I suppose GDB will need to move these cris
> > > files from ALL_TARGET_OBS to ALL_64_TARGET_OBS.
> > > 
> > > I see opcodes/Makefile.am also puts the cris files into
> > > TARGET32_LIBOPCODES_CFILES. Is that a mistake?
> > 
> > Hmm, maybe config.bfd is wrong.  I see cris files in bfd/Makefile.am
> > BFD32_BACKENDS.  Hans-Peter, do you know what is going on here?
> 
> I didn't remember that I had changed anything 32/64-related
> in that area, so I had to consult git blame.  The most
> recent change I did was in 2017, so whatever 32/64-changes
> is probably related to 56fbd041853a "Fix gas/22304 by
> forcing a 64-bit bfd for cris*-*" (TL;DR: expressions
> overflow).  That commit *could* be slightly wrong; perhaps
> it shouldn't be wrapped in #ifdef BFD64?
> 
> Adding CRIS files to BFD32_BACKENDS happened when the port
> was added, back in 2000.  If it's wrong for a 32-bit target
> to be there, then I don't want to be right! :)

OK, so this means we have two slightly different versions of cris
support.  Configured to support cris directly with --target or
--enable-targets mentioning one of the cris tuples will always give
you a 64-bit bfd.  On a 32-bit host configured with
--enable-targets=all you'll get a 32-bit bfd, and miss some support
for explicitly choosing and displaying targets.  (The #ifdef comments
in config.bfd are used to generate targmatch.h, which gets included
into targets.c.)

I'm going to apply the following, and Luis, please apply your
disassemble.c change.

	* config.bfd (cris): Remove #idef BFD64.

diff --git a/bfd/config.bfd b/bfd/config.bfd
index 5a690742eb3..2a6aec28036 100644
--- a/bfd/config.bfd
+++ b/bfd/config.bfd
@@ -460,7 +460,6 @@ case "${targ}" in
     targ_underscore=yes
     ;;
 
-#ifdef BFD64
   cris-*-* | crisv32-*-*)
     targ_defvec=cris_aout_vec
     targ_selvecs="cris_elf32_us_vec cris_elf32_vec"
@@ -470,7 +469,6 @@ case "${targ}" in
     esac
     want64=true
     ;;
-#endif
 
   crx-*-elf*)
     targ_defvec=crx_elf32_vec


-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Don't define ARCH_cris for BFD64

[Luis Machado]

On 5/4/22 23:37, Alan Modra wrote:
> On Wed, May 04, 2022 at 04:37:03PM +0200, Hans-Peter Nilsson wrote:
>>> From: Alan Modra <amodra@gmail.com>
>>> Date: Wed, 4 May 2022 10:39:58 +0200
>>
>>> On Wed, May 04, 2022 at 09:15:43AM +0100, Luis Machado wrote:
>>>> On 5/4/22 09:08, Alan Modra wrote:
>>>>> On Wed, May 04, 2022 at 08:56:28AM +0100, Luis Machado via Binutils wrote:
>>>>>> I believe it is a mistake to define ARCH_cris when BFD64 is defined. It is
>>>>>> a 32-bit architecture, so should be placed outside of the BFD64 block.
>>>>>
>>>>> No.  cris may be 32-bit but the bfd support requires a 64-bit bfd.
>>>>> See config.bfd.
>>>>>
>>>>
>>>> Interesting, I missed that. So I suppose GDB will need to move these cris
>>>> files from ALL_TARGET_OBS to ALL_64_TARGET_OBS.
>>>>
>>>> I see opcodes/Makefile.am also puts the cris files into
>>>> TARGET32_LIBOPCODES_CFILES. Is that a mistake?
>>>
>>> Hmm, maybe config.bfd is wrong.  I see cris files in bfd/Makefile.am
>>> BFD32_BACKENDS.  Hans-Peter, do you know what is going on here?
>>
>> I didn't remember that I had changed anything 32/64-related
>> in that area, so I had to consult git blame.  The most
>> recent change I did was in 2017, so whatever 32/64-changes
>> is probably related to 56fbd041853a "Fix gas/22304 by
>> forcing a 64-bit bfd for cris*-*" (TL;DR: expressions
>> overflow).  That commit *could* be slightly wrong; perhaps
>> it shouldn't be wrapped in #ifdef BFD64?
>>
>> Adding CRIS files to BFD32_BACKENDS happened when the port
>> was added, back in 2000.  If it's wrong for a 32-bit target
>> to be there, then I don't want to be right! :)
> 
> OK, so this means we have two slightly different versions of cris
> support.  Configured to support cris directly with --target or
> --enable-targets mentioning one of the cris tuples will always give
> you a 64-bit bfd.  On a 32-bit host configured with
> --enable-targets=all you'll get a 32-bit bfd, and miss some support
> for explicitly choosing and displaying targets.  (The #ifdef comments
> in config.bfd are used to generate targmatch.h, which gets included
> into targets.c.)
> 
> I'm going to apply the following, and Luis, please apply your
> disassemble.c change.

Pushed now. Thanks!

Re: [PATCH] Don't define ARCH_cris for BFD64

[Pedro Alves]

On 2022-05-04 23:37, Alan Modra via Binutils wrote:

> OK, so this means we have two slightly different versions of cris
> support.  Configured to support cris directly with --target or
> --enable-targets mentioning one of the cris tuples will always give
> you a 64-bit bfd.  On a 32-bit host configured with
> --enable-targets=all you'll get a 32-bit bfd, and miss some support
> for explicitly choosing and displaying targets.  (The #ifdef comments
> in config.bfd are used to generate targmatch.h, which gets included
> into targets.c.)

I guess I don't understand the logic fully, and if you have the patience for me, I'd like
to understand it better.

In the --enable-targets=all case with a 32-bit bfd, it seems counterintuitive to want to be able to choose
and display cris, if its bfd supposedly wants 64-bit, given want64=true.  If bfd says cris wants
64-bit bfd, and you have a 32-bit bfd, it seems weird to build it in 32-bit mode, and let users choose and
use it?  

I just tried a 32-bit --enable-targets=all --disable-sim build of current master, and indeed we end
up with these files built:

 $ ls bfd/*cris*
 bfd/aout-cris.lo  bfd/aout-cris.o  bfd/cpu-cris.lo  bfd/cpu-cris.o  bfd/elf32-cris.lo  bfd/elf32-cris.o

however, those are built with a 32-bit bfd ("#define BFD_ARCH_SIZE 32" in bfd/bfd.h), which if we
trust win64=true (and 56fbd041853a "Fix gas/22304 by forcing a 64-bit bfd for cris*-*") is not
supposed to happen.

Currently, gas --enable-targets=all isn't very useful, as you can't make it assemble
for non-primary targets AFAIK, but if it did, then a 32-bit --enable-targets=all build would
assemble cris using 32-bit bfd, which I assume would be affected by gas/22304 again.

Unlike gas however, a 32-bit --target-targets=all gdb knows how to cross-debug cris and will happily
do it, again with a 32-bit bfd.  Maybe the gas issue that led to making cris want 64-bit bfd doesn't
affect any other tool, so meh, who cares, right?, but it still seems counterintuitive/strange
to have tool dependencies or mismatches like that.

Shouldn't instead BFD64 / want64 / TARGET64_LIBOPCODES_CFILES all agree in principle
so we avoid these tool mismatches, where a port says it wants 64-bit bfd but then it
can happen to be built and be used with 32-bit anyhow?

Sometimes I wonder whether 32-bit bfd_vma is still worth it, though.
FWIW, GDB always has 64-bit CORE_ADDR nowadays, even on 32-bit hosts.

Anyhow, I got confused trying to figure out how all this is supposed to work, and I realize
that the above may sound like a rant, though it was not intended, so sorry if it does.
I guess other people who read this code for the first time may end up scratching their
heads too.

Re: [PATCH] Don't define ARCH_cris for BFD64

[Hans-Peter Nilsson]

> From: Pedro Alves <pedro@palves.net>
> Date: Thu, 5 May 2022 13:11:29 +0200

> On 2022-05-04 23:37, Alan Modra via Binutils wrote:
> 
> > OK, so this means we have two slightly different versions of cris
> > support.  Configured to support cris directly with --target or
> > --enable-targets mentioning one of the cris tuples will always give
> > you a 64-bit bfd.  On a 32-bit host configured with
> > --enable-targets=all you'll get a 32-bit bfd, and miss some support
> > for explicitly choosing and displaying targets.  (The #ifdef comments
> > in config.bfd are used to generate targmatch.h, which gets included
> > into targets.c.)
> 
> I guess I don't understand the logic fully, and if you have the patience for me, I'd like
> to understand it better.
> 
> In the --enable-targets=all case with a 32-bit bfd, it seems counterintuitive to want to be able to choose
> and display cris, if its bfd supposedly wants 64-bit, given want64=true.  If bfd says cris wants
> 64-bit bfd, and you have a 32-bit bfd, it seems weird to build it in 32-bit mode, and let users choose and
> use it?  
> 
> I just tried a 32-bit --enable-targets=all --disable-sim build of current master, and indeed we end
> up with these files built:
> 
>  $ ls bfd/*cris*
>  bfd/aout-cris.lo  bfd/aout-cris.o  bfd/cpu-cris.lo  bfd/cpu-cris.o  bfd/elf32-cris.lo  bfd/elf32-cris.o
> 
> however, those are built with a 32-bit bfd ("#define BFD_ARCH_SIZE 32" in bfd/bfd.h), which if we
> trust win64=true (and 56fbd041853a "Fix gas/22304 by forcing a 64-bit bfd for cris*-*") is not
        ^^^^^
There's your problem! :)
(want64=true)

> supposed to happen.

Right.  I somehow thought with --enable-targets=all we'd get
a 64-bit bfd_vma but sure, then we'd not be discussing this.

But I agree we should make that change.

Host systems (with a compiler) without a 64-bit type these
days seem IMHO unlikely to be suitable as hosts, at least
not for --enable-targets=all and newer binutils (+gdb+sim).

brgds, H-P

Re: [PATCH] Don't define ARCH_cris for BFD64

[Alan Modra]

On Thu, May 05, 2022 at 12:11:29PM +0100, Pedro Alves wrote:
> On 2022-05-04 23:37, Alan Modra via Binutils wrote:
> 
> > OK, so this means we have two slightly different versions of cris
> > support.  Configured to support cris directly with --target or
> > --enable-targets mentioning one of the cris tuples will always give
> > you a 64-bit bfd.  On a 32-bit host configured with
> > --enable-targets=all you'll get a 32-bit bfd, and miss some support
> > for explicitly choosing and displaying targets.  (The #ifdef comments
> > in config.bfd are used to generate targmatch.h, which gets included
> > into targets.c.)
> 
> I guess I don't understand the logic fully, and if you have the patience for me, I'd like
> to understand it better.
> 
> In the --enable-targets=all case with a 32-bit bfd, it seems counterintuitive to want to be able to choose
> and display cris, if its bfd supposedly wants 64-bit, given want64=true.  If bfd says cris wants
> 64-bit bfd, and you have a 32-bit bfd, it seems weird to build it in 32-bit mode, and let users choose and
> use it?  

As is often the case the truth is more complicated than it would
appear on the surface.  I think the cris support works fine with a
32-bit bfd.  It's just that the cris testsuite has expressions
that overflow 32 bits.

gas/testsuite/gas/cris/shexpr-1.s has this expression:

    ((0x17<<23)+((0xfede4194/8192)<<4)+8)

It evaluates to 0x0bff6f28 when calculating in 64 bits, but to
0x0b7f6f38 when calculating in 32 bits, because 0xfede4194 is
−0x121be6c as a signed 32-bit number and the division is done using
signed bfd_vma values.

gas/testsuite/gas/cris/range-err-1.s has three uses of 0xffffffff that
don't give the expected error when used in 8-bit and 16-bit fields if
bfd_vma is 32 bits.  That's because 0xffffffff is -1, and apparently
-1 is OK for those fields.

No doubt gas expression evaluation could be improved.  eg. We could
use C rules for whether constants are signed or unsigned, and track
that through expression evaluation.  The thing that give me pause in
recommending that (or doing it myself) is that I suspect it might open
up a can of worms assembling 64-bit target code, discovering lots of
places where compiler output or user assembly isn't clean.  A simple
example is that 0xffffffffffffffff would no longer be equal to -1, and
therefore might trigger field overflow errors.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Don't define ARCH_cris for BFD64

[Hans-Peter Nilsson]

> From: Alan Modra <amodra@gmail.com>
> Date: Fri, 6 May 2022 02:56:46 +0200

> As is often the case the truth is more complicated than it would
> appear on the surface.

While aiming for the bigger picture is a good thing, you're
here letting the perfect be the enemy of the good.  A
reasonable step would be to get all targets want64=true, to
avoid this annoying host-32/64-bit difference.

>  I think the cris support works fine with a
> 32-bit bfd.  It's just that the cris testsuite has expressions
> that overflow 32 bits.
> 
> gas/testsuite/gas/cris/shexpr-1.s has this expression:
> 
>     ((0x17<<23)+((0xfede4194/8192)<<4)+8)
>
> It evaluates to 0x0bff6f28 when calculating in 64 bits, but to
> 0x0b7f6f38 when calculating in 32 bits

But seeing it as "calculating in 32 bits" is just So Wrong!

I admit I worked around this missing tracking of
subexpression signeness and papering over the gas bug by
just making the problem go away for *this* port.

Now 20 years down the line, when 64-bit is the norm, IMHO
that's a preferred solution (while waiting for Someone to
rewrite the expression bits, but it also happens to get
host-size-uniform address output).

brgds, H-P

Re: [PATCH] Don't define ARCH_cris for BFD64

[Luis Machado]

On 5/6/22 03:35, Hans-Peter Nilsson via Binutils wrote:
>> From: Alan Modra <amodra@gmail.com>
>> Date: Fri, 6 May 2022 02:56:46 +0200
> 
>> As is often the case the truth is more complicated than it would
>> appear on the surface.
> 
> While aiming for the bigger picture is a good thing, you're
> here letting the perfect be the enemy of the good.  A
> reasonable step would be to get all targets want64=true, to
> avoid this annoying host-32/64-bit difference.
> 
>>   I think the cris support works fine with a
>> 32-bit bfd.  It's just that the cris testsuite has expressions
>> that overflow 32 bits.
>>
>> gas/testsuite/gas/cris/shexpr-1.s has this expression:
>>
>>      ((0x17<<23)+((0xfede4194/8192)<<4)+8)
>>
>> It evaluates to 0x0bff6f28 when calculating in 64 bits, but to
>> 0x0b7f6f38 when calculating in 32 bits
> 
> But seeing it as "calculating in 32 bits" is just So Wrong!
> 
> I admit I worked around this missing tracking of
> subexpression signeness and papering over the gas bug by
> just making the problem go away for *this* port.
> 
> Now 20 years down the line, when 64-bit is the norm, IMHO
> that's a preferred solution (while waiting for Someone to
> rewrite the expression bits, but it also happens to get
> host-size-uniform address output).
> 
> brgds, H-P

It does seem like trying to adjust code like this is a bit dangerous and 
may lead to some breakage that is hard to exercise.

 From a "--enable-targets=all" and GDB build's perspective, making these 
targets use a 64-bit BFD and defining disassembler functions 
conditionally according to the presence of a 32-bit BFD or 64-bit BFD 
causes some issues.

I think most of the problem lies in the fact that sim/ wants to build 
everything (all sims, for all targets), regardless of whether we're 
using a 32-bit BFD or 64-bit BFD. And that leads to undesirable build 
breakages for 32-bit builds.

If a disassembler function is not defined properly for GDB, then that 
also leads to internal errors in GDB's testsuite, as the architecture 
will be visible to GDB, and it will cycle through all of the available 
architectures doing multiple checks.

I'd expect at least a clean 32-bit build to go through, and that has 
been my goal so far. And I did notice some files were placed incorrectly 
(ones using 64-bit BFD's in the 32-bit BFD list or the other way 
around). Then again, it seems the current mechanism is to keep 
everything in sync by hand, which is a bit fragile.

32-bit archs, want64=true, and gas integers (Re: [PATCH] Don't define ARCH_cris for BFD64)

[Pedro Alves]

Hi Alan,

On 2022-05-06 01:56, Alan Modra wrote:
> On Thu, May 05, 2022 at 12:11:29PM +0100, Pedro Alves wrote:
>> On 2022-05-04 23:37, Alan Modra via Binutils wrote:
>>
>>> OK, so this means we have two slightly different versions of cris
>>> support.  Configured to support cris directly with --target or
>>> --enable-targets mentioning one of the cris tuples will always give
>>> you a 64-bit bfd.  On a 32-bit host configured with
>>> --enable-targets=all you'll get a 32-bit bfd, and miss some support
>>> for explicitly choosing and displaying targets.  (The #ifdef comments
>>> in config.bfd are used to generate targmatch.h, which gets included
>>> into targets.c.)
>>
>> I guess I don't understand the logic fully, and if you have the patience for me, I'd like
>> to understand it better.
>>
>> In the --enable-targets=all case with a 32-bit bfd, it seems counterintuitive to want to be able to choose
>> and display cris, if its bfd supposedly wants 64-bit, given want64=true.  If bfd says cris wants
>> 64-bit bfd, and you have a 32-bit bfd, it seems weird to build it in 32-bit mode, and let users choose and
>> use it?  
> 
> As is often the case the truth is more complicated than it would
> appear on the surface.  I think the cris support works fine with a
> 32-bit bfd.  It's just that the cris testsuite has expressions
> that overflow 32 bits.

I see.  It wasn't just cris that contributed to my head scratching.
We have these other cases like:


  powerpc-*-aix5.[01] | rs6000-*-aix5.[01])
    targ_defvec=rs6000_xcoff_vec
    targ_selvecs="rs6000_xcoff64_aix_vec"
    want64=true
    ;;

...

  powerpc-*-aix[5-9]* | rs6000-*-aix[5-9]*)
    targ_cflags=-DAIX_WEAK_SUPPORT
    targ_defvec=rs6000_xcoff_vec
    targ_selvecs="rs6000_xcoff64_aix_vec"
    want64=true
    ;;

...

  spu-*-elf)
    targ_defvec=spu_elf32_vec
    want64=true
    ;;

that say they want 64-bit bfd, but aren't wrapped in BFD64, nor do they
have a targ64_selvecs.  Makes me wonder whether they do that for a similar
reason (gas), or whether they really need 64-bit vmas.  It'd be great if these
details / odd requirements were explained in comments so that people didn't have
dig this stuff up.

> 
> gas/testsuite/gas/cris/shexpr-1.s has this expression:
> 
>     ((0x17<<23)+((0xfede4194/8192)<<4)+8)
> 
> It evaluates to 0x0bff6f28 when calculating in 64 bits, but to
> 0x0b7f6f38 when calculating in 32 bits, because 0xfede4194 is
> −0x121be6c as a signed 32-bit number and the division is done using
> signed bfd_vma values.
> 
> gas/testsuite/gas/cris/range-err-1.s has three uses of 0xffffffff that
> don't give the expected error when used in 8-bit and 16-bit fields if
> bfd_vma is 32 bits.  That's because 0xffffffff is -1, and apparently
> -1 is OK for those fields.
> 
> No doubt gas expression evaluation could be improved.  eg. We could
> use C rules for whether constants are signed or unsigned, and track
> that through expression evaluation.  The thing that give me pause in
> recommending that (or doing it myself) is that I suspect it might open
> up a can of worms assembling 64-bit target code, discovering lots of
> places where compiler output or user assembly isn't clean.  A simple
> example is that 0xffffffffffffffff would no longer be equal to -1, and
> therefore might trigger field overflow errors.
> 

Looking at the "Numbers" chapter in the manual:

 https://sourceware.org/binutils/docs/as/Numbers.html

we have:

 "Integers are numbers that would fit into an int in the C language. Bignums are integers, but they are stored in more than 32 bits."

I note it says "more than 32 bits", not "32 bits or more".

By my reading of the "Integers" and "Bignums" subsections, these integers are always signed.  Maybe
that should be explicitly said in the manual, instead of introducing signed vs unsigned numbers?

And then, couldn't we make gas use int32_t for integers, and int64_t for Bignums (and clarify in the manual
that "more than 32 bits" is exactly "64 bits"?  IOW, use int64_t instead of bfd_vma in the
expression evaluation stuff.  

That way, gas would be following what it documents, and 32-bit or 64-bit bfd would make no difference,
and we'd get rid of a subtle host / --enable-64-bit-bfd -dependent behavior change for 32-bit ports.

Pedro Alves

Re: 32-bit archs, want64=true, and gas integers

[Jan Beulich]

On 06.05.2022 11:00, Pedro Alves wrote:
> Looking at the "Numbers" chapter in the manual:
> 
>  https://sourceware.org/binutils/docs/as/Numbers.html
> 
> we have:
> 
>  "Integers are numbers that would fit into an int in the C language. Bignums are integers, but they are stored in more than 32 bits."
> 
> I note it says "more than 32 bits", not "32 bits or more".
> 
> By my reading of the "Integers" and "Bignums" subsections, these integers are always signed.  Maybe
> that should be explicitly said in the manual, instead of introducing signed vs unsigned numbers?
> 
> And then, couldn't we make gas use int32_t for integers, and int64_t for Bignums (and clarify in the manual
> that "more than 32 bits" is exactly "64 bits"?  IOW, use int64_t instead of bfd_vma in the
> expression evaluation stuff.  

Bignums are quite a bit wider than 64 bits, and on 64-bit architectures
using just int32_t for integers is definitely insufficient (there are
pretty limited operations one can do on bignums). I'm afraid the doc is
simply outdated in talking about "int in C language"; it's more like
"long", and even then only in Unix-like environments, so it would likely
be even better to talk about {,u}intptr_t.

Jan

> That way, gas would be following what it documents, and 32-bit or 64-bit bfd would make no difference,
> and we'd get rid of a subtle host / --enable-64-bit-bfd -dependent behavior change for 32-bit ports.
> 
> Pedro Alves
> 

Re: 32-bit archs, want64=true, and gas integers

[Pedro Alves]

On 2022-05-06 10:55, Jan Beulich wrote:
> On 06.05.2022 11:00, Pedro Alves wrote:
>> Looking at the "Numbers" chapter in the manual:
>>
>>  https://sourceware.org/binutils/docs/as/Numbers.html
>>
>> we have:
>>
>>  "Integers are numbers that would fit into an int in the C language. Bignums are integers, but they are stored in more than 32 bits."
>>
>> I note it says "more than 32 bits", not "32 bits or more".
>>
>> By my reading of the "Integers" and "Bignums" subsections, these integers are always signed.  Maybe
>> that should be explicitly said in the manual, instead of introducing signed vs unsigned numbers?
>>
>> And then, couldn't we make gas use int32_t for integers, and int64_t for Bignums (and clarify in the manual
>> that "more than 32 bits" is exactly "64 bits"?  IOW, use int64_t instead of bfd_vma in the
>> expression evaluation stuff.  
> 
> Bignums are quite a bit wider than 64 bits, and on 64-bit architectures
> using just int32_t for integers is definitely insufficient (there are
> pretty limited operations one can do on bignums). I'm afraid the doc is
> simply outdated in talking about "int in C language"; it's more like
> "long", and even then only in Unix-like environments, so it would likely
> be even better to talk about {,u}intptr_t.

But why {,u}intptr_t?  Why does it have to be the size of a pointer, which 
raises the question of -- which pointer, host or target?  And if you support
multiple targets, which target?

Couldn't gas always use int64_t for integers instead?  Effectively it would
be the same as if you always built gas with --enable-64-bit-bfd, except it
wouldn't force 64-bit bfd_vma.

Pedro Alves

> 
> Jan
> 
>> That way, gas would be following what it documents, and 32-bit or 64-bit bfd would make no difference,
>> and we'd get rid of a subtle host / --enable-64-bit-bfd -dependent behavior change for 32-bit ports.
>>
>> Pedro Alves
>>
> 

Re: 32-bit archs, want64=true, and gas integers

[Jan Beulich]

On 06.05.2022 12:01, Pedro Alves wrote:
> On 2022-05-06 10:55, Jan Beulich wrote:
>> On 06.05.2022 11:00, Pedro Alves wrote:
>>> Looking at the "Numbers" chapter in the manual:
>>>
>>>  https://sourceware.org/binutils/docs/as/Numbers.html
>>>
>>> we have:
>>>
>>>  "Integers are numbers that would fit into an int in the C language. Bignums are integers, but they are stored in more than 32 bits."
>>>
>>> I note it says "more than 32 bits", not "32 bits or more".
>>>
>>> By my reading of the "Integers" and "Bignums" subsections, these integers are always signed.  Maybe
>>> that should be explicitly said in the manual, instead of introducing signed vs unsigned numbers?
>>>
>>> And then, couldn't we make gas use int32_t for integers, and int64_t for Bignums (and clarify in the manual
>>> that "more than 32 bits" is exactly "64 bits"?  IOW, use int64_t instead of bfd_vma in the
>>> expression evaluation stuff.  
>>
>> Bignums are quite a bit wider than 64 bits, and on 64-bit architectures
>> using just int32_t for integers is definitely insufficient (there are
>> pretty limited operations one can do on bignums). I'm afraid the doc is
>> simply outdated in talking about "int in C language"; it's more like
>> "long", and even then only in Unix-like environments, so it would likely
>> be even better to talk about {,u}intptr_t.
> 
> But why {,u}intptr_t?

Because it's used for address calculations.

>  Why does it have to be the size of a pointer, which 
> raises the question of -- which pointer, host or target?

Target.

>  And if you support
> multiple targets, which target?

That of the biggest target. But that's no an issue in e.g. gas (and
hence with expression evaluation), which only supports a single target
(but perhaps multiple sub-targets) at a time. Sub-targets with more
narrow address size indeed aren't always properly handled.

> Couldn't gas always use int64_t for integers instead?

This might be possible. Once a 128-bit arch appears it'll end up
outdated again, though.

Jan

Re: 32-bit archs, want64=true, and gas integers

[Pedro Alves]

On 2022-05-06 11:17, Jan Beulich wrote:
> On 06.05.2022 12:01, Pedro Alves wrote:
>> On 2022-05-06 10:55, Jan Beulich wrote:
>>> On 06.05.2022 11:00, Pedro Alves wrote:
>>>> Looking at the "Numbers" chapter in the manual:
>>>>
>>>>  https://sourceware.org/binutils/docs/as/Numbers.html
>>>>
>>>> we have:
>>>>
>>>>  "Integers are numbers that would fit into an int in the C language. Bignums are integers, but they are stored in more than 32 bits."
>>>>
>>>> I note it says "more than 32 bits", not "32 bits or more".
>>>>
>>>> By my reading of the "Integers" and "Bignums" subsections, these integers are always signed.  Maybe
>>>> that should be explicitly said in the manual, instead of introducing signed vs unsigned numbers?
>>>>
>>>> And then, couldn't we make gas use int32_t for integers, and int64_t for Bignums (and clarify in the manual
>>>> that "more than 32 bits" is exactly "64 bits"?  IOW, use int64_t instead of bfd_vma in the
>>>> expression evaluation stuff.  
>>>
>>> Bignums are quite a bit wider than 64 bits, and on 64-bit architectures
>>> using just int32_t for integers is definitely insufficient (there are
>>> pretty limited operations one can do on bignums). I'm afraid the doc is
>>> simply outdated in talking about "int in C language"; it's more like
>>> "long", and even then only in Unix-like environments, so it would likely
>>> be even better to talk about {,u}intptr_t.
>>
>> But why {,u}intptr_t?
> 
> Because it's used for address calculations.
> 
>>  Why does it have to be the size of a pointer, which 
>> raises the question of -- which pointer, host or target?
> 
> Target.
> 
>>  And if you support
>> multiple targets, which target?
> 
> That of the biggest target. But that's no an issue in e.g. gas (and
> hence with expression evaluation), which only supports a single target
> (but perhaps multiple sub-targets) at a time. Sub-targets with more
> narrow address size indeed aren't always properly handled.

Sure, but a gas implementation detail is leaking into bfd, and contributing to
a 32-bit vs 64-bit mess in multiple tools that do support multiple targets properly,
which is what I'm trying to see if we could decouple.

> 
>> Couldn't gas always use int64_t for integers instead?
> 
> This might be possible. Once a 128-bit arch appears it'll end up
> outdated again, though.

I've never looked at gas code before, but I found the the code that parses integers,
and it seems to automatically handle integers larger than 64-bit, by promoting to bignum.
Maybe the ideal or the original intent was for integers to behave as as they had unlimited
precision, with non-bignum integers being an optimization?  And then make sure that all
operations, including division would handle bignums too?

Anyhow, here's a tentative patch of what I was imagining.  I'm not set up for testing
this properly.  I was hoping one of you guys would like the idea so much that
you'd run with it.  :-)

If something like this went in, then I assume that cris could go back to not setting
want64=yes.

From 7fbbecc9eacd7e5dac076bfc25b099b9ff0ca1ac Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 6 May 2022 11:15:11 +0100
Subject: [PATCH] gas: make non-bignum integers be always 64-bit

Currently, on 32-bit hosts, gas integers are either 32-bit or 64-bit,
depending on --enable-64-bit-bfd.  Make valueT be always uint64_t to
make gas behave the same on all hosts.

Change-Id: I6258dfcd975ea9abb9cc4eb1d4e604a077df3033
---
 gas/as.h   |  2 +-
 gas/expr.c | 65 ++++++++++++++----------------------------------------
 gas/expr.h |  1 -
 3 files changed, 18 insertions(+), 50 deletions(-)

diff --git a/gas/as.h b/gas/as.h
index 135abc8f23d..5b636cfd59a 100644
--- a/gas/as.h
+++ b/gas/as.h
@@ -139,7 +139,7 @@ typedef bfd_vma addressT;
 typedef bfd_signed_vma offsetT;
 
 /* Type of symbol value, etc.  For use in prototypes.  */
-typedef addressT valueT;
+typedef uint64_t valueT;
 
 #ifndef COMMON
 #ifdef TEST
diff --git a/gas/expr.c b/gas/expr.c
index 6ad8bee2733..5dce12fe99d 100644
--- a/gas/expr.c
+++ b/gas/expr.c
@@ -255,14 +255,6 @@ floating_constant (expressionS *expressionP)
   expressionP->X_add_number = -1;
 }
 
-uint32_t
-generic_bignum_to_int32 (void)
-{
-  return ((((uint32_t) generic_bignum[1] & LITTLENUM_MASK)
-	   << LITTLENUM_NUMBER_OF_BITS)
-	  | ((uint32_t) generic_bignum[0] & LITTLENUM_MASK));
-}
-
 uint64_t
 generic_bignum_to_int64 (void)
 {
@@ -288,31 +280,24 @@ integer_constant (int radix, expressionS *expressionP)
   char *name;		/* Points to name of symbol.  */
   symbolS *symbolP;	/* Points to symbol.  */
 
-  int small;			/* True if fits in 32 bits.  */
+  bool small;			/* True if fits in 64 bits.  */
 
-  /* May be bignum, or may fit in 32 bits.  */
-  /* Most numbers fit into 32 bits, and we want this case to be fast.
-     so we pretend it will fit into 32 bits.  If, after making up a 32
+  /* May be bignum, or may fit in 64 bits.  */
+  /* Most numbers fit into 64 bits, and we want this case to be fast.
+     so we pretend it will fit into 64 bits.  If, after making up a 64
      bit number, we realise that we have scanned more digits than
-     comfortably fit into 32 bits, we re-scan the digits coding them
+     comfortably fit into 64 bits, we re-scan the digits coding them
      into a bignum.  For decimal and octal numbers we are
      conservative: Some numbers may be assumed bignums when in fact
-     they do fit into 32 bits.  Numbers of any radix can have excess
+     they do fit into 64 bits.  Numbers of any radix can have excess
      leading zeros: We strive to recognise this and cast them back
      into 32 bits.  We must check that the bignum really is more than
-     32 bits, and change it back to a 32-bit number if it fits.  The
+     64 bits, and change it back to a 64-bit number if it fits.  The
      number we are looking for is expected to be positive, but if it
-     fits into 32 bits as an unsigned number, we let it be a 32-bit
+     fits into 64 bits as an unsigned number, we let it be a 64-bit
      number.  The cavalier approach is for speed in ordinary cases.  */
-  /* This has been extended for 64 bits.  We blindly assume that if
-     you're compiling in 64-bit mode, the target is a 64-bit machine.
-     This should be cleaned up.  */
-
-#ifdef BFD64
-#define valuesize 64
-#else /* includes non-bfd case, mostly */
-#define valuesize 32
-#endif
+
+#define valuesize (sizeof (valueT) * CHAR_BIT)
 
   if (is_end_of_line[(unsigned char) *input_line_pointer])
     {
@@ -384,7 +369,6 @@ integer_constant (int radix, expressionS *expressionP)
       maxdig = radix = 10;
       too_many_digits = (valuesize + 11) / 4; /* Very rough.  */
     }
-#undef valuesize
   start = input_line_pointer;
   c = *input_line_pointer++;
   for (number = 0;
@@ -455,23 +439,15 @@ integer_constant (int radix, expressionS *expressionP)
 	     && num_little_digits > 1)
 	num_little_digits--;
 
-      if (num_little_digits <= 2)
-	{
-	  /* will fit into 32 bits.  */
-	  number = generic_bignum_to_int32 ();
-	  small = 1;
-	}
-#ifdef BFD64
-      else if (num_little_digits <= 4)
+      if (num_little_digits <= 4)
 	{
 	  /* Will fit into 64 bits.  */
 	  number = generic_bignum_to_int64 ();
-	  small = 1;
+	  small = true;
 	}
-#endif
       else
 	{
-	  small = 0;
+	  small = false;
 
 	  /* Number of littlenums in the bignum.  */
 	  number = num_little_digits;
@@ -513,20 +489,12 @@ integer_constant (int radix, expressionS *expressionP)
       /* Again, c is char after number.  */
       /* input_line_pointer -> after c.  */
       know (LITTLENUM_NUMBER_OF_BITS == 16);
-      if (leader < generic_bignum + 2)
-	{
-	  /* Will fit into 32 bits.  */
-	  number = generic_bignum_to_int32 ();
-	  small = 1;
-	}
-#ifdef BFD64
-      else if (leader < generic_bignum + 4)
+      if (leader < generic_bignum + 4)
 	{
 	  /* Will fit into 64 bits.  */
 	  number = generic_bignum_to_int64 ();
 	  small = 1;
 	}
-#endif
       else
 	{
 	  /* Number of littlenums in the bignum.  */
@@ -1947,14 +1915,15 @@ expr (int rankarg,		/* Larger # is higher rank.  */
 	      as_warn (_("division by zero"));
 	      v = 1;
 	    }
-	  if ((valueT) v >= sizeof(valueT) * CHAR_BIT
+	  if ((valueT) v >= valuesize
 	      && (op_left == O_left_shift || op_left == O_right_shift))
 	    {
 	      as_warn_value_out_of_range (_("shift count"), v, 0,
-					  sizeof(valueT) * CHAR_BIT - 1,
+					  valuesize - 1,
 					  NULL, 0);
 	      resultP->X_add_number = v = 0;
 	    }
+#undef valuesize
 	  switch (op_left)
 	    {
 	    default:			goto general;
diff --git a/gas/expr.h b/gas/expr.h
index dff40857427..766c70146e3 100644
--- a/gas/expr.h
+++ b/gas/expr.h
@@ -186,7 +186,6 @@ extern int expr_symbol_where (symbolS *, const char **, unsigned int *);
 extern void current_location (expressionS *);
 extern symbolS *expr_build_uconstant (offsetT);
 extern symbolS *expr_build_dot (void);
-extern uint32_t generic_bignum_to_int32 (void);
 extern uint64_t generic_bignum_to_int64 (void);
 extern int resolve_expression (expressionS *);
 

base-commit: 0ee8858e7aeca5ba5f702204daad2ddd290ef229
-- 
2.36.0

Re: 32-bit archs, want64=true, and gas integers

[Jan Beulich]

On 06.05.2022 12:43, Pedro Alves wrote:
> I've never looked at gas code before, but I found the the code that parses integers,
> and it seems to automatically handle integers larger than 64-bit, by promoting to bignum.
> Maybe the ideal or the original intent was for integers to behave as as they had unlimited
> precision,

Well, not unlimited, but far higher.

> with non-bignum integers being an optimization?  And then make sure that all
> operations, including division would handle bignums too?

Don't know. To me the limitations of bignum don't look as if that might
have been the plan.

> Anyhow, here's a tentative patch of what I was imagining.  I'm not set up for testing
> this properly.  I was hoping one of you guys would like the idea so much that
> you'd run with it.  :-)

It look plausible at the first glance, but ...

> If something like this went in, then I assume that cris could go back to not setting
> want64=yes.
> 
> From 7fbbecc9eacd7e5dac076bfc25b099b9ff0ca1ac Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Fri, 6 May 2022 11:15:11 +0100
> Subject: [PATCH] gas: make non-bignum integers be always 64-bit
> 
> Currently, on 32-bit hosts, gas integers are either 32-bit or 64-bit,
> depending on --enable-64-bit-bfd.  Make valueT be always uint64_t to
> make gas behave the same on all hosts.
> 
> Change-Id: I6258dfcd975ea9abb9cc4eb1d4e604a077df3033
> ---
>  gas/as.h   |  2 +-
>  gas/expr.c | 65 ++++++++++++++----------------------------------------
>  gas/expr.h |  1 -
>  3 files changed, 18 insertions(+), 50 deletions(-)
> 
> diff --git a/gas/as.h b/gas/as.h
> index 135abc8f23d..5b636cfd59a 100644
> --- a/gas/as.h
> +++ b/gas/as.h
> @@ -139,7 +139,7 @@ typedef bfd_vma addressT;
>  typedef bfd_signed_vma offsetT;
>  
>  /* Type of symbol value, etc.  For use in prototypes.  */
> -typedef addressT valueT;
> +typedef uint64_t valueT;

... I'd be afraid this might alter behavior for purely 32-bit targets.
Whether such a behavioral change would be deemed okay I'm not sure.

Jan

Re: 32-bit archs, want64=true, and gas integers

[Pedro Alves]

On 2022-05-06 12:55, Jan Beulich wrote:

>> diff --git a/gas/as.h b/gas/as.h
>> index 135abc8f23d..5b636cfd59a 100644
>> --- a/gas/as.h
>> +++ b/gas/as.h
>> @@ -139,7 +139,7 @@ typedef bfd_vma addressT;
>>  typedef bfd_signed_vma offsetT;
>>  
>>  /* Type of symbol value, etc.  For use in prototypes.  */
>> -typedef addressT valueT;
>> +typedef uint64_t valueT;
> 
> ... I'd be afraid this might alter behavior for purely 32-bit targets.
> Whether such a behavioral change would be deemed okay I'm not sure.

My thinking is that if that were a problem in practice, --enable-64-bit-bfd would already
have such a problem, but I couldn't find anything in gas's configure rejecting that option
for any target.  I looked a bit around in bugzilla and didn't find anyone complaining about gas
32-bit vs 64-bit behavior on 32-bit archs, and didn't find anything, but admittedly
I may have not searched properly.

Re: 32-bit archs, want64=true, and gas integers

[Hans-Peter Nilsson]

> From: Pedro Alves <pedro@palves.net>
> Date: Fri, 6 May 2022 12:43:27 +0200

> If something like this went in, then I assume that cris could go back to not setting
> want64=yes.
> [gas expr patch]

While something like that could work (thanks for your work),
I still prefer having all binutils behavior consistent
regardless of host 32/64-bitness.  There's at least objdump
output too.

brgds, H-P

Re: 32-bit archs, want64=true, and gas integers

[Pedro Alves]

On 2022-05-06 15:32, Hans-Peter Nilsson wrote:
>> From: Pedro Alves <pedro@palves.net>
>> Date: Fri, 6 May 2022 12:43:27 +0200
> 
>> If something like this went in, then I assume that cris could go back to not setting
>> want64=yes.
>> [gas expr patch]
> 
> While something like that could work (thanks for your work),
> I still prefer having all binutils behavior consistent
> regardless of host 32/64-bitness.  

Can't argue with that.  As I mentioned, GDB switched to always using 64-bit CORE_ADDR
a few years ago.  I've never heard anyone complain about that causing slowdown or higher
memory usage.  Plus, who develops on 32-bit hosts nowadays, anyhow?

> There's at least objdump output too.

Curious, what sort of objdump output difference does 32-bit/64-bit bfd cause?

Re: 32-bit archs, want64=true, and gas integers

[Hans-Peter Nilsson]

> From: Pedro Alves <pedro@palves.net>
> Date: Fri, 6 May 2022 16:44:47 +0200

> > There's at least objdump output too.
> 
> Curious, what sort of objdump output difference does 32-bit/64-bit bfd cause?

None that I know of; I misremembered, sorry.

(I had a related experience with mips objects a few years
ago, so I'll have to look into what part of that I
misremembered.  You may be able to do it with a "binary"
bfd, but you'd have to jump through hoops to get it "wrong"
so that doesn't count.)

brgds, H-P