This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] break gdb build on 32-bit host with ADI support
- From: Pedro Alves <palves at redhat dot com>
- To: Wei-min Pan <weimin dot pan at oracle dot com>, gdb-patches at sourceware dot org
- Date: Thu, 24 Aug 2017 20:23:49 +0100
- Subject: Re: [PATCH] break gdb build on 32-bit host with ADI support
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 638BBC04B938
- References: <1503595405-89600-1-git-send-email-weimin.pan@oracle.com> <479506e1-9478-1a38-d15c-3df13a817fff@redhat.com> <faf36e12-9f84-bc05-746c-abcb1c20fbf9@oracle.com>
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 (¤t_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 (¤t_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