break gdb build on 32-bit host with ADI support

  The problem of failing to build with arm-linux-gnueabihf-g++-4.8 was
that type CORE_ADDR is of "unsigned long" on a 64-bit machine but is
of type "unsigned long long" on a 32 bit system, which caused type
mismatch when passing arguments or printing errors.

Fixed the problem in three places - (1) change type of "blksize" and "nbits"
in struct adi_stat_t to "unsigned long long"; (2) explicitly cast argument 3
type when calling target_auxv_search(); (2) use %llx with explicit cast when
printing type CORE_ADDR in either printf_filtered() or error().
---
 gdb/sparc64-tdep.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)
  

On 08/24/2017 06:23 PM, Weimin Pan wrote:

> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
> index 6f4fca7..0da2ae5 100644
> --- a/gdb/sparc64-tdep.c
> +++ b/gdb/sparc64-tdep.c
> @@ -90,12 +90,12 @@ static struct cmd_list_element *sparc64adilist = NULL;
>  typedef struct
>  {
>    /* The ADI block size.  */
> -  unsigned long blksize;
> +  unsigned long long blksize;
>  
>    /* Number of bits used for an ADI version tag which can be
>     * used together with the shift value for an ADI version tag
>     * to encode or extract the ADI version value in a pointer.  */
> -  unsigned long nbits;
> +  unsigned long long nbits;

Do you really need to count 64-bit bits? :-P  :-)

(Formatting of comment is incorrect for GNU code, BTW.  No '*'
on each line.)

>  
>    /* The maximum ADI version tag value supported.  */
>    int max_version;
> @@ -223,9 +223,10 @@ adi_available (void)
>  
>    proc->stat.checked_avail = true;
>    if (target_auxv_search (&current_target, AT_ADI_BLKSZ, 
> -                          &proc->stat.blksize) <= 0)
> +                          (CORE_ADDR *)&proc->stat.blksize) <= 0)

Please don't introduce potential aliasing problems.  Also, missing
space before &.

Either make blksize really be a CORE_ADDR or do

    CORE_ADDR value;
    if (target_auxv_search (&current_target, AT_ADI_BLKSZ, &value) <= 0)
      return false;
    proc->stat.blksize = value;

> -  target_auxv_search (&current_target, AT_ADI_NBITS, &proc->stat.nbits);
> +  target_auxv_search (&current_target, AT_ADI_NBITS, 
> +                      (CORE_ADDR *)&proc->stat.nbits);
>    proc->stat.max_version = (1 << proc->stat.nbits) - 2;
>    proc->stat.is_avail = true;
>  

Ditto.

> @@ -346,7 +347,8 @@ adi_read_versions (CORE_ADDR vaddr, size_t size, unsigned char *tags)
>    if (!adi_is_addr_mapped (vaddr, size))
>      {
>        adi_stat_t ast = get_adi_info (ptid_get_pid (inferior_ptid));
> -      error(_("Address at 0x%lx is not in ADI maps"), vaddr*ast.blksize);
> +      error(_("Address at 0x%llx is not in ADI maps"), 
> +            (long long)(vaddr*ast.blksize));
>      }

Use paddress instead?  Also spaces around '*' and after the cast.

