Message ID | 2907792.3mtlvelY3m@ralph.baldwin.cx |
---|---|
State | New |
Headers | show |
John Baldwin <jhb@freebsd.org> writes: > I'm not sure why these should actually be equal at all. In theory we > are resolving two different target descriptions which should have each > called tdesc_create_reg(). I don't see anything that follows a > singleton-like pattern so that if two tdesc's create the same register > with the same name they point to the same 'struct tdesc_reg *' though > that seems to be what is happening for other registers other than > fs_base and gs_base. The tdesc_reg is created for each target description, so there are multiple tdesc_reg objects created. That is, the tdesc_reg of "fs_base" are different among these tdesc_amd64_*_linux. However, the tdesc_reg is a singleton in each target description. Note that we can't use singleton for each register globally, because tdesc_reg.target_regnum can be different in different target descriptions. For example, fs_base's regnum is 57 in "bare-metal" amd64 target descriptions, while it is 58 in "linux" amd64 target descriptions, because of 64bit-linux.xml. > > I noticed that amd64_init_abi() in amd64-tdep.c invokes > tdesc_numbered_register earlier than is done for other registers on x86. > In particular, all the other registers are "added" via > tdesc_numbered_register in i386_validate_tdesc() which is called after > gdbarch_init_abi() (and thus after any OS-dependent hooks have had a > chance to complete). On a whim I tried deferring adding fs_base and > gs_base until i386_validate_tdesc(). This does seem to fix the crash > for me on a CentOS 7 VM I have lying around. The fs_base and gs_base > registers still work for me on FreeBSD. They do not show up in > info registers on the CentOS VM, but perhaps it is too old to have > the relevant ptrace ops (I know it's too old to have the updated Your patch fixes the crash, but I can't see fs_base and gs_base on my machine either. > struct user_reg). What I don't really understand though is why this > works. I also don't fully understand why 'data->arch_regs' is supposed > to always hold the same pointer values as in 'target_desc' in > tdesc_use_registers(). because each tdesc_reg is a singleton among the target description. The reason Simon observed that we have different "fs_base" tdesc_reg added and removed from the hash table is that they are from different target descriptions. GDB crashes in tdesc_use_registers. The arguments tdesc and tdesc_data are not consistent, tdesc is amd64 linux target description, but tdesc_data was got when tdesc is amd64 target description, so the tdesc_reg in tdesc_data are from amd64 target description as well. So, we push a "fs_base" from one target description and want to remove a "fs_base" from another target description. Does this answer your question? I think maybe there is some "better" fix that is to keep tdesc and tdes_data consistent. However, I don't think it further. Since current GDB trunk is unusable on x86_64-linux, it is better get a fix soon. > > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c > index 9ff7dfc513..3196ef75a1 100644 > --- a/gdb/amd64-tdep.c > +++ b/gdb/amd64-tdep.c > @@ -3061,15 +3061,7 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > > if (tdesc_find_feature (tdesc, "org.gnu.gdb.i386.segments") != NULL) > { > - const struct tdesc_feature *feature = > - tdesc_find_feature (tdesc, "org.gnu.gdb.i386.segments"); > - struct tdesc_arch_data *tdesc_data_segments = > - (struct tdesc_arch_data *) info.tdep_info; > - > - tdesc_numbered_register (feature, tdesc_data_segments, > - AMD64_FSBASE_REGNUM, "fs_base"); > - tdesc_numbered_register (feature, tdesc_data_segments, > - AMD64_GSBASE_REGNUM, "gs_base"); > + tdep->fsbase_regnum = AMD64_FSBASE_REGNUM; > } > There is one line, so braces are not needed. > if (tdesc_find_feature (tdesc, "org.gnu.gdb.i386.pkeys") != NULL) > diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c > index bd728f03dc..1c8263cc87 100644 > --- a/gdb/i386-tdep.c > +++ b/gdb/i386-tdep.c > @@ -8199,7 +8199,7 @@ i386_validate_tdesc_p (struct gdbarch_tdep *tdep, > const struct tdesc_feature *feature_core; > > const struct tdesc_feature *feature_sse, *feature_avx, *feature_mpx, > - *feature_avx512, *feature_pkeys; > + *feature_avx512, *feature_pkeys, *feature_segments; Indentation looks wrong. Did you run regression test on x86_64-linux?
On Thursday, July 13, 2017 05:55:06 PM Yao Qi wrote: > John Baldwin <jhb@freebsd.org> writes: > Your patch fixes the crash, but I can't see fs_base and gs_base on my > machine either. Ok. I assume yours is newer than my (ancient) VM and thus this patch doesn't work. > > struct user_reg). What I don't really understand though is why this > > works. I also don't fully understand why 'data->arch_regs' is supposed > > to always hold the same pointer values as in 'target_desc' in > > tdesc_use_registers(). > > because each tdesc_reg is a singleton among the target description. The > reason Simon observed that we have different "fs_base" tdesc_reg added > and removed from the hash table is that they are from different target > descriptions. GDB crashes in tdesc_use_registers. The arguments tdesc > and tdesc_data are not consistent, tdesc is amd64 linux target > description, but tdesc_data was got when tdesc is amd64 target > description, so the tdesc_reg in tdesc_data are from amd64 target > description as well. So, we push a "fs_base" from one target > description and want to remove a "fs_base" from another target > description. > > Does this answer your question? I think maybe there is some "better" > fix that is to keep tdesc and tdes_data consistent. However, I don't > think it further. Since current GDB trunk is unusable on x86_64-linux, > it is better get a fix soon. Ok. I think then that the hash table in tdesc_use_registers shouldn't be using the 'tdesc_reg' as the key but instead use the register name? This should be fairly simple to implement via a std::unordered_map<>. I'll try that in a bit, but if that doesn't resolve it we should revert the commits until we have a real fix.
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c index 9ff7dfc513..3196ef75a1 100644 --- a/gdb/amd64-tdep.c +++ b/gdb/amd64-tdep.c @@ -3061,15 +3061,7 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) if (tdesc_find_feature (tdesc, "org.gnu.gdb.i386.segments") != NULL) { - const struct tdesc_feature *feature = - tdesc_find_feature (tdesc, "org.gnu.gdb.i386.segments"); - struct tdesc_arch_data *tdesc_data_segments = - (struct tdesc_arch_data *) info.tdep_info; - - tdesc_numbered_register (feature, tdesc_data_segments, - AMD64_FSBASE_REGNUM, "fs_base"); - tdesc_numbered_register (feature, tdesc_data_segments, - AMD64_GSBASE_REGNUM, "gs_base"); + tdep->fsbase_regnum = AMD64_FSBASE_REGNUM; } if (tdesc_find_feature (tdesc, "org.gnu.gdb.i386.pkeys") != NULL) diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index bd728f03dc..1c8263cc87 100644 --- a/gdb/i386-tdep.c +++ b/gdb/i386-tdep.c @@ -8199,7 +8199,7 @@ i386_validate_tdesc_p (struct gdbarch_tdep *tdep, const struct tdesc_feature *feature_core; const struct tdesc_feature *feature_sse, *feature_avx, *feature_mpx, - *feature_avx512, *feature_pkeys; + *feature_avx512, *feature_pkeys, *feature_segments; int i, num_regs, valid_p; if (! tdesc_has_registers (tdesc)) @@ -8225,6 +8225,9 @@ i386_validate_tdesc_p (struct gdbarch_tdep *tdep, /* Try PKEYS */ feature_pkeys = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.pkeys"); + /* Try segment base registers. */ + feature_segments = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.segments"); + valid_p = 1; /* The XCR0 bits. */ @@ -8347,6 +8350,14 @@ i386_validate_tdesc_p (struct gdbarch_tdep *tdep, tdep->pkeys_register_names[i]); } + if (feature_segments && tdep->fsbase_regnum >= 0) + { + valid_p &= tdesc_numbered_register (feature_segments, tdesc_data, + tdep->fsbase_regnum, "fs_base"); + valid_p &= tdesc_numbered_register (feature_segments, tdesc_data, + tdep->fsbase_regnum + 1, "gs_base"); + } + return valid_p; } @@ -8591,6 +8602,9 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) tdep->pkru_regnum = -1; tdep->num_pkeys_regs = 0; + /* No segment base registers. */ + tdep->fsbase_regnum = -1; + tdesc_data = tdesc_data_alloc (); set_gdbarch_relocate_instruction (gdbarch, i386_relocate_instruction); diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h index 1ce89fcf65..a887a47752 100644 --- a/gdb/i386-tdep.h +++ b/gdb/i386-tdep.h @@ -198,6 +198,10 @@ struct gdbarch_tdep /* PKEYS register names. */ const char **pkeys_register_names; + /* Register number for %fs_base. Set this to -1 indicate the absence of + segment base registers. */ + int fsbase_regnum; + /* Target description. */ const struct target_desc *tdesc;