| Message ID | alpine.DEB.2.00.1610150200220.31859@tp.orcam.me.uk |
|---|---|
| State | Committed |
| Headers | show |
On 10/17/2016 04:18 PM, Maciej W. Rozycki wrote: > Fix a load of aliasing build errors: > > cc1plus: warnings being treated as errors > .../gdb/tilegx-tdep.c: In function 'CORE_ADDR tilegx_analyze_prologue(gdbarch*, CORE_ADDR, CORE_ADDR, tilegx_frame_cache*, frame_info*)': > .../gdb/tilegx-tdep.c:609: error: dereferencing pointer 'operands' does break strict-aliasing rules > .../gdb/tilegx-tdep.c:592: error: dereferencing pointer 'operands' does break strict-aliasing rules > .../gdb/tilegx-tdep.c:571: error: dereferencing pointer 'operands' does break strict-aliasing rules > [...] > .../gdb/tilegx-tdep.c:601: error: dereferencing pointer '<anonymous>' does break strict-aliasing rules > .../gdb/tilegx-tdep.c:601: note: initialized from here > cc1plus: error: dereferencing pointer 'operands' does break strict-aliasing rules > cc1plus: error: dereferencing pointer 'operands' does break strict-aliasing rules > .../gdb/tilegx-tdep.c:452: note: initialized from here > cc1plus: error: dereferencing pointer 'pretmp.896' does break strict-aliasing rules > cc1plus: note: initialized from here > cc1plus: error: dereferencing pointer 'pretmp.896' does break strict-aliasing rules > cc1plus: note: initialized from here > make[1]: *** [tilegx-tdep.o] Error 1 > > from an attempt to cast a `long long' pointer to an `int64_t' pointer, > which may not necessarily be compatible types. Use the `long long' type > for the auxiliary variable then as this is the type of the structure > member referred. > > gdb/ > * tilegx-tdep.c (tilegx_analyze_prologue): Use the `long long' > type for `operands'. > --- > Hi, > > This was discovered in an `--enable-targets=all' `mips-mti-linux-gnu' > build and may be dependent on the host type and compiler version, which > are `x86_64-linux' and 4.4.7 here, respectively. OK to apply? > Sure, please do ahead. I'd be better if tilegx.h used uint32_t/uint64_t, etc, but that can always be done separately by someone motivated. Thanks, Pedro Alves
On Mon, 17 Oct 2016, Pedro Alves wrote: > > Fix a load of aliasing build errors: > > > > cc1plus: warnings being treated as errors > > .../gdb/tilegx-tdep.c: In function 'CORE_ADDR tilegx_analyze_prologue(gdbarch*, CORE_ADDR, CORE_ADDR, tilegx_frame_cache*, frame_info*)': > > .../gdb/tilegx-tdep.c:609: error: dereferencing pointer 'operands' does break strict-aliasing rules > > .../gdb/tilegx-tdep.c:592: error: dereferencing pointer 'operands' does break strict-aliasing rules > > .../gdb/tilegx-tdep.c:571: error: dereferencing pointer 'operands' does break strict-aliasing rules > > [...] > > .../gdb/tilegx-tdep.c:601: error: dereferencing pointer '<anonymous>' does break strict-aliasing rules > > .../gdb/tilegx-tdep.c:601: note: initialized from here > > cc1plus: error: dereferencing pointer 'operands' does break strict-aliasing rules > > cc1plus: error: dereferencing pointer 'operands' does break strict-aliasing rules > > .../gdb/tilegx-tdep.c:452: note: initialized from here > > cc1plus: error: dereferencing pointer 'pretmp.896' does break strict-aliasing rules > > cc1plus: note: initialized from here > > cc1plus: error: dereferencing pointer 'pretmp.896' does break strict-aliasing rules > > cc1plus: note: initialized from here > > make[1]: *** [tilegx-tdep.o] Error 1 > > > > from an attempt to cast a `long long' pointer to an `int64_t' pointer, > > which may not necessarily be compatible types. Use the `long long' type > > for the auxiliary variable then as this is the type of the structure > > member referred. > > > > gdb/ > > * tilegx-tdep.c (tilegx_analyze_prologue): Use the `long long' > > type for `operands'. > > --- > > Hi, > > > > This was discovered in an `--enable-targets=all' `mips-mti-linux-gnu' > > build and may be dependent on the host type and compiler version, which > > are `x86_64-linux' and 4.4.7 here, respectively. OK to apply? > > > > Sure, please do ahead. > > I'd be better if tilegx.h used uint32_t/uint64_t, etc, > but that can always be done separately by someone motivated. The issue there is `tilegx.h' is shared with binutils which have not yet moved past C89 I believe. Of course the use of `long long' is already non-C89, however it's been there with some C89 compilers before `stdint.h' types have been introduced. We could use `bfd_uint64_t' instead, but that would make `tilegx.h' depend on a BFD header. So it's not an obvious call and therefore I agree it's best left to someone who has an actual interest with the target. Committed now, thanks for your review. Maciej
On 10/18/2016 04:50 AM, Maciej W. Rozycki wrote: >> > >> > Sure, please do ahead. >> > >> > I'd be better if tilegx.h used uint32_t/uint64_t, etc, >> > but that can always be done separately by someone motivated. > The issue there is `tilegx.h' is shared with binutils which have not yet > moved past C89 I believe. Of course the use of `long long' is already > non-C89, however it's been there with some C89 compilers before `stdint.h' > types have been introduced. We could use `bfd_uint64_t' instead, but that > would make `tilegx.h' depend on a BFD header. So it's not an obvious call > and therefore I agree it's best left to someone who has an actual interest > with the target. Yeah, C89 on paper. :-) It's certainly fine to use unadorned uint64_t etc. directly nowadays. Even if the host compiler is so old that it doesn't support those, we have "bfd_stdint.h" to fill the gap: opcodes/aarch64-dis.h:23:#include "bfd_stdint.h" opcodes/nds32-dis.c:30:#include "bfd_stdint.h" opcodes/aarch64-dis.c:22:#include "bfd_stdint.h" ld/elf-hints-local.h:28:#include "bfd_stdint.h" Something like 'grep "u\?int\(8\|16\|32\|64\)_t" -rn' will show many uses of uint32_t, etc. (not bfd_uint*_t) throughout opcodes, bfd, ld, etc. I won't be surprised at all if someone missed adding bfd_stdint.h for some of them and they've been working in practice for a long while. (better than bfd_stdint.h would be to use gnulib for the whole toolchain, not just for gdb, but that's yet another matter.) As I've been suggesting in recent C++11 discussions, IMO, what we claim to support in theory matters significantly less than what we actually support in practice. And in practice, all it matters is which compilers (and their versions), not standards, people care about actually using and supporting. Standards give us guidelines, not final answers. > Committed now, thanks for your review. Thanks. Thanks, Pedro Alves
Index: binutils/gdb/tilegx-tdep.c =================================================================== --- binutils.orig/gdb/tilegx-tdep.c 2016-10-05 00:58:09.000000000 +0100 +++ binutils/gdb/tilegx-tdep.c 2016-10-15 01:29:35.689998335 +0100 @@ -449,7 +449,7 @@ tilegx_analyze_prologue (struct gdbarch* for (i = 0; i < num_insns; i++) { struct tilegx_decoded_instruction *this_insn = &decoded[i]; - int64_t *operands = (int64_t *) this_insn->operand_values; + long long *operands = this_insn->operand_values; const struct tilegx_opcode *opcode = this_insn->opcode; switch (opcode->mnemonic)