>  
>    int target_errno;
> @@ -366,7 +368,8 @@ adi_write_versions (CORE_ADDR vaddr, size_t size, unsigned char *tags)
>    if (!adi_is_addr_mapped (vaddr, size))
>      {
>        adi_stat_t ast = get_adi_info (ptid_get_pid (inferior_ptid));
> -      error(_("Address at 0x%lx is not in ADI maps"), vaddr*ast.blksize);
> +      error(_("Address at 0x%llx is not in ADI maps"), 
> +            (long long)(vaddr*ast.blksize));

Ditto.

>      }
>  
>    int target_errno;
> @@ -387,7 +390,7 @@ adi_print_versions (CORE_ADDR vaddr, size_t cnt, unsigned char *tags)
>    while (cnt > 0)
>      {
>        QUIT;
> -      printf_filtered ("0x%016lx:\t", vaddr * adi_stat.blksize);
> +      printf_filtered ("0x%016llx:\t", (long long)(vaddr*adi_stat.blksize));

paddress / hex_string / phex_nz ?

>        for (int i = maxelts; i > 0 && cnt > 0; i--, cnt--)
>          {
>            if (tags[v_idx] == 0xff)    /* no version tag */
> @@ -418,7 +421,7 @@ do_examine (CORE_ADDR start, int bcnt)
>    if (read_cnt == -1)
>      error (_("No ADI information"));
>    else if (read_cnt < cnt)
> -    error(_("No ADI information at 0x%lx"), vaddr);
> +    error(_("No ADI information at 0x%llx"), (long long)vaddr);

padress.

>  
>    adi_print_versions (vstart, cnt, buf);
>  
> @@ -438,7 +441,7 @@ do_assign (CORE_ADDR start, size_t bcnt, int version)
>    if (set_cnt == -1)
>      error (_("No ADI information"));
>    else if (set_cnt < cnt)
> -    error(_("No ADI information at 0x%lx"), vaddr);
> +    error(_("No ADI information at 0x%llx"), (long long)vaddr);

paddress.

BTW, this cast to long here:

 static CORE_ADDR
 adi_normalize_address (CORE_ADDR addr)
 {
   adi_stat_t ast = get_adi_info (ptid_get_pid (inferior_ptid));

   if (ast.nbits)
     return ((CORE_ADDR)(((long)addr << ast.nbits) >> ast.nbits));
   return addr;
 }

looks suspiciously bogus to me.  Consider a 32-bit host
remote/cross debugging a SPARC64 target machine.  Also consider
a Win64-hosted GDB.

Thanks,
Pedro Alves
  

On 8/24/2017 11:07 AM, Pedro Alves wrote:
> On 08/24/2017 06:23 PM, Weimin Pan wrote:
>
>> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
>> index 6f4fca7..0da2ae5 100644
>> --- a/gdb/sparc64-tdep.c
>> +++ b/gdb/sparc64-tdep.c
>> @@ -90,12 +90,12 @@ static struct cmd_list_element *sparc64adilist = NULL;
>>   typedef struct
>>   {
>>     /* The ADI block size.  */
>> -  unsigned long blksize;
>> +  unsigned long long blksize;
>>   
>>     /* Number of bits used for an ADI version tag which can be
>>      * used together with the shift value for an ADI version tag
>>      * to encode or extract the ADI version value in a pointer.  */
>> -  unsigned long nbits;
>> +  unsigned long long nbits;
> Do you really need to count 64-bit bits? :-P  :-)

Since the value of either nbits or blksize is between 0 and 64,
no, I really don't. But without a 32-bit host, I'm simply play
safe here so that the compiler won't bark.
> (Formatting of comment is incorrect for GNU code, BTW.  No '*'
> on each line.)

Corrected.

>>   
>>     /* The maximum ADI version tag value supported.  */
>>     int max_version;
>> @@ -223,9 +223,10 @@ adi_available (void)
>>   
>>     proc->stat.checked_avail = true;
>>     if (target_auxv_search (&current_target, AT_ADI_BLKSZ,
>> -                          &proc->stat.blksize) <= 0)
>> +                          (CORE_ADDR *)&proc->stat.blksize) <= 0)
> Please don't introduce potential aliasing problems.  Also, missing
> space before &.
>
> Either make blksize really be a CORE_ADDR or do
>
>      CORE_ADDR value;
>      if (target_auxv_search (&current_target, AT_ADI_BLKSZ, &value) <= 0)
>        return false;
>      proc->stat.blksize = value;

Since neither blksize nor nbits is a CORE_ADDR, I'm taking your second 
suggestion.

