Re: [PATCH 02/26] -Wpointer-sign: gdb_byte -> char.

Pedro Alves
2013-04-12T09:54:12Z

On 04/12/2013 03:32 AM, Yao Qi wrote:
> On 04/12/2013 06:59 AM, Pedro Alves wrote:
>> diff --git a/gdb/i386-cygwin-tdep.c b/gdb/i386-cygwin-tdep.c
>> index cee2adf..26ffbaa 100644
>> --- a/gdb/i386-cygwin-tdep.c
>> +++ b/gdb/i386-cygwin-tdep.c
>> @@ -125,11 +125,11 @@ core_process_module_section (bfd *abfd, asection *sect, void *obj)
>> struct cpms_data *data = obj;
>> enum bfd_endian byte_order = gdbarch_byte_order (data->gdbarch);
>>
>> - char *module_name;
>> + gdb_byte *module_name;
>> size_t module_name_size;
>> CORE_ADDR base_addr;
>>
>> - char *buf = NULL;
>> + gdb_byte *buf = NULL;
>>
>> if (strncmp (sect->name, ".module", 7) != 0)
>> return;
>> @@ -160,7 +160,7 @@ core_process_module_section (bfd *abfd, asection *sect, void *obj)
>>
>> /* The first module is the .exe itself. */
>> if (data->module_count != 0)
>> - windows_xfer_shared_library (module_name, base_addr,
>> + windows_xfer_shared_library ((char *) module_name, base_addr,
>> data->gdbarch, data->obstack);
>> data->module_count++;
>>
>
> It would be nice to keep 'module_name' of type 'char *', because it is a string.

Hmm, in my original attempt, I tried it and ended up with more casts overall.
But I now notice that "module_name - buf" is "12". I've merged this in
into the patch:

gdb/i386-cygwin-tdep.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/i386-cygwin-tdep.c b/gdb/i386-cygwin-tdep.c
index 26ffbaa..dc5d614 100644
--- a/gdb/i386-cygwin-tdep.c
+++ b/gdb/i386-cygwin-tdep.c
@@ -125,7 +125,7 @@ core_process_module_section (bfd *abfd, asection *sect, void *obj)
struct cpms_data *data = obj;
enum bfd_endian byte_order = gdbarch_byte_order (data->gdbarch);

- gdb_byte *module_name;
+ char *module_name;
size_t module_name_size;
CORE_ADDR base_addr;

@@ -154,13 +154,13 @@ core_process_module_section (bfd *abfd, asection *sect, void *obj)
module_name_size =
extract_unsigned_integer (buf + 8, 4, byte_order);

- module_name = buf + 12;
- if (module_name - buf + module_name_size > bfd_get_section_size (sect))
+ if (12 + module_name_size > bfd_get_section_size (sect))
goto out;
+ module_name = (char *) buf + 12;

/* The first module is the .exe itself. */
if (data->module_count != 0)
- windows_xfer_shared_library ((char *) module_name, base_addr,
+ windows_xfer_shared_library (module_name, base_addr,
data->gdbarch, data->obstack);
data->module_count++;

I moved the "module_name =" line below because it's neater
to check overflow before causing it. Overall this does look nicer.
Thanks.

> bfd_boolean bfd_get_section_contents
> (bfd *abfd, asection *section, void *location, file_ptr offset,
> bfd_size_type count);
>
> the parameter 'location' 's type is 'void *', do we really need to cast 'buf' to 'bfd_byte *'?

Thanks. I really can't say why I added it.

> Likewise.

Both bits reverted.

Below's the current version. I've updated the github branch too.

-------------
-Wpointer-sign: gdb_byte -> char.

This is sort of the opposite of a previous patch. Places that
manipulate strings or interfaces that return strings are changed to
use char* instead of gdb_byte*.

gdb/
2013-04-12 Pedro Alves <palves@redhat.com>

* avr-tdep.c (avr_io_reg_read_command): New local 'bufstr'. Use
it to get a string view of the byte buffer.
* i386-cygwin-tdep.c (core_process_module_section): Change local 'buf'
type to gdb_byte *. Adjust.
* linux-tdep.c (linux_info_proc, linux_find_memory_regions_full):
Change local to char *.
* solib-darwin.c (find_program_interpreter): Change return type to
char *. Adjust.
(darwin_solib_get_all_image_info_addr_at_init): Adjust.
* solib-dsbt.c (enable_break2): Change local 'buf' to char *.
* solib-frv.c (enable_break2): Change local 'buf' to char *.
* solib-spu.c (spu_current_sos): Add gdb_byte * cast.
* solib-svr4.c (find_program_interpreter): Change return type to
char *. Adjust.
(enable_break): Change local 'interp_name' to char *.
* spu-multiarch.c (spu_xfer_partial): Add cast to 'char *'.
* spu-tdep.c (spu_pseudo_register_read_spu): Add cast to 'char *'.
(spu_pseudo_register_write_spu): Use char for string buffer.
Adjust.
(info_spu_event_command, info_spu_signal_command): Add casts to
'char *'.
---

gdb/avr-tdep.c | 8 +++++---
gdb/i386-cygwin-tdep.c | 6 +++---
gdb/linux-tdep.c | 4 ++--
gdb/solib-darwin.c | 6 +++---
gdb/solib-dsbt.c | 2 +-
gdb/solib-frv.c | 2 +-
gdb/solib-spu.c | 2 +-
gdb/solib-svr4.c | 6 +++---
gdb/spu-multiarch.c | 2 +-
gdb/spu-tdep.c | 16 +++++++++-------
10 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c
index b14bf83..0bc08a8 100644
--- a/gdb/avr-tdep.c
+++ b/gdb/avr-tdep.c
@@ -1468,8 +1468,9 @@ avr_io_reg_read_command (char *args, int from_tty)
{
LONGEST bufsiz = 0;
gdb_byte *buf;
+ const char *bufstr;
char query[400];
- char *p;
+ const char *p;
unsigned int nreg = 0;
unsigned int val;
int i, j, k, step;
@@ -1477,6 +1478,7 @@ avr_io_reg_read_command (char *args, int from_tty)
/* Find out how many io registers the target has. */
bufsiz = target_read_alloc (&current_target, TARGET_OBJECT_AVR,
"avr.io_reg", &buf);
+ bufstr = (const char *) buf;

if (bufsiz <= 0)
{
@@ -1486,7 +1488,7 @@ avr_io_reg_read_command (char *args, int from_tty)
return;
}

- if (sscanf (buf, "%x", &nreg) != 1)
+ if (sscanf (bufstr, "%x", &nreg) != 1)
{
fprintf_unfiltered (gdb_stderr,
_("Error fetching number of io registers\n"));
@@ -1514,7 +1516,7 @@ avr_io_reg_read_command (char *args, int from_tty)
bufsiz = target_read_alloc (&current_target, TARGET_OBJECT_AVR,
query, &buf);

- p = buf;
+ p = (const char *) buf;
for (k = i; k < (i + j); k++)
{
if (sscanf (p, "%[^,],%x;", query, &val) == 2)
diff --git a/gdb/i386-cygwin-tdep.c b/gdb/i386-cygwin-tdep.c
index cee2adf..dc5d614 100644
--- a/gdb/i386-cygwin-tdep.c
+++ b/gdb/i386-cygwin-tdep.c
@@ -129,7 +129,7 @@ core_process_module_section (bfd *abfd, asection *sect, void *obj)
size_t module_name_size;
CORE_ADDR base_addr;

- char *buf = NULL;
+ gdb_byte *buf = NULL;

if (strncmp (sect->name, ".module", 7) != 0)
return;
@@ -154,9 +154,9 @@ core_process_module_section (bfd *abfd, asection *sect, void *obj)
module_name_size =
extract_unsigned_integer (buf + 8, 4, byte_order);

- module_name = buf + 12;
- if (module_name - buf + module_name_size > bfd_get_section_size (sect))
+ if (12 + module_name_size > bfd_get_section_size (sect))
goto out;
+ module_name = (char *) buf + 12;

/* The first module is the .exe itself. */
if (data->module_count != 0)
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 9def108..9623d19 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -261,7 +261,7 @@ linux_info_proc (struct gdbarch *gdbarch, char *args,
int status_f = (what == IP_STATUS || what == IP_ALL);
int stat_f = (what == IP_STAT || what == IP_ALL);
char filename[100];
- gdb_byte *data;
+ char *data;
int target_errno;

if (args && isdigit (args[0]))
@@ -676,7 +676,7 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
void *obfd)
{
char mapsfilename[100];
- gdb_byte *data;
+ char *data;

/* We need to know the real target PID to access /proc. */
if (current_inferior ()->fake_pid_p)
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index 8c2307e..c4c6308 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -210,10 +210,10 @@ lookup_symbol_from_bfd (bfd *abfd, char *symname)

/* Return program interpreter string. */

-static gdb_byte *
+static char *
find_program_interpreter (void)
{
- gdb_byte *buf = NULL;
+ char *buf = NULL;

/* If we have an exec_bfd, get the interpreter from the load commands. */
if (exec_bfd)
@@ -420,7 +420,7 @@ gdb_bfd_mach_o_fat_extract (bfd *abfd, bfd_format format,
static void
darwin_solib_get_all_image_info_addr_at_init (struct darwin_info *info)
{
- gdb_byte *interp_name;
+ char *interp_name;
CORE_ADDR load_addr = 0;
bfd *dyld_bfd = NULL;
struct cleanup *cleanup;
diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index e2822c1..90321d9 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -840,7 +840,7 @@ enable_break2 (void)
if (interp_sect)
{
unsigned int interp_sect_size;
- gdb_byte *buf;
+ char *buf;
bfd *tmp_bfd = NULL;
CORE_ADDR addr;
gdb_byte addr_buf[TIC6X_PTR_SIZE];
diff --git a/gdb/solib-frv.c b/gdb/solib-frv.c
index 52588bc..0f3e5d7 100644
--- a/gdb/solib-frv.c
+++ b/gdb/solib-frv.c
@@ -535,7 +535,7 @@ enable_break2 (void)
if (interp_sect)
{
unsigned int interp_sect_size;
- gdb_byte *buf;
+ char *buf;
bfd *tmp_bfd = NULL;
int status;
CORE_ADDR addr, interp_loadmap_addr;
diff --git a/gdb/solib-spu.c b/gdb/solib-spu.c
index 7be5232..3acf9c5 100644
--- a/gdb/solib-spu.c
+++ b/gdb/solib-spu.c
@@ -209,7 +209,7 @@ spu_current_sos (void)
yet. Skip such entries; we'll be back for them later. */
xsnprintf (annex, sizeof annex, "%d/object-id", fd);
len = target_read (&current_target, TARGET_OBJECT_SPU, annex,
- id, 0, sizeof id);
+ (gdb_byte *) id, 0, sizeof id);
if (len <= 0 || len >= sizeof id)
continue;
id[len] = 0;
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index bb2a4e9..f3bff6e 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -496,7 +496,7 @@ read_program_header (int type, int *p_sect_size, int *p_arch_size)

/* Return program interpreter string. */
-static gdb_byte *
+static char *
find_program_interpreter (void)
{
gdb_byte *buf = NULL;
@@ -521,7 +521,7 @@ find_program_interpreter (void)
if (!buf)
buf = read_program_header (PT_INTERP, NULL, NULL);

- return buf;
+ return (char *) buf;
}

@@ -1446,7 +1446,7 @@ enable_break (struct svr4_info *info, int from_tty)
struct minimal_symbol *msymbol;
const char * const *bkpt_namep;
asection *interp_sect;
- gdb_byte *interp_name;
+ char *interp_name;
CORE_ADDR sym_addr;

info->interp_text_sect_low = info->interp_text_sect_high = 0;
diff --git a/gdb/spu-multiarch.c b/gdb/spu-multiarch.c
index 0922d04..a74bd30 100644
--- a/gdb/spu-multiarch.c
+++ b/gdb/spu-multiarch.c
@@ -285,7 +285,7 @@ spu_xfer_partial (struct target_ops *ops, enum target_object object,
0, sizeof buf) <= 0)
return ret;

- lslr = strtoulst (buf, NULL, 16);
+ lslr = strtoulst ((char *) buf, NULL, 16);
return ops_beneath->to_xfer_partial (ops_beneath, TARGET_OBJECT_SPU,
mem_annex, readbuf, writebuf,
addr & lslr, len);
diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c
index 32c8f1e..9ea9c73 100644
--- a/gdb/spu-tdep.c
+++ b/gdb/spu-tdep.c
@@ -192,6 +192,7 @@ spu_pseudo_register_read_spu (struct regcache *regcache, const char *regname,
gdb_byte reg[32];
char annex[32];
ULONGEST id;
+ ULONGEST ul;

status = regcache_raw_read_unsigned (regcache, SPU_ID_REGNUM, &id);
if (status != REG_VALID)
@@ -201,7 +202,8 @@ spu_pseudo_register_read_spu (struct regcache *regcache, const char *regname,
target_read (&current_target, TARGET_OBJECT_SPU, annex,
reg, 0, sizeof reg);

- store_unsigned_integer (buf, 4, byte_order, strtoulst (reg, NULL, 16));
+ ul = strtoulst ((char *) reg, NULL, 16);
+ store_unsigned_integer (buf, 4, byte_order, ul);
return REG_VALID;
}

@@ -254,7 +256,7 @@ spu_pseudo_register_write_spu (struct regcache *regcache, const char *regname,
{
struct gdbarch *gdbarch = get_regcache_arch (regcache);
enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
- gdb_byte reg[32];
+ char reg[32];
char annex[32];
ULONGEST id;

@@ -263,7 +265,7 @@ spu_pseudo_register_write_spu (struct regcache *regcache, const char *regname,
xsnprintf (reg, sizeof reg, "0x%s",
phex_nz (extract_unsigned_integer (buf, 4, byte_order), 4));
target_write (&current_target, TARGET_OBJECT_SPU, annex,
- reg, 0, strlen (reg));
+ (gdb_byte *) reg, 0, strlen (reg));
}

static void
@@ -2044,7 +2046,7 @@ info_spu_event_command (char *args, int from_tty)
if (len <= 0)
error (_("Could not read event_status."));
buf[len] = '\0';
- event_status = strtoulst (buf, NULL, 16);
+ event_status = strtoulst ((char *) buf, NULL, 16);

xsnprintf (annex, sizeof annex, "%d/event_mask", id);
len = target_read (&current_target, TARGET_OBJECT_SPU, annex,
@@ -2052,7 +2054,7 @@ info_spu_event_command (char *args, int from_tty)
if (len <= 0)
error (_("Could not read event_mask."));
buf[len] = '\0';
- event_mask = strtoulst (buf, NULL, 16);
+ event_mask = strtoulst ((char *) buf, NULL, 16);

chain = make_cleanup_ui_out_tuple_begin_end (current_uiout, "SPUInfoEvent");

@@ -2111,7 +2113,7 @@ info_spu_signal_command (char *args, int from_tty)
if (len <= 0)
error (_("Could not read signal1_type."));
buf[len] = '\0';
- signal1_type = strtoulst (buf, NULL, 16);
+ signal1_type = strtoulst ((char *) buf, NULL, 16);

xsnprintf (annex, sizeof annex, "%d/signal2", id);
len = target_read (&current_target, TARGET_OBJECT_SPU, annex, buf, 0, 4);
@@ -2129,7 +2131,7 @@ info_spu_signal_command (char *args, int from_tty)
if (len <= 0)
error (_("Could not read signal2_type."));
buf[len] = '\0';
- signal2_type = strtoulst (buf, NULL, 16);
+ signal2_type = strtoulst ((char *) buf, NULL, 16);

chain = make_cleanup_ui_out_tuple_begin_end (current_uiout, "SPUInfoSignal");