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

[Pedro Alves <palves@redhat.com>]

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