>> -  target_auxv_search (&current_target, AT_ADI_NBITS, &proc->stat.nbits);
>> +  target_auxv_search (&current_target, AT_ADI_NBITS,
>> +                      (CORE_ADDR *)&proc->stat.nbits);
>>     proc->stat.max_version = (1 << proc->stat.nbits) - 2;
>>     proc->stat.is_avail = true;
>>   
> Ditto.
>
>> @@ -346,7 +347,8 @@ adi_read_versions (CORE_ADDR vaddr, size_t size, unsigned char *tags)
>>     if (!adi_is_addr_mapped (vaddr, size))
>>       {
>>         adi_stat_t ast = get_adi_info (ptid_get_pid (inferior_ptid));
>> -      error(_("Address at 0x%lx is not in ADI maps"), vaddr*ast.blksize);
>> +      error(_("Address at 0x%llx is not in ADI maps"),
>> +            (long long)(vaddr*ast.blksize));
>>       }
> Use paddress instead?  Also spaces around '*' and after the cast.

Where is paddress defined? I tried casting to "uint64" which yields to
"unsigned long" on a 64-bit host and didn't bode well with %llx.

>>   
>>     int target_errno;
>> @@ -366,7 +368,8 @@ adi_write_versions (CORE_ADDR vaddr, size_t size, unsigned char *tags)
>>     if (!adi_is_addr_mapped (vaddr, size))
>>       {
>>         adi_stat_t ast = get_adi_info (ptid_get_pid (inferior_ptid));
>> -      error(_("Address at 0x%lx is not in ADI maps"), vaddr*ast.blksize);
>> +      error(_("Address at 0x%llx is not in ADI maps"),
>> +            (long long)(vaddr*ast.blksize));
> Ditto.
>
>>       }
>>   
>>     int target_errno;
>> @@ -387,7 +390,7 @@ adi_print_versions (CORE_ADDR vaddr, size_t cnt, unsigned char *tags)
>>     while (cnt > 0)
>>       {
>>         QUIT;
>> -      printf_filtered ("0x%016lx:\t", vaddr * adi_stat.blksize);
>> +      printf_filtered ("0x%016llx:\t", (long long)(vaddr*adi_stat.blksize));
> paddress / hex_string / phex_nz ?

??

>>         for (int i = maxelts; i > 0 && cnt > 0; i--, cnt--)
>>           {
>>             if (tags[v_idx] == 0xff)    /* no version tag */
>> @@ -418,7 +421,7 @@ do_examine (CORE_ADDR start, int bcnt)
>>     if (read_cnt == -1)
>>       error (_("No ADI information"));
>>     else if (read_cnt < cnt)
>> -    error(_("No ADI information at 0x%lx"), vaddr);
>> +    error(_("No ADI information at 0x%llx"), (long long)vaddr);
> padress.
>
>>   
>>     adi_print_versions (vstart, cnt, buf);
>>   
>> @@ -438,7 +441,7 @@ do_assign (CORE_ADDR start, size_t bcnt, int version)
>>     if (set_cnt == -1)
>>       error (_("No ADI information"));
>>     else if (set_cnt < cnt)
>> -    error(_("No ADI information at 0x%lx"), vaddr);
>> +    error(_("No ADI information at 0x%llx"), (long long)vaddr);
> paddress.
>
> BTW, this cast to long here:
>
>   static CORE_ADDR
>   adi_normalize_address (CORE_ADDR addr)
>   {
>     adi_stat_t ast = get_adi_info (ptid_get_pid (inferior_ptid));
>
>     if (ast.nbits)
>       return ((CORE_ADDR)(((long)addr << ast.nbits) >> ast.nbits));
>     return addr;
>   }
>
> looks suspiciously bogus to me.  Consider a 32-bit host
> remote/cross debugging a SPARC64 target machine.  Also consider
> a Win64-hosted GDB.

Good point. Changing it to:

       return ((long long)(((long long)addr << ast.nbits) >> ast.nbits));

Thanks.

>
> Thanks,
> Pedro Alves
>
  

On 08/24/2017 08:09 PM, Wei-min Pan wrote:
> 
> 
> On 8/24/2017 11:07 AM, Pedro Alves wrote:
>> On 08/24/2017 06:23 PM, Weimin Pan wrote:
>>
>>> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
>>> index 6f4fca7..0da2ae5 100644
>>> --- a/gdb/sparc64-tdep.c
>>> +++ b/gdb/sparc64-tdep.c
>>> @@ -90,12 +90,12 @@ static struct cmd_list_element *sparc64adilist =
>>> NULL;
>>>   typedef struct
>>>   {
>>>     /* The ADI block size.  */
>>> -  unsigned long blksize;
>>> +  unsigned long long blksize;
>>>       /* Number of bits used for an ADI version tag which can be
>>>      * used together with the shift value for an ADI version tag
>>>      * to encode or extract the ADI version value in a pointer.  */
>>> -  unsigned long nbits;
>>> +  unsigned long long nbits;
>> Do you really need to count 64-bit bits? :-P  :-)
> 
> Since the value of either nbits or blksize is between 0 and 64,
> no, I really don't. But without a 32-bit host, I'm simply play
> safe here so that the compiler won't bark.
>> (Formatting of comment is incorrect for GNU code, BTW.  No '*'
>> on each line.)
> 
> Corrected.
> 
>>>       /* The maximum ADI version tag value supported.  */
>>>     int max_version;
>>> @@ -223,9 +223,10 @@ adi_available (void)
>>>       proc->stat.checked_avail = true;
>>>     if (target_auxv_search (&current_target, AT_ADI_BLKSZ,
>>> -                          &proc->stat.blksize) <= 0)
>>> +                          (CORE_ADDR *)&proc->stat.blksize) <= 0)
>> Please don't introduce potential aliasing problems.  Also, missing
>> space before &.
>>
>> Either make blksize really be a CORE_ADDR or do
>>
>>      CORE_ADDR value;
>>      if (target_auxv_search (&current_target, AT_ADI_BLKSZ, &value) <= 0)
>>        return false;
>>      proc->stat.blksize = value;
> 
> Since neither blksize nor nbits is a CORE_ADDR, I'm taking your second
> suggestion.

