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

[Wei-min Pan <weimin dot pan at oracle dot 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