Re: [PATCH] or1k: Add support for a little-endian target variant

[Samuel Holland <samuel@sholland.org>]

Hi Stafford,

Thanks for the review.

On 6/9/22 6:21 AM, Stafford Horne wrote:
> On Thu, Jun 09, 2022 at 01:01:33AM -0500, Samuel Holland wrote:
>> While not officially sanctioned by the architecture spec, little-endian
>> or1k processors do exist in the wild, for example the Allwinner AR100.
>> Let's add native support for this, instead of hacks like using objcopy
>> to byteswap ELF file contents.
> 
> Hello,
> 
> In general I have no objections to this.  If there are processors that
> are little endian it makes sense to support it.  Do you have any details
> of how people built for these before? I am curious.

We post-process the binary with "objcopy --reverse-bytes 4".

https://github.com/crust-firmware/crust/blob/master/Makefile#L193
https://github.com/intelligent-agent/klipper/blob/master/src/ar100/Makefile#L39
https://github.com/nfeske/genode-allwinner/blob/scp/src/scp/Makefile#L22

The processor has both a word-invariant mode (the default), and a byte-invariant
mode:

https://linux-sunxi.org/AR100#Byte-invariant_Ranges

With a big-endian toolchain, in the word-invariant mode, all GCC optimizations
work fine, but sharing data with other software is painful. Sub-32-bit memory
accesses are reversed within each group of 4 bytes, which affects strings and
struct layouts.

In the byte-invariant mode, sharing strings and structs causes no problems, but
some GCC optimizations like -fstore-merging are broken.

>> diff --git a/gas/config/tc-or1k.c b/gas/config/tc-or1k.c
>> index ae4e3452f48..9dc5a46f2e2 100644
>> --- a/gas/config/tc-or1k.c
>> +++ b/gas/config/tc-or1k.c
>> @@ -58,8 +58,16 @@ const char FLT_CHARS[]            = "dD";
>>  #define OR1K_SHORTOPTS "m:"
>>  const char * md_shortopts = OR1K_SHORTOPTS;
>>  
>> +enum
>> +{
>> +  OPTION_LITTLE_ENDIAN = OPTION_MD_BASE,
>> +  OPTION_BIG_ENDIAN
>> +};
>> +
>>  struct option md_longopts[] =
>>  {
>> +  {"EB", no_argument, NULL, OPTION_BIG_ENDIAN},
>> +  {"EL", no_argument, NULL, OPTION_LITTLE_ENDIAN},
>>    {NULL, no_argument, NULL, 0}
>>  };
>>  size_t md_longopts_size = sizeof (md_longopts);
>> @@ -67,14 +75,30 @@ size_t md_longopts_size = sizeof (md_longopts);
>>  unsigned long or1k_machine = 0; /* default */
>>  
>>  int
>> -md_parse_option (int c ATTRIBUTE_UNUSED, const char * arg ATTRIBUTE_UNUSED)
>> +md_parse_option (int c, const char * arg ATTRIBUTE_UNUSED)
>>  {
>> -  return 0;
>> +  switch (c)
>> +    {
>> +    case OPTION_BIG_ENDIAN:
>> +      target_big_endian = 1;
>> +      break;
>> +    case OPTION_LITTLE_ENDIAN:
>> +      target_big_endian = 0;
>> +      break;
>> +    default:
>> +      return 0;
>> +    }
>> +
>> +  return 1;
>>  }
>>  
>>  void
>> -md_show_usage (FILE * stream ATTRIBUTE_UNUSED)
>> +md_show_usage (FILE * stream)
>>  {
>> +  fprintf (stream, _(" OR1K-specific assembler options:\n"));
>> +  fprintf (stream, _("\
>> +  --EB			generate code for a big endian machine\n\
>> +  --EL			generate code for a little endian machine\n"));
>>  }
> 
> Aboce you mention -EB, -EL, here is is --EB, --EL.

They are long options. I will fix the changelog.

> Does this setup big endian as the default?  We should specify that in the
> options.  i.e. "generate code for a big endian machine, this is the default."
> 
> But I am not sure how that defaulting works now.  I will try to build this
> and understand better.

The default depends on the target. It comes from gas/configure.tgt.

Regards,
Samuel