Then you don't need the

 -  unsigned long nbits;
 +  unsigned long long nbits;

change anymore..

> 

>>> @@ -346,7 +347,8 @@ adi_read_versions (CORE_ADDR vaddr, size_t size,
>>> unsigned char *tags)
>>>     if (!adi_is_addr_mapped (vaddr, size))
>>>       {
>>>         adi_stat_t ast = get_adi_info (ptid_get_pid (inferior_ptid));
>>> -      error(_("Address at 0x%lx is not in ADI maps"),
>>> vaddr*ast.blksize);
>>> +      error(_("Address at 0x%llx is not in ADI maps"),
>>> +            (long long)(vaddr*ast.blksize));
>>>       }
>> Use paddress instead?  Also spaces around '*' and after the cast.
> 
> Where is paddress defined? I tried casting to "uint64" which yields to
> "unsigned long" on a 64-bit host and didn't bode well with %llx.

 $ grep paddress *.h
 utils.h:extern const char *paddress (struct gdbarch *gdbarch, CORE_ADDR addr);


>>> @@ -387,7 +390,7 @@ adi_print_versions (CORE_ADDR vaddr, size_t cnt,
>>> unsigned char *tags)
>>>     while (cnt > 0)
>>>       {
>>>         QUIT;
>>> -      printf_filtered ("0x%016lx:\t", vaddr * adi_stat.blksize);
>>> +      printf_filtered ("0x%016llx:\t", (long
>>> long)(vaddr*adi_stat.blksize));
>> paddress / hex_string / phex_nz ?
> 
> ??

Try grepping for those things.


>>   static CORE_ADDR
>>   adi_normalize_address (CORE_ADDR addr)
>>   {
>>     adi_stat_t ast = get_adi_info (ptid_get_pid (inferior_ptid));
>>
>>     if (ast.nbits)
>>       return ((CORE_ADDR)(((long)addr << ast.nbits) >> ast.nbits));
>>     return addr;
>>   }
>>
>> looks suspiciously bogus to me.  Consider a 32-bit host
>> remote/cross debugging a SPARC64 target machine.  Also consider
>> a Win64-hosted GDB.
> 
> Good point. Changing it to:
> 
>       return ((long long)(((long long)addr << ast.nbits) >> ast.nbits));
> 
> Thanks.

Still looks odd to me.  
Why are you shifting signed types, for instance?
Any why do you need the casts in the first place, BTW?

Thanks,
Pedro Alves
  

On 8/24/2017 12:23 PM, Pedro Alves wrote:
> On 08/24/2017 08:09 PM, Wei-min Pan wrote:
>>
>> On 8/24/2017 11:07 AM, Pedro Alves wrote:
>>> On 08/24/2017 06:23 PM, Weimin Pan wrote:
>>>
>>>> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
>>>> index 6f4fca7..0da2ae5 100644
>>>> --- a/gdb/sparc64-tdep.c
>>>> +++ b/gdb/sparc64-tdep.c
>>>> @@ -90,12 +90,12 @@ static struct cmd_list_element *sparc64adilist =
>>>> NULL;
>>>>    typedef struct
>>>>    {
>>>>      /* The ADI block size.  */
>>>> -  unsigned long blksize;
>>>> +  unsigned long long blksize;
>>>>        /* Number of bits used for an ADI version tag which can be
>>>>       * used together with the shift value for an ADI version tag
>>>>       * to encode or extract the ADI version value in a pointer.  */
>>>> -  unsigned long nbits;
>>>> +  unsigned long long nbits;
>>> Do you really need to count 64-bit bits? :-P  :-)
>> Since the value of either nbits or blksize is between 0 and 64,
>> no, I really don't. But without a 32-bit host, I'm simply play
>> safe here so that the compiler won't bark.
>>> (Formatting of comment is incorrect for GNU code, BTW.  No '*'
>>> on each line.)
>> Corrected.
>>
>>>>        /* The maximum ADI version tag value supported.  */
>>>>      int max_version;
>>>> @@ -223,9 +223,10 @@ adi_available (void)
>>>>        proc->stat.checked_avail = true;
>>>>      if (target_auxv_search (&current_target, AT_ADI_BLKSZ,
>>>> -                          &proc->stat.blksize) <= 0)
>>>> +                          (CORE_ADDR *)&proc->stat.blksize) <= 0)
>>> Please don't introduce potential aliasing problems.  Also, missing
>>> space before &.
>>>
>>> Either make blksize really be a CORE_ADDR or do
>>>
>>>       CORE_ADDR value;
>>>       if (target_auxv_search (&current_target, AT_ADI_BLKSZ, &value) <= 0)
>>>         return false;
>>>       proc->stat.blksize = value;
>> Since neither blksize nor nbits is a CORE_ADDR, I'm taking your second
>> suggestion.
> Then you don't need the
>
>   -  unsigned long nbits;
>   +  unsigned long long nbits;
>
> change anymore..
>
>>>> @@ -346,7 +347,8 @@ adi_read_versions (CORE_ADDR vaddr, size_t size,
>>>> unsigned char *tags)
>>>>      if (!adi_is_addr_mapped (vaddr, size))
>>>>        {
>>>>          adi_stat_t ast = get_adi_info (ptid_get_pid (inferior_ptid));
>>>> -      error(_("Address at 0x%lx is not in ADI maps"),
>>>> vaddr*ast.blksize);
>>>> +      error(_("Address at 0x%llx is not in ADI maps"),
>>>> +            (long long)(vaddr*ast.blksize));
>>>>        }
>>> Use paddress instead?  Also spaces around '*' and after the cast.
>> Where is paddress defined? I tried casting to "uint64" which yields to
>> "unsigned long" on a 64-bit host and didn't bode well with %llx.
>   $ grep paddress *.h
>   utils.h:extern const char *paddress (struct gdbarch *gdbarch, CORE_ADDR addr);

