* [PATCH] Handle loading improper core files gracefully in the mips backend. @ 2016-01-08 18:32 Luis Machado 2016-01-09 3:02 ` Maciej W. Rozycki 0 siblings, 1 reply; 19+ messages in thread From: Luis Machado @ 2016-01-08 18:32 UTC (permalink / raw) To: gdb-patches; +Cc: macro, jan.kratochvil bz 18964 was reported as a internal error from the mips backend when running gdb.arch/i386-biarch-core.exp. The backend gets confused and ends up picking up an invalid combination of 32-bit ISA and 64-bit ABI, resulting, later on, in an unexpected mismatch between the raw register size for PC (4) and the pseudo register size for PC (8). The point that makes the architecture selection go south is when we notice the core file is 64-bit, so we go with MIPS_ABI_N64, but we are using MIPS16 as the ISA. The attached patch adds a bit more logic to the incompatibility check during the architecture initialization. It doesn't completely solve the problem with this testcase though, but it gets rid of an ugly internal error and changes things from this: FAIL: gdb.arch/i386-biarch-core.exp: core-file (GDB internal error) === gdb Summary === to this: FAIL: gdb.arch/i386-biarch-core.exp: .text is readable === gdb Summary === Leaving this failure behind: x/bx 0x400078^M 0x400078: Cannot access memory at address 0x400078^M (gdb) FAIL: gdb.arch/i386-biarch-core.exp: .text is readable It is not clear to me what is expected of this particular test when executed for a non-x86 target. In any case, hardening of the mips backend seems to be an improvement already. Maciej, do you have more input on additional incompatibilities that we need to check for? gdb/ChangeLog: 2016-01-08 Luis Machado <lgustavo@codesourcery.com> bz 18964 * mips-tdep.c (mips_gdbarch_init): Handle additional incompatible ISA/ABI pairs gracefully and return. --- gdb/mips-tdep.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c index c8e12e8..9bdcf96 100644 --- a/gdb/mips-tdep.c +++ b/gdb/mips-tdep.c @@ -8206,6 +8206,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) enum mips_isa mips_isa; int dspacc; int dspctl; + /* Whether we have an unknown/unsuitable architecture description. */ + int arch_is_incompatible = 0; /* Fill in the OS dependent register numbers and names. */ if (info.osabi == GDB_OSABI_IRIX) @@ -8571,11 +8573,16 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) /* If we have only 32-bit registers, then we can't debug a 64-bit ABI. */ - if (info.target_desc - && tdesc_property (info.target_desc, PROPERTY_GP32) != NULL - && mips_abi != MIPS_ABI_EABI32 - && mips_abi != MIPS_ABI_O32) + if (mips_abi != MIPS_ABI_EABI32 && mips_abi != MIPS_ABI_O32 + && ((info.target_desc + && tdesc_property (info.target_desc, PROPERTY_GP32) != NULL) + || mips_isa == ISA_MIPS16)) + arch_is_incompatible = 1; + + if (arch_is_incompatible) { + /* This is an unsuitable combination of ABI/ISA. Just cleanup and + return. */ if (tdesc_data != NULL) tdesc_data_cleanup (tdesc_data); return NULL; -- 1.9.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-01-08 18:32 [PATCH] Handle loading improper core files gracefully in the mips backend Luis Machado @ 2016-01-09 3:02 ` Maciej W. Rozycki 2016-01-11 15:47 ` Luis Machado 0 siblings, 1 reply; 19+ messages in thread From: Maciej W. Rozycki @ 2016-01-09 3:02 UTC (permalink / raw) To: Luis Machado; +Cc: gdb-patches, jan.kratochvil On Fri, 8 Jan 2016, Luis Machado wrote: > Maciej, do you have more input on additional incompatibilities that we need > to check for? Why do we get this far in the first place where e_machine != EM_MIPS? I think rather than adding additional checks here (unless they're needed for a valid MIPS ELF binary; but that would be a separate problem to fix) we should reject such a core file outright. Maciej ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-01-09 3:02 ` Maciej W. Rozycki @ 2016-01-11 15:47 ` Luis Machado 2016-01-12 12:46 ` Pedro Alves 0 siblings, 1 reply; 19+ messages in thread From: Luis Machado @ 2016-01-11 15:47 UTC (permalink / raw) To: Maciej W. Rozycki; +Cc: gdb-patches, jan.kratochvil [-- Attachment #1: Type: text/plain, Size: 986 bytes --] On 01/09/2016 01:02 AM, Maciej W. Rozycki wrote: > On Fri, 8 Jan 2016, Luis Machado wrote: > >> Maciej, do you have more input on additional incompatibilities that we need >> to check for? > > Why do we get this far in the first place where e_machine != EM_MIPS? I > think rather than adding additional checks here (unless they're needed for > a valid MIPS ELF binary; but that would be a separate problem to fix) we > should reject such a core file outright. I thought about that, but no targets other than v850 use e_machine information during arch initialization, which is strange. It seems we just don't enforce compatibility, at least when loading core files. Does the attached patch match what you have in mind? I also adjusted the testcase to expect core file rejection and also got rid of duplicated test strings. Now a mips-targeted GDB says the following when trying to load a x86 core file: gdb.arch/i386-biarch-core.core": no core file handler recognizes format [-- Attachment #2: 0001-Handle-loading-improper-core-files-gracefully-in-the.patch --] [-- Type: text/x-patch, Size: 2699 bytes --] gdb/ChangeLog: 2016-01-11 Luis Machado <lgustavo@codesourcery.com> bz 18964 * mips-tdep.c (mips_gdbarch_init): Sanity check ELF e_machine field and bail out if it is not MIPS-compatible. gdb/testsuite/ChangeLog: 2016-01-11 Luis Machado <lgustavo@codesourcery.com> bz 18964 * gdb.arch/i376-biarch-core.exp: Handle the core file not being recognized. Use variables for repeating test names. --- gdb/mips-tdep.c | 6 ++++++ gdb/testsuite/gdb.arch/i386-biarch-core.exp | 18 +++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c index ca17864..cdfd80e 100644 --- a/gdb/mips-tdep.c +++ b/gdb/mips-tdep.c @@ -8208,6 +8208,12 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) int dspacc; int dspctl; + /* Sanity check the e_machine field. */ + if (info.abfd + && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour + && elf_elfheader (info.abfd)->e_machine != EM_MIPS) + return NULL; + /* Fill in the OS dependent register numbers and names. */ if (info.osabi == GDB_OSABI_IRIX) { diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.exp b/gdb/testsuite/gdb.arch/i386-biarch-core.exp index 607b947..a4c0541 100644 --- a/gdb/testsuite/gdb.arch/i386-biarch-core.exp +++ b/gdb/testsuite/gdb.arch/i386-biarch-core.exp @@ -28,13 +28,14 @@ gdb_start gdb_reinitialize_dir $srcdir/$subdir set test "complete set gnutarget" +set text_test ".text is readable" gdb_test_multiple "complete set gnutarget " $test { -re "set gnutarget elf64-little\r\n(.*\r\n)?$gdb_prompt $" { pass $test } -re "\r\n$gdb_prompt $" { pass $test - untested ".text is readable" + untested $text_test return } } @@ -62,8 +63,19 @@ if {$corestat(size) != 102400} { # objcopy as it corrupts the core file beyond all recognition. # The output therefore does not matter much, just we should not get GDB # internal error. -gdb_test "core-file ${corefile}" ".*" "core-file" +set test "core-file" +gdb_test_multiple "core-file ${corefile}" $test { + -re ".* no core file handler recognizes format\r\n$gdb_prompt $" { + # Make sure we handle cases where the core file isn't even properly + # loaded due to architecture incompatibilities. + untested $text_test + return + } + -re "\r\n$gdb_prompt $" { + pass $test + } +} # Test if at least the core file segments memory has been loaded. # https://bugzilla.redhat.com/show_bug.cgi?id=457187 -gdb_test "x/bx $address" "\r\n\[ \t\]*$address:\[ \t\]*0xf4\[ \t\]*" ".text is readable" +gdb_test "x/bx $address" "\r\n\[ \t\]*$address:\[ \t\]*0xf4\[ \t\]*" $text_test -- 1.9.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-01-11 15:47 ` Luis Machado @ 2016-01-12 12:46 ` Pedro Alves 2016-01-12 13:25 ` Luis Machado 0 siblings, 1 reply; 19+ messages in thread From: Pedro Alves @ 2016-01-12 12:46 UTC (permalink / raw) To: Luis Machado, Maciej W. Rozycki; +Cc: gdb-patches, jan.kratochvil On 01/11/2016 03:47 PM, Luis Machado wrote: > diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c > index ca17864..cdfd80e 100644 > --- a/gdb/mips-tdep.c > +++ b/gdb/mips-tdep.c > @@ -8208,6 +8208,12 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > int dspacc; > int dspctl; > > + /* Sanity check the e_machine field. */ > + if (info.abfd > + && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour > + && elf_elfheader (info.abfd)->e_machine != EM_MIPS) > + return NULL; This callback is registered for bfd_arch_mips: gdbarch_register (bfd_arch_mips, mips_gdbarch_init, mips_dump_tdep); Does bfd think this a bfd_arch_mips binary? How so? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-01-12 12:46 ` Pedro Alves @ 2016-01-12 13:25 ` Luis Machado 2016-01-12 14:10 ` Pedro Alves 0 siblings, 1 reply; 19+ messages in thread From: Luis Machado @ 2016-01-12 13:25 UTC (permalink / raw) To: Pedro Alves, Maciej W. Rozycki; +Cc: gdb-patches, jan.kratochvil On 01/12/2016 10:46 AM, Pedro Alves wrote: > On 01/11/2016 03:47 PM, Luis Machado wrote: >> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c >> index ca17864..cdfd80e 100644 >> --- a/gdb/mips-tdep.c >> +++ b/gdb/mips-tdep.c >> @@ -8208,6 +8208,12 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) >> int dspacc; >> int dspctl; >> >> + /* Sanity check the e_machine field. */ >> + if (info.abfd >> + && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour >> + && elf_elfheader (info.abfd)->e_machine != EM_MIPS) >> + return NULL; > > This callback is registered for bfd_arch_mips: > > gdbarch_register (bfd_arch_mips, mips_gdbarch_init, mips_dump_tdep); > > Does bfd think this a bfd_arch_mips binary? How so? In the second time we call gdbarch_info_fill, when opening the core file alone, we have this: p *info $8 = {bfd_arch_info = 0x0, byte_order = BFD_ENDIAN_UNKNOWN, byte_order_for_code = BFD_ENDIAN_UNKNOWN, abfd = 0xe1ce80, tdep_info = 0x0, osabi = GDB_OSABI_UNINITIALIZED, target_desc = 0x0} p *info->abfd->arch_info $10 = {bits_per_word = 32, bits_per_address = 32, bits_per_byte = 8, arch = bfd_arch_unknown, mach = 0, arch_name = 0x9b799f "unknown", printable_name = 0x9b799f "unknown", section_align_power = 2, the_default = 1, compatible = 0x78a592 <bfd_default_compatible>, scan = 0x78a60a <bfd_default_scan>, fill = 0x78acc6 <bfd_arch_default_fill>, next = 0x0} p *default_bfd_arch $12 = {bits_per_word = 32, bits_per_address = 32, bits_per_byte = 8, arch = bfd_arch_mips, mach = 0, arch_name = 0x9d98e0 "mips", printable_name = 0x9d98e0 "mips", section_align_power = 3, the_default = 1, compatible = 0x832b40 <mips_compatible>, scan = 0x78a60a <bfd_default_scan>, fill = 0x78acc6 <bfd_arch_default_fill>, next = 0x9d9b00 <arch_info_struct>} The data above leads gdbarch_info_fill to assign default_bfd_arch to info->bfd_arch_info here: /* From the default. */ if (info->bfd_arch_info == NULL) info->bfd_arch_info = default_bfd_arch; So the core file essentially turns into a mips-compatible core file. This also happens with a powerpc-targeted gdb and likely any other architecture. For powerpc we get lucky and end up "passing" this test because it has no fatal failing conditions. It ends up displaying frame -1 for me, like so: PC not available^M #-1 <unavailable> in ?? () ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-01-12 13:25 ` Luis Machado @ 2016-01-12 14:10 ` Pedro Alves 2016-01-12 15:43 ` Luis Machado 0 siblings, 1 reply; 19+ messages in thread From: Pedro Alves @ 2016-01-12 14:10 UTC (permalink / raw) To: Luis Machado, Maciej W. Rozycki; +Cc: gdb-patches, jan.kratochvil On 01/12/2016 01:25 PM, Luis Machado wrote: > On 01/12/2016 10:46 AM, Pedro Alves wrote: >> On 01/11/2016 03:47 PM, Luis Machado wrote: >>> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c >>> index ca17864..cdfd80e 100644 >>> --- a/gdb/mips-tdep.c >>> +++ b/gdb/mips-tdep.c >>> @@ -8208,6 +8208,12 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) >>> int dspacc; >>> int dspctl; >>> >>> + /* Sanity check the e_machine field. */ >>> + if (info.abfd >>> + && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour >>> + && elf_elfheader (info.abfd)->e_machine != EM_MIPS) >>> + return NULL; >> >> This callback is registered for bfd_arch_mips: >> >> gdbarch_register (bfd_arch_mips, mips_gdbarch_init, mips_dump_tdep); >> >> Does bfd think this a bfd_arch_mips binary? How so? > > In the second time we call gdbarch_info_fill, when opening the core file > alone, we have this: > > p *info > $8 = {bfd_arch_info = 0x0, byte_order = BFD_ENDIAN_UNKNOWN, > byte_order_for_code = BFD_ENDIAN_UNKNOWN, abfd = 0xe1ce80, tdep_info = > 0x0, osabi = GDB_OSABI_UNINITIALIZED, target_desc = 0x0} > > p *info->abfd->arch_info > $10 = {bits_per_word = 32, bits_per_address = 32, bits_per_byte = 8, > arch = bfd_arch_unknown, mach = 0, arch_name = 0x9b799f "unknown", > printable_name = 0x9b799f "unknown", section_align_power = 2, > the_default = 1, compatible = 0x78a592 <bfd_default_compatible>, > scan = 0x78a60a <bfd_default_scan>, fill = 0x78acc6 > <bfd_arch_default_fill>, next = 0x0} > > p *default_bfd_arch > $12 = {bits_per_word = 32, bits_per_address = 32, bits_per_byte = 8, > arch = bfd_arch_mips, mach = 0, arch_name = 0x9d98e0 "mips", > printable_name = 0x9d98e0 "mips", section_align_power = 3, the_default = > 1, compatible = 0x832b40 <mips_compatible>, > scan = 0x78a60a <bfd_default_scan>, fill = 0x78acc6 > <bfd_arch_default_fill>, next = 0x9d9b00 <arch_info_struct>} > > The data above leads gdbarch_info_fill to assign default_bfd_arch to > info->bfd_arch_info here: > > /* From the default. */ > if (info->bfd_arch_info == NULL) > info->bfd_arch_info = default_bfd_arch; > > So the core file essentially turns into a mips-compatible core file. Hmmm. I see. I think we can't really change this, given that there are formats that don't have an architecture. Like, e.g., srec: (gdb) file testsuite/gdb.base/intstr2.srec Reading symbols from testsuite/gdb.base/intstr2.srec...(no debugging symbols found)...done. I take it that a --enable-targets=all wouldn't fail like this? Also, sounds like you should be able to trigger these incompatibilities and assertion by loading a 32-bit MIPS binary and playing with "set mips abi n64/o64", etc? All in all, maybe your original patch that flagged incompatible abi/isa combination is the way to go? I also wonder whether the bfd arch detection couldn't be always compiled in, at least for elf. Why does bfd fail to detect that this is an bfd_arch_i386 file in the first place? > This also happens with a powerpc-targeted gdb and likely any other > architecture. > > For powerpc we get lucky and end up "passing" this test because it has > no fatal failing conditions. It ends up displaying frame -1 for me, like so: > > PC not available^M > #-1 <unavailable> in ?? () Which is obviously bogus. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-01-12 14:10 ` Pedro Alves @ 2016-01-12 15:43 ` Luis Machado 2016-01-12 16:00 ` Pedro Alves 2016-01-12 18:30 ` Maciej W. Rozycki 0 siblings, 2 replies; 19+ messages in thread From: Luis Machado @ 2016-01-12 15:43 UTC (permalink / raw) To: Pedro Alves, Maciej W. Rozycki; +Cc: gdb-patches, jan.kratochvil On 01/12/2016 12:10 PM, Pedro Alves wrote: > On 01/12/2016 01:25 PM, Luis Machado wrote: >> On 01/12/2016 10:46 AM, Pedro Alves wrote: >>> On 01/11/2016 03:47 PM, Luis Machado wrote: >>>> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c >>>> index ca17864..cdfd80e 100644 >>>> --- a/gdb/mips-tdep.c >>>> +++ b/gdb/mips-tdep.c >>>> @@ -8208,6 +8208,12 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) >>>> int dspacc; >>>> int dspctl; >>>> >>>> + /* Sanity check the e_machine field. */ >>>> + if (info.abfd >>>> + && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour >>>> + && elf_elfheader (info.abfd)->e_machine != EM_MIPS) >>>> + return NULL; >>> >>> This callback is registered for bfd_arch_mips: >>> >>> gdbarch_register (bfd_arch_mips, mips_gdbarch_init, mips_dump_tdep); >>> >>> Does bfd think this a bfd_arch_mips binary? How so? >> >> In the second time we call gdbarch_info_fill, when opening the core file >> alone, we have this: >> >> p *info >> $8 = {bfd_arch_info = 0x0, byte_order = BFD_ENDIAN_UNKNOWN, >> byte_order_for_code = BFD_ENDIAN_UNKNOWN, abfd = 0xe1ce80, tdep_info = >> 0x0, osabi = GDB_OSABI_UNINITIALIZED, target_desc = 0x0} >> >> p *info->abfd->arch_info >> $10 = {bits_per_word = 32, bits_per_address = 32, bits_per_byte = 8, >> arch = bfd_arch_unknown, mach = 0, arch_name = 0x9b799f "unknown", >> printable_name = 0x9b799f "unknown", section_align_power = 2, >> the_default = 1, compatible = 0x78a592 <bfd_default_compatible>, >> scan = 0x78a60a <bfd_default_scan>, fill = 0x78acc6 >> <bfd_arch_default_fill>, next = 0x0} >> >> p *default_bfd_arch >> $12 = {bits_per_word = 32, bits_per_address = 32, bits_per_byte = 8, >> arch = bfd_arch_mips, mach = 0, arch_name = 0x9d98e0 "mips", >> printable_name = 0x9d98e0 "mips", section_align_power = 3, the_default = >> 1, compatible = 0x832b40 <mips_compatible>, >> scan = 0x78a60a <bfd_default_scan>, fill = 0x78acc6 >> <bfd_arch_default_fill>, next = 0x9d9b00 <arch_info_struct>} >> >> The data above leads gdbarch_info_fill to assign default_bfd_arch to >> info->bfd_arch_info here: >> >> /* From the default. */ >> if (info->bfd_arch_info == NULL) >> info->bfd_arch_info = default_bfd_arch; >> >> So the core file essentially turns into a mips-compatible core file. > > Hmmm. I see. I think we can't really change this, given that there > are formats that don't have an architecture. Like, e.g., srec: > > (gdb) file testsuite/gdb.base/intstr2.srec > Reading symbols from testsuite/gdb.base/intstr2.srec...(no debugging symbols found)...done. > > I take it that a --enable-targets=all wouldn't fail like this? > Yes, because, at least in my case, we default to the proper i386 architecture. > Also, sounds like you should be able to trigger these incompatibilities > and assertion by loading a 32-bit MIPS binary and playing with > "set mips abi n64/o64", etc? > Yes, most likely, but see below. > All in all, maybe your original patch that flagged incompatible > abi/isa combination is the way to go? > > I also wonder whether the bfd arch detection couldn't be always > compiled in, at least for elf. Why does bfd fail to detect that this > is an bfd_arch_i386 file in the first place? It seems bfd also falls back to the default, which is mips in this case. p bfd_default_vector[0] $3 = (const bfd_target *) 0x9beac0 <mips_elf32_trad_be_vec> I gave it a try with a legitimate x86 core file being loaded in a mips-targeted gdb and i see the same problem with the internal error. Initially, when loading the core, the bfd arch is unknown, and then we pick the default arch in bfd_find_target here: /* This is safe; the vector cannot be null. */ if (targname == NULL || strcmp (targname, "default") == 0) { if (bfd_default_vector[0] != NULL) target = bfd_default_vector[0]; else target = bfd_target_vector[0]; if (abfd) { abfd->xvec = target; abfd->target_defaulted = TRUE; } return target; } Sounds like we have a couple issues. The mips backend not handling weird abi/isa combinations and GDB not preventing clearly incompatible core files from proceeding further into processing in the target's backend? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-01-12 15:43 ` Luis Machado @ 2016-01-12 16:00 ` Pedro Alves 2016-01-12 18:30 ` Maciej W. Rozycki 1 sibling, 0 replies; 19+ messages in thread From: Pedro Alves @ 2016-01-12 16:00 UTC (permalink / raw) To: Luis Machado, Maciej W. Rozycki; +Cc: gdb-patches, jan.kratochvil On 01/12/2016 03:43 PM, Luis Machado wrote: > Sounds like we have a couple issues. The mips backend not handling weird > abi/isa combinations and GDB not preventing clearly incompatible core > files from proceeding further into processing in the target's backend? Sounds like it. Not sure whether the blame for the latter is BFD's or GDB's though. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-01-12 15:43 ` Luis Machado 2016-01-12 16:00 ` Pedro Alves @ 2016-01-12 18:30 ` Maciej W. Rozycki 2016-01-12 19:08 ` Pedro Alves ` (2 more replies) 1 sibling, 3 replies; 19+ messages in thread From: Maciej W. Rozycki @ 2016-01-12 18:30 UTC (permalink / raw) To: Luis Machado; +Cc: Pedro Alves, gdb-patches, jan.kratochvil On Tue, 12 Jan 2016, Luis Machado wrote: > > > The data above leads gdbarch_info_fill to assign default_bfd_arch to > > > info->bfd_arch_info here: > > > > > > /* From the default. */ > > > if (info->bfd_arch_info == NULL) > > > info->bfd_arch_info = default_bfd_arch; > > > > > > So the core file essentially turns into a mips-compatible core file. > > > > Hmmm. I see. I think we can't really change this, given that there > > are formats that don't have an architecture. Like, e.g., srec: > > > > (gdb) file testsuite/gdb.base/intstr2.srec > > Reading symbols from testsuite/gdb.base/intstr2.srec...(no debugging > > symbols found)...done. Or we could be talking to a live target with no executable selected at all. This is also why there are settings like `set mips abi ...' available -- to let the user select the executable model for a target there's no other source of information about. > > I also wonder whether the bfd arch detection couldn't be always > > compiled in, at least for elf. Why does bfd fail to detect that this > > is an bfd_arch_i386 file in the first place? The mapping between `e_machine' and `bfd_architecture' is only provided by individual BFD ELF target backends, via the ELF_MACHINE_CODE and ELF_ARCH macros. > It seems bfd also falls back to the default, which is mips in this case. > > p bfd_default_vector[0] > $3 = (const bfd_target *) 0x9beac0 <mips_elf32_trad_be_vec> Regardless, I'd expect a suitable generic ELF BFD target to be selected, which is what AFAICT `bfd_check_format' does. It is called by our `core_open' function and has a `core_file_p' handler, which makes suitable checks including `e_machine' in particular, except for generic ELF BFD targets, which are special-cased (and always come last). So in the absence of specific ELF target support in BFD I'd expect a compatible generic ELF target to be chosen rather than the default BFD target, which might be incompatible. > Sounds like we have a couple issues. The mips backend not handling weird > abi/isa combinations and GDB not preventing clearly incompatible core files > from proceeding further into processing in the target's backend? I have given it some thought and came to a conclusion that we should at least try being consistent. Which means I think we should not try to handle files within the MIPS backend which would not be passed in the first place in an `--enable-targets=all' configuration. Rather than checking `e_machine' explicitly I'd be leaning towards using BFD to detect such a situation though, perhaps by using a condition like if (info.abfd != NULL && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour && bfd_get_arch (info.abfd) != bfd_arch_mips) return NULL; (maybe with an additional error message) though ultimately I think it would make sense to define different BFD architecture codes for file formats which by definition carry no architecture information and for ones that do but are not supported. Then for the formers we could continue selecting the target using the current algorithm and for the latters we'd just reject them as incompatible with the given backend -- all somewhere in generic code so that individual target backends do not have to repeat it all. As to ABI, ISA, etc. settings -- these are internal to the MIPS backend, so its the backend's job to sanitise them. Maciej ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-01-12 18:30 ` Maciej W. Rozycki @ 2016-01-12 19:08 ` Pedro Alves 2016-02-02 12:58 ` Luis Machado 2017-01-09 19:57 ` Luis Machado 2 siblings, 0 replies; 19+ messages in thread From: Pedro Alves @ 2016-01-12 19:08 UTC (permalink / raw) To: Maciej W. Rozycki, Luis Machado; +Cc: gdb-patches, jan.kratochvil On 01/12/2016 06:30 PM, Maciej W. Rozycki wrote: > On Tue, 12 Jan 2016, Luis Machado wrote: >> Pedro Alves wrote: > >>> I also wonder whether the bfd arch detection couldn't be always >>> compiled in, at least for elf. Why does bfd fail to detect that this >>> is an bfd_arch_i386 file in the first place? > > The mapping between `e_machine' and `bfd_architecture' is only provided > by individual BFD ELF target backends, via the ELF_MACHINE_CODE and > ELF_ARCH macros. Thanks. In principle, it sounds to me that at least the ELF_MACHINE_CODE -> bfd_architecture sniffing bits could be factored out and always be present. But, that might not be practical. >> Sounds like we have a couple issues. The mips backend not handling weird >> abi/isa combinations and GDB not preventing clearly incompatible core files >> from proceeding further into processing in the target's backend? > > I have given it some thought and came to a conclusion that we should at > least try being consistent. Which means I think we should not try to > handle files within the MIPS backend which would not be passed in the > first place in an `--enable-targets=all' configuration. Rather than > checking `e_machine' explicitly I'd be leaning towards using BFD to detect > such a situation though, perhaps by using a condition like > > if (info.abfd != NULL > && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour > && bfd_get_arch (info.abfd) != bfd_arch_mips) > return NULL; > > (maybe with an additional error message) though ultimately I think it > would make sense to define different BFD architecture codes for file > formats which by definition carry no architecture information and for ones > that do but are not supported. Agreed. Seems like that could be the job of bfd_arch_obscure -- it's used as default/unhandled case in some formats that do have architecture information. Though it isn't used throughout all bfd backends. > Then for the formers we could continue > selecting the target using the current algorithm and for the latters we'd > just reject them as incompatible with the given backend -- all somewhere > in generic code so that individual target backends do not have to repeat > it all. > > As to ABI, ISA, etc. settings -- these are internal to the MIPS backend, > so its the backend's job to sanitise them. /me nods. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-01-12 18:30 ` Maciej W. Rozycki 2016-01-12 19:08 ` Pedro Alves @ 2016-02-02 12:58 ` Luis Machado 2016-02-02 14:19 ` Pedro Alves 2017-01-09 19:57 ` Luis Machado 2 siblings, 1 reply; 19+ messages in thread From: Luis Machado @ 2016-02-02 12:58 UTC (permalink / raw) To: Maciej W. Rozycki; +Cc: Pedro Alves, gdb-patches, jan.kratochvil On 01/12/2016 04:30 PM, Maciej W. Rozycki wrote: > On Tue, 12 Jan 2016, Luis Machado wrote: >>> I also wonder whether the bfd arch detection couldn't be always >>> compiled in, at least for elf. Why does bfd fail to detect that this >>> is an bfd_arch_i386 file in the first place? > > The mapping between `e_machine' and `bfd_architecture' is only provided > by individual BFD ELF target backends, via the ELF_MACHINE_CODE and > ELF_ARCH macros. > >> It seems bfd also falls back to the default, which is mips in this case. >> >> p bfd_default_vector[0] >> $3 = (const bfd_target *) 0x9beac0 <mips_elf32_trad_be_vec> > > Regardless, I'd expect a suitable generic ELF BFD target to be selected, > which is what AFAICT `bfd_check_format' does. It is called by our > `core_open' function and has a `core_file_p' handler, which makes suitable > checks including `e_machine' in particular, except for generic ELF BFD > targets, which are special-cased (and always come last). So in the > absence of specific ELF target support in BFD I'd expect a compatible > generic ELF target to be chosen rather than the default BFD target, which > might be incompatible. > Ah, indeed this is the case. We switch to a generic ELF target during bfd_check_format. So that is working as it should. >> Sounds like we have a couple issues. The mips backend not handling weird >> abi/isa combinations and GDB not preventing clearly incompatible core files >> from proceeding further into processing in the target's backend? > > I have given it some thought and came to a conclusion that we should at > least try being consistent. Which means I think we should not try to > handle files within the MIPS backend which would not be passed in the > first place in an `--enable-targets=all' configuration. Rather than > checking `e_machine' explicitly I'd be leaning towards using BFD to detect > such a situation though, perhaps by using a condition like > > if (info.abfd != NULL > && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour > && bfd_get_arch (info.abfd) != bfd_arch_mips) > return NULL; > > (maybe with an additional error message) though ultimately I think it > would make sense to define different BFD architecture codes for file > formats which by definition carry no architecture information and for ones > that do but are not supported. Then for the formers we could continue > selecting the target using the current algorithm and for the latters we'd > just reject them as incompatible with the given backend -- all somewhere > in generic code so that individual target backends do not have to repeat > it all. Though the above doesn't solve the bigger picture, it gets rid of the internal error when loading the incompatible core file. Should we go ahead and have this additional check committed? Luis ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-02-02 12:58 ` Luis Machado @ 2016-02-02 14:19 ` Pedro Alves 2016-02-02 14:22 ` Pedro Alves 0 siblings, 1 reply; 19+ messages in thread From: Pedro Alves @ 2016-02-02 14:19 UTC (permalink / raw) To: Luis Machado, Maciej W. Rozycki; +Cc: gdb-patches, jan.kratochvil On 02/02/2016 12:58 PM, Luis Machado wrote: > On 01/12/2016 04:30 PM, Maciej W. Rozycki wrote: >> On Tue, 12 Jan 2016, Luis Machado wrote: >>>> I also wonder whether the bfd arch detection couldn't be always >>>> compiled in, at least for elf. Why does bfd fail to detect that this >>>> is an bfd_arch_i386 file in the first place? >> >> The mapping between `e_machine' and `bfd_architecture' is only provided >> by individual BFD ELF target backends, via the ELF_MACHINE_CODE and >> ELF_ARCH macros. >> >>> It seems bfd also falls back to the default, which is mips in this case. >>> >>> p bfd_default_vector[0] >>> $3 = (const bfd_target *) 0x9beac0 <mips_elf32_trad_be_vec> >> >> Regardless, I'd expect a suitable generic ELF BFD target to be selected, >> which is what AFAICT `bfd_check_format' does. It is called by our >> `core_open' function and has a `core_file_p' handler, which makes suitable >> checks including `e_machine' in particular, except for generic ELF BFD >> targets, which are special-cased (and always come last). So in the >> absence of specific ELF target support in BFD I'd expect a compatible >> generic ELF target to be chosen rather than the default BFD target, which >> might be incompatible. >> > > Ah, indeed this is the case. We switch to a generic ELF target during > bfd_check_format. So that is working as it should. > >>> Sounds like we have a couple issues. The mips backend not handling weird >>> abi/isa combinations and GDB not preventing clearly incompatible core files >>> from proceeding further into processing in the target's backend? >> >> I have given it some thought and came to a conclusion that we should at >> least try being consistent. Which means I think we should not try to >> handle files within the MIPS backend which would not be passed in the >> first place in an `--enable-targets=all' configuration. Rather than >> checking `e_machine' explicitly I'd be leaning towards using BFD to detect >> such a situation though, perhaps by using a condition like >> >> if (info.abfd != NULL >> && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour >> && bfd_get_arch (info.abfd) != bfd_arch_mips) >> return NULL; >> >> (maybe with an additional error message) though ultimately I think it >> would make sense to define different BFD architecture codes for file >> formats which by definition carry no architecture information and for ones >> that do but are not supported. Then for the formers we could continue >> selecting the target using the current algorithm and for the latters we'd >> just reject them as incompatible with the given backend -- all somewhere >> in generic code so that individual target backends do not have to repeat >> it all. > > Though the above doesn't solve the bigger picture, it gets rid of the > internal error when loading the incompatible core file. > > Should we go ahead and have this additional check committed? Did you try to trigger the assertion by loading a 32-bit MIPS binary into gdb, and playing with "set mips abi n64/o64...", "set mipsfpu", etc? I think that adding a test to the testsuite that iterates through all the possible combinations just to make sure gdb doesn't crash would be great, and also show that the patch stands on its own as well, irrespective of the bfd arch compatibility issues. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-02-02 14:19 ` Pedro Alves @ 2016-02-02 14:22 ` Pedro Alves 2016-02-04 21:01 ` Maciej W. Rozycki 0 siblings, 1 reply; 19+ messages in thread From: Pedro Alves @ 2016-02-02 14:22 UTC (permalink / raw) To: Luis Machado, Maciej W. Rozycki; +Cc: gdb-patches, jan.kratochvil On 02/02/2016 02:19 PM, Pedro Alves wrote: > On 02/02/2016 12:58 PM, Luis Machado wrote: > Did you try to trigger the assertion by loading a 32-bit MIPS binary > into gdb, and playing with "set mips abi n64/o64...", "set mipsfpu", > etc? > > I think that adding a test to the testsuite that iterates through all > the possible combinations just to make sure gdb doesn't crash > would be great, and also show that the patch stands on its own > as well, irrespective of the bfd arch compatibility issues. TBC, I meant, the original patch that checked unsuitable ABI/ISA combinations. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-02-02 14:22 ` Pedro Alves @ 2016-02-04 21:01 ` Maciej W. Rozycki 2016-02-05 11:29 ` Luis Machado 0 siblings, 1 reply; 19+ messages in thread From: Maciej W. Rozycki @ 2016-02-04 21:01 UTC (permalink / raw) To: Pedro Alves; +Cc: Luis Machado, gdb-patches, Jan Kratochvil On Tue, 2 Feb 2016, Pedro Alves wrote: > > Did you try to trigger the assertion by loading a 32-bit MIPS binary > > into gdb, and playing with "set mips abi n64/o64...", "set mipsfpu", > > etc? > > > > I think that adding a test to the testsuite that iterates through all > > the possible combinations just to make sure gdb doesn't crash > > would be great, and also show that the patch stands on its own > > as well, irrespective of the bfd arch compatibility issues. > > TBC, I meant, the original patch that checked unsuitable ABI/ISA > combinations. NB I can only see the use for these knobs to deal with two situations: 1. There's no executable and we want to connect to a live target for minimal binary-only/disasembly-level debugging. We need to set the endianness, ABI, ISA, etc. to match the target then (although arguably at least the endianness should be supplied by the debug stub somehow; we just don't have a way defined right now). 2. We have a buggy or outdated GDB executable which fails to determine the right settings automatically. As it looks for example as recently as last week I came across a problem where GDB failed to detect the presence of the FPU under Linux for some reason. I had to forcefully request `set mipsfpu double' to override the wrongly chosen setting of `none', at which point accesses to the FPU worked as expected, so it wasn't like the unit was inaccessible. Obviously I need to debug #2, but overall I agree we ought to bail out gracefully rather than crash if the manual settings lead to a nonsensical configuration. Therefore I went back to the original patch now. It obscures things a bit I'm afraid from my point of view as it combines a syntactical change (the addition of the `arch_is_incompatible' auxiliary variable) and a semantical change (the addition of the `mips_isa == ISA_MIPS16' check), so I'd like to see the two changes separated. TBH I'm not convinced whether the auxiliary variable buys us anything here -- it doesn't serve as documentation as we have an explanatory comment here already, which BTW needs to be updated accordingly if the condition is extended to cover an ISA incompatibility. As to which, and more importantly -- there is no actual architectural incompatibility between the n64 (or n32) ABI and the MIPS16 instruction set; there are 64-bit MIPS processors in existence which implement the MIPS16 ISA as well, e.g. the NEC VR4111, and the ISA itself includes 64-bit instructions on such a processor. So the MIPS16 ISA is really agnostic to the ABI, just as is the regular MIPS ISA or the microMIPS ISA. Therefore any such fix needs to go elsewhere I'm afraid -- we probably do something outright silly for the ISA_MIPS16 setting. Maciej ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-02-04 21:01 ` Maciej W. Rozycki @ 2016-02-05 11:29 ` Luis Machado 2016-02-05 14:10 ` Maciej W. Rozycki 0 siblings, 1 reply; 19+ messages in thread From: Luis Machado @ 2016-02-05 11:29 UTC (permalink / raw) To: Maciej W. Rozycki, Pedro Alves; +Cc: gdb-patches, Jan Kratochvil On 02/04/2016 07:01 PM, Maciej W. Rozycki wrote: > On Tue, 2 Feb 2016, Pedro Alves wrote: > >>> Did you try to trigger the assertion by loading a 32-bit MIPS binary >>> into gdb, and playing with "set mips abi n64/o64...", "set mipsfpu", >>> etc? >>> >>> I think that adding a test to the testsuite that iterates through all >>> the possible combinations just to make sure gdb doesn't crash >>> would be great, and also show that the patch stands on its own >>> as well, irrespective of the bfd arch compatibility issues. I was playing around with these settings. >> >> TBC, I meant, the original patch that checked unsuitable ABI/ISA >> combinations. > > NB I can only see the use for these knobs to deal with two situations: > > 1. There's no executable and we want to connect to a live target for > minimal binary-only/disasembly-level debugging. We need to set the > endianness, ABI, ISA, etc. to match the target then (although arguably > at least the endianness should be supplied by the debug stub somehow; > we just don't have a way defined right now). > While trying reproducers out, i noticed this use case doesn't seem to work properly under some conditions anymore. Whenever GDB doesn't find a binary and sysroot is set to empty, it will not attempt to continue with the remote session. It seems to just give up. Sending packet: $qXfer:exec-file:read:6394:0,fff#60...Packet received: lgdb.base/break <- remote->to_xfer_partial (0xcb3a80, 27, 6394, 0xe38cc0, 0x0, 0x0, 0xfff, 0x98) = 1 remote:target_xfer_partial (27, 6394, 0xe38cc0, 0x0, 0x0, 4095) = 1, 152, bytes = 2f 6e 65 74 2f 62 75 69 6c 64 32 2d 6c 75 63 69 ... -> remote->to_check_pending_interrupt (...) <- remote->to_check_pending_interrupt (0xcb3a80) -> remote->to_xfer_partial (...) <- remote->to_xfer_partial (0xcb3a80, 27, 6394, 0xe38d58, 0x0, 0x98, 0xf67, 0x0) = 0 remote:target_xfer_partial (27, 6394, 0xe38d58, 0x0, 0x98, 3943) = 0, 0 <- remote->to_pid_to_exec_file (0xcb3a80, 25492) = gdb.base/break target_close () gdb.base/break: No such file or directory. (gdb) i r The program has no registers now. (gdb) kill The program is not being run. Otherwise gdbserver will transfer the file over from the remote end. But i digress. I can easily reproduce the internal error by simply loading a 32-bit MIPS binary and flipping the abi to any of the 64-bit variants. This doesn't seem to be terribly important as people interested in playing with these setting will most likely know what they're doing. The testcase causing an internal error seems to be even less important and very unlikely to occur, but it always runs as part of the testsuite and it is a bit of an annoyance. GDB should not give an internal error or crash, obviously. > 2. We have a buggy or outdated GDB executable which fails to determine the > right settings automatically. As it looks for example as recently as > last week I came across a problem where GDB failed to detect the > presence of the FPU under Linux for some reason. I had to forcefully > request `set mipsfpu double' to override the wrongly chosen setting of > `none', at which point accesses to the FPU worked as expected, so it > wasn't like the unit was inaccessible. > > Obviously I need to debug #2, but overall I agree we ought to bail out > gracefully rather than crash if the manual settings lead to a nonsensical > configuration. > > Therefore I went back to the original patch now. It obscures things a > bit I'm afraid from my point of view as it combines a syntactical change > (the addition of the `arch_is_incompatible' auxiliary variable) and a > semantical change (the addition of the `mips_isa == ISA_MIPS16' check), so > I'd like to see the two changes separated. > > TBH I'm not convinced whether the auxiliary variable buys us anything > here -- it doesn't serve as documentation as we have an explanatory > comment here already, which BTW needs to be updated accordingly if the > condition is extended to cover an ISA incompatibility. The naming could've been better. I went that route in the hopes that future checks would just flip that boolean while keeping the conditional block separate, otherwise we would have a bigger conditional block that may not be as straightforward to parse. > > As to which, and more importantly -- there is no actual architectural > incompatibility between the n64 (or n32) ABI and the MIPS16 instruction > set; there are 64-bit MIPS processors in existence which implement the > MIPS16 ISA as well, e.g. the NEC VR4111, and the ISA itself includes > 64-bit instructions on such a processor. So the MIPS16 ISA is really > agnostic to the ABI, just as is the regular MIPS ISA or the microMIPS ISA. > Therefore any such fix needs to go elsewhere I'm afraid -- we probably do > something outright silly for the ISA_MIPS16 setting. Fair enough. Do you have a suggestion on where that fix should go to? The culprit seems to be the mix of an arch selection that gives us 64-bit cooked registers (due to mips_abi_regsize) and an ISA that gives us 32-bit registers (due to mips_isa_regsize). With that combination, mips_pseudo_register_read will fail in a fatal way, as well as mips_pseudo_register_write if we ever manage to go past the reading step. Luis ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-02-05 11:29 ` Luis Machado @ 2016-02-05 14:10 ` Maciej W. Rozycki 0 siblings, 0 replies; 19+ messages in thread From: Maciej W. Rozycki @ 2016-02-05 14:10 UTC (permalink / raw) To: Luis Machado; +Cc: Pedro Alves, gdb-patches, Jan Kratochvil On Fri, 5 Feb 2016, Luis Machado wrote: > > 1. There's no executable and we want to connect to a live target for > > minimal binary-only/disasembly-level debugging. We need to set the > > endianness, ABI, ISA, etc. to match the target then (although arguably > > at least the endianness should be supplied by the debug stub somehow; > > we just don't have a way defined right now). > > > > While trying reproducers out, i noticed this use case doesn't seem to work > properly under some conditions anymore. Whenever GDB doesn't find a binary and > sysroot is set to empty, it will not attempt to continue with the remote > session. It seems to just give up. > > Sending packet: $qXfer:exec-file:read:6394:0,fff#60...Packet received: > lgdb.base/break > <- remote->to_xfer_partial (0xcb3a80, 27, 6394, 0xe38cc0, 0x0, 0x0, 0xfff, > 0x98) = 1 > remote:target_xfer_partial (27, 6394, 0xe38cc0, 0x0, 0x0, 4095) = 1, 152, > bytes = > 2f 6e 65 74 2f 62 75 69 6c 64 32 2d 6c 75 63 69 ... > -> remote->to_check_pending_interrupt (...) > <- remote->to_check_pending_interrupt (0xcb3a80) > -> remote->to_xfer_partial (...) > <- remote->to_xfer_partial (0xcb3a80, 27, 6394, 0xe38d58, 0x0, 0x98, 0xf67, > 0x0) = 0 > remote:target_xfer_partial (27, 6394, 0xe38d58, 0x0, 0x98, 3943) = 0, 0 > <- remote->to_pid_to_exec_file (0xcb3a80, 25492) = gdb.base/break > target_close () > gdb.base/break: No such file or directory. > (gdb) i r > The program has no registers now. > (gdb) kill > The program is not being run. > > Otherwise gdbserver will transfer the file over from the remote end. But i > digress. Thanks for letting me know, I'll have a look -- this might be an issue with gdbserver rather than GDB proper. Actually I have never tried this scenario with gdbserver, my usual case was running through board firmware over JTAG and a third-party debug stub talking to it. Or, in the very old days, the MDI target I posted a while ago (<https://sourceware.org/ml/gdb-patches/2008-02/msg00439.html>). > I can easily reproduce the internal error by simply loading a 32-bit MIPS > binary and flipping the abi to any of the 64-bit variants. I think I can see where this can happen. We have this condition in `mips_gdbarch_init' to catch this situation: /* If we have only 32-bit registers, then we can't debug a 64-bit ABI. */ if (info.target_desc && tdesc_property (info.target_desc, PROPERTY_GP32) != NULL && mips_abi != MIPS_ABI_EABI32 && mips_abi != MIPS_ABI_O32) -- however it works in positive logic, that is only if we have a valid target description and that description is wrong, and doing nothing if we don't. However `mips_isa_regsize' also has fallback logic, specifically: /* Fall back to the previous behavior. */ return (gdbarch_bfd_arch_info (gdbarch)->bits_per_word / gdbarch_bfd_arch_info (gdbarch)->bits_per_byte); So I think in `mips_gdbarch_init' we need to incorporate a corresponding check and reject any BFD arch which implies an incompatible register size; I think irrespectively of whether we have a target description or not. We have the necessary bits readily available here AFAICT. > This doesn't seem to be terribly important as people interested in playing > with these setting will most likely know what they're doing. > > The testcase causing an internal error seems to be even less important and > very unlikely to occur, but it always runs as part of the testsuite and it is > a bit of an annoyance. > > GDB should not give an internal error or crash, obviously. Oh absolutely, no argument about this as far as I'm concerned. I'd even say stronger that it must not! > > TBH I'm not convinced whether the auxiliary variable buys us anything > > here -- it doesn't serve as documentation as we have an explanatory > > comment here already, which BTW needs to be updated accordingly if the > > condition is extended to cover an ISA incompatibility. > > The naming could've been better. I went that route in the hopes that future > checks would just flip that boolean while keeping the conditional block > separate, otherwise we would have a bigger conditional block that may not be > as straightforward to parse. Fair enough -- right now we only have this single `if' statement, but if we have a separate knob to be driven, as you're proposing, then it'll make it easier, and therefore might encourage people to keep it clean if it tunrs out the control needs to be more complex, e.g. by using a `switch' statement if needed. So OK, I'm not opposing it -- let's just make it separate from the fix to the original issue. > > As to which, and more importantly -- there is no actual architectural > > incompatibility between the n64 (or n32) ABI and the MIPS16 instruction > > set; there are 64-bit MIPS processors in existence which implement the > > MIPS16 ISA as well, e.g. the NEC VR4111, and the ISA itself includes > > 64-bit instructions on such a processor. So the MIPS16 ISA is really > > agnostic to the ABI, just as is the regular MIPS ISA or the microMIPS ISA. > > Therefore any such fix needs to go elsewhere I'm afraid -- we probably do > > something outright silly for the ISA_MIPS16 setting. > > Fair enough. Do you have a suggestion on where that fix should go to? None offhand, I'll see if I can have a look soon. With my observation above about `mips_gdbarch_init' vs `mips_isa_regsize' I think this is a separate bug though, probably in BFD. > The culprit seems to be the mix of an arch selection that gives us 64-bit > cooked registers (due to mips_abi_regsize) and an ISA that gives us 32-bit > registers (due to mips_isa_regsize). With that combination, > mips_pseudo_register_read will fail in a fatal way, as well as > mips_pseudo_register_write if we ever manage to go past the reading step. Correct, however setting the MIPS16 ISA (or microMIPS, for that matter, as this is analogous) shouldn't affect `mips_isa_regsize'. That just seems plain wrong to me. I suspect this might be just some historical baggage, needing cleaning up. Maciej ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2016-01-12 18:30 ` Maciej W. Rozycki 2016-01-12 19:08 ` Pedro Alves 2016-02-02 12:58 ` Luis Machado @ 2017-01-09 19:57 ` Luis Machado 2017-01-19 16:56 ` Pedro Alves 2 siblings, 1 reply; 19+ messages in thread From: Luis Machado @ 2017-01-09 19:57 UTC (permalink / raw) To: Maciej W. Rozycki; +Cc: Pedro Alves, gdb-patches, jan.kratochvil On 01/12/2016 12:30 PM, Maciej W. Rozycki wrote: > On Tue, 12 Jan 2016, Luis Machado wrote: > >>>> The data above leads gdbarch_info_fill to assign default_bfd_arch to >>>> info->bfd_arch_info here: >>>> >>>> /* From the default. */ >>>> if (info->bfd_arch_info == NULL) >>>> info->bfd_arch_info = default_bfd_arch; >>>> >>>> So the core file essentially turns into a mips-compatible core file. >>> >>> Hmmm. I see. I think we can't really change this, given that there >>> are formats that don't have an architecture. Like, e.g., srec: >>> >>> (gdb) file testsuite/gdb.base/intstr2.srec >>> Reading symbols from testsuite/gdb.base/intstr2.srec...(no debugging >>> symbols found)...done. > > Or we could be talking to a live target with no executable selected at > all. This is also why there are settings like `set mips abi ...' > available -- to let the user select the executable model for a target > there's no other source of information about. > >>> I also wonder whether the bfd arch detection couldn't be always >>> compiled in, at least for elf. Why does bfd fail to detect that this >>> is an bfd_arch_i386 file in the first place? > > The mapping between `e_machine' and `bfd_architecture' is only provided > by individual BFD ELF target backends, via the ELF_MACHINE_CODE and > ELF_ARCH macros. > >> It seems bfd also falls back to the default, which is mips in this case. >> >> p bfd_default_vector[0] >> $3 = (const bfd_target *) 0x9beac0 <mips_elf32_trad_be_vec> > > Regardless, I'd expect a suitable generic ELF BFD target to be selected, > which is what AFAICT `bfd_check_format' does. It is called by our > `core_open' function and has a `core_file_p' handler, which makes suitable > checks including `e_machine' in particular, except for generic ELF BFD > targets, which are special-cased (and always come last). So in the > absence of specific ELF target support in BFD I'd expect a compatible > generic ELF target to be chosen rather than the default BFD target, which > might be incompatible. > >> Sounds like we have a couple issues. The mips backend not handling weird >> abi/isa combinations and GDB not preventing clearly incompatible core files >> from proceeding further into processing in the target's backend? > > I have given it some thought and came to a conclusion that we should at > least try being consistent. Which means I think we should not try to > handle files within the MIPS backend which would not be passed in the > first place in an `--enable-targets=all' configuration. Rather than > checking `e_machine' explicitly I'd be leaning towards using BFD to detect > such a situation though, perhaps by using a condition like > > if (info.abfd != NULL > && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour > && bfd_get_arch (info.abfd) != bfd_arch_mips) > return NULL; > > (maybe with an additional error message) though ultimately I think it > would make sense to define different BFD architecture codes for file > formats which by definition carry no architecture information and for ones > that do but are not supported. Then for the formers we could continue > selecting the target using the current algorithm and for the latters we'd > just reject them as incompatible with the given backend -- all somewhere > in generic code so that individual target backends do not have to repeat > it all. > > As to ABI, ISA, etc. settings -- these are internal to the MIPS backend, > so its the backend's job to sanitise them. > > Maciej > After quite a while, i'm revisiting this one. Reading the thread once again, is my understanding correct that the first patch is more suitable now? Possibly with some cleanups? https://sourceware.org/ml/gdb-patches/2016-01/msg00134.html Regards, Luis ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2017-01-09 19:57 ` Luis Machado @ 2017-01-19 16:56 ` Pedro Alves 2017-01-19 17:05 ` Luis Machado 0 siblings, 1 reply; 19+ messages in thread From: Pedro Alves @ 2017-01-19 16:56 UTC (permalink / raw) To: Luis Machado, Maciej W. Rozycki; +Cc: gdb-patches, jan.kratochvil On 01/09/2017 07:56 PM, Luis Machado wrote: > Reading the thread once again, is my understanding correct that the > first patch is more suitable now? Possibly with some cleanups? > > https://sourceware.org/ml/gdb-patches/2016-01/msg00134.html I think a summary of the discussion would help. I think it'd be very interesting to come up with a combination of "set mips abi n64/o64" etc, and maybe "set gnutarget" that exposes the problem. I've since added the bit gdb.base/all-architectures*.exp testcase, and that does try all sorts of combinations, including mips ones, but it evidently isn't catching this for some reason. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Handle loading improper core files gracefully in the mips backend. 2017-01-19 16:56 ` Pedro Alves @ 2017-01-19 17:05 ` Luis Machado 0 siblings, 0 replies; 19+ messages in thread From: Luis Machado @ 2017-01-19 17:05 UTC (permalink / raw) To: Pedro Alves, Maciej W. Rozycki; +Cc: gdb-patches, jan.kratochvil On 01/19/2017 10:56 AM, Pedro Alves wrote: > On 01/09/2017 07:56 PM, Luis Machado wrote: > >> Reading the thread once again, is my understanding correct that the >> first patch is more suitable now? Possibly with some cleanups? >> >> https://sourceware.org/ml/gdb-patches/2016-01/msg00134.html > > I think a summary of the discussion would help. > I'll need a refresh myself. Let me gather the most useful bits in the discussion and i'll report back. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-01-19 17:05 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-08 18:32 [PATCH] Handle loading improper core files gracefully in the mips backend Luis Machado 2016-01-09 3:02 ` Maciej W. Rozycki 2016-01-11 15:47 ` Luis Machado 2016-01-12 12:46 ` Pedro Alves 2016-01-12 13:25 ` Luis Machado 2016-01-12 14:10 ` Pedro Alves 2016-01-12 15:43 ` Luis Machado 2016-01-12 16:00 ` Pedro Alves 2016-01-12 18:30 ` Maciej W. Rozycki 2016-01-12 19:08 ` Pedro Alves 2016-02-02 12:58 ` Luis Machado 2016-02-02 14:19 ` Pedro Alves 2016-02-02 14:22 ` Pedro Alves 2016-02-04 21:01 ` Maciej W. Rozycki 2016-02-05 11:29 ` Luis Machado 2016-02-05 14:10 ` Maciej W. Rozycki 2017-01-09 19:57 ` Luis Machado 2017-01-19 16:56 ` Pedro Alves 2017-01-19 17:05 ` Luis Machado
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).