Re: [PATCH] break gdb build on 32-bit host with ADI support

[Wei-min Pan <weimin.pan@oracle.com>]

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