On a 64-bit host:
...
sparc64-tdep.c: In function 'void do_assign(CORE_ADDR, size_t, int)':
sparc64-tdep.c:449:56: error: expected ')' before 'vaddr'
      error(_("No ADI information at 0x%llx"), (paddress)vaddr);
                                                         ^~~~~
sparc64-tdep.c:449:61: error: format '%llx' expects argument of type 
'long long
unsigned int', but argument 2 has type 'const char* (*)(gdbarch*, 
CORE_ADDR) {ak
a const char* (*)(gdbarch*, long unsigned int)}' [-Werror=format=]
      error(_("No ADI information at 0x%llx"), (paddress)vaddr);
                                                              ^
cc1plus: all warnings being treated as errors
make: *** [sparc64-tdep.o] Error 1


>>>> @@ -387,7 +390,7 @@ adi_print_versions (CORE_ADDR vaddr, size_t cnt,
>>>> unsigned char *tags)
>>>>      while (cnt > 0)
>>>>        {
>>>>          QUIT;
>>>> -      printf_filtered ("0x%016lx:\t", vaddr * adi_stat.blksize);
>>>> +      printf_filtered ("0x%016llx:\t", (long
>>>> long)(vaddr*adi_stat.blksize));
>>> paddress / hex_string / phex_nz ?
>> ??
> Try grepping for those things.
>
>
>>>    static CORE_ADDR
>>>    adi_normalize_address (CORE_ADDR addr)
>>>    {
>>>      adi_stat_t ast = get_adi_info (ptid_get_pid (inferior_ptid));
>>>
>>>      if (ast.nbits)
>>>        return ((CORE_ADDR)(((long)addr << ast.nbits) >> ast.nbits));
>>>      return addr;
>>>    }
>>>
>>> looks suspiciously bogus to me.  Consider a 32-bit host
>>> remote/cross debugging a SPARC64 target machine.  Also consider
>>> a Win64-hosted GDB.
>> Good point. Changing it to:
>>
>>        return ((long long)(((long long)addr << ast.nbits) >> ast.nbits));
>>
>> Thanks.
> Still looks odd to me.
> Why are you shifting signed types, for instance?
> Any why do you need the casts in the first place, BTW?

We need to sign extend to get a normalized address, based on the 
definitions in ADI space:

         ADI versioned address - a VA with ADI bits (63-60) set
         Normalized address - a VA with bit 59 sign extended into ADI bits


Thanks.

>
> Thanks,
> Pedro Alves
  

On 08/24/2017 09:27 PM, Wei-min Pan wrote:

>>>> Use paddress instead?  Also spaces around '*' and after the cast.
>>> Where is paddress defined? I tried casting to "uint64" which yields to
>>> "unsigned long" on a 64-bit host and didn't bode well with %llx.
>>   $ grep paddress *.h
>>   utils.h:extern const char *paddress (struct gdbarch *gdbarch,
>> CORE_ADDR addr);
> 
> On a 64-bit host:
> ...
> sparc64-tdep.c: In function 'void do_assign(CORE_ADDR, size_t, int)':
> sparc64-tdep.c:449:56: error: expected ')' before 'vaddr'
>      error(_("No ADI information at 0x%llx"), (paddress)vaddr);
>                                                         ^~~~~
> sparc64-tdep.c:449:61: error: format '%llx' expects argument of type
> 'long long
> unsigned int', but argument 2 has type 'const char* (*)(gdbarch*,
> CORE_ADDR) {ak
> a const char* (*)(gdbarch*, long unsigned int)}' [-Werror=format=]
>      error(_("No ADI information at 0x%llx"), (paddress)vaddr);
>                                                              ^
> cc1plus: all warnings being treated as errors
> make: *** [sparc64-tdep.o] Error 1

Of course.  paddress is a function.  See any of the other ~400 uses
in the tree.  Something like:

 error(_("No ADI information at %s"), paddress (gdbarch, vaddr));

>>>> looks suspiciously bogus to me.  Consider a 32-bit host
>>>> remote/cross debugging a SPARC64 target machine.  Also consider
>>>> a Win64-hosted GDB.
>>> Good point. Changing it to:
>>>
>>>        return ((long long)(((long long)addr << ast.nbits) >>
>>> ast.nbits));
>>>
>>> Thanks.
>> Still looks odd to me.
>> Why are you shifting signed types, for instance?
>> Any why do you need the casts in the first place, BTW?
> 
> We need to sign extend to get a normalized address, based on the
> definitions in ADI space:
> 
>         ADI versioned address - a VA with ADI bits (63-60) set
>         Normalized address - a VA with bit 59 sign extended into ADI bits

OK, but then please do it with valid/portable C/C++ code, with
the usual masking and xoring.
Left shifting signed integer values is undefined C++11.
Right shifting a signed integer is implementation defined.

Thanks,
Pedro Alves