[RFC,6/7] Lazily and dynamically create i386-linux target descriptions
Commit Message
Instead of using pre-generated target descriptions, this patch
changes GDB to lazily and dynamically create target descriptions
according to the target hardware capability (xcr0 in i386).
This support any combination of target features.
This patch also adds a unit test to make sure dynamically generated
tdesc are identical to these generated from xml files.
gdb:
2017-04-27 Yao Qi <yao.qi@linaro.org>
* i386-linux-tdep.c (i386_linux_read_description): Generate
target description if it doesn't exist.
[GDB_SELF_TEST] (i386_linux_read_description_test): New unit test.
(_initialize_i386_linux_tdep) [GDB_SELF_TEST]: Register unit test.
---
gdb/i386-linux-tdep.c | 85 ++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 67 insertions(+), 18 deletions(-)
Comments
On Thursday, May 11, 2017 04:55:04 PM Yao Qi wrote:
> Instead of using pre-generated target descriptions, this patch
> changes GDB to lazily and dynamically create target descriptions
> according to the target hardware capability (xcr0 in i386).
> This support any combination of target features.
>
> This patch also adds a unit test to make sure dynamically generated
> tdesc are identical to these generated from xml files.
I definitely like this approach of composing the tdesc. For the
non-Linux case there are already amd64_target_description() and
i386_target_description() functions in the respective -tdep.c files
that could use the same treatment. Not sure if you wanted to fix all
of x86 in the same patch series or do it as a separate followup?
On 17-05-11 10:04:39, John Baldwin wrote:
> On Thursday, May 11, 2017 04:55:04 PM Yao Qi wrote:
> > Instead of using pre-generated target descriptions, this patch
> > changes GDB to lazily and dynamically create target descriptions
> > according to the target hardware capability (xcr0 in i386).
> > This support any combination of target features.
> >
> > This patch also adds a unit test to make sure dynamically generated
> > tdesc are identical to these generated from xml files.
>
> I definitely like this approach of composing the tdesc. For the
> non-Linux case there are already amd64_target_description() and
> i386_target_description() functions in the respective -tdep.c files
> that could use the same treatment. Not sure if you wanted to fix all
> of x86 in the same patch series or do it as a separate followup?
>
The later, but I can fix all x86 in one patch if people think it is more
reasonable to do so. I plan to change one arch each time, and gradually
change all archs to use the new style of target description.
On 05/11/2017 04:55 PM, Yao Qi wrote:
> +#if GDB_SELF_TEST
> + struct xml_and_mask
> + {
> + const char *xml_file_name;
> + uint64_t mask;
> + };
Minor nit: I'd instinctively find it more natural to read the
list below as { input, output } (left to right), while you
have have { output, input }.
> +
> + struct xml_and_mask array[] = {
> + { "i386/i386-linux.xml", X86_XSTATE_SSE_MASK },
> + { "i386/i386-mmx-linux.xml", X86_XSTATE_X87_MASK },
> + { "i386/i386-avx-linux.xml", X86_XSTATE_AVX_MASK },
> + { "i386/i386-mpx-linux.xml", X86_XSTATE_MPX_MASK },
> + { "i386/i386-avx-mpx-linux.xml", X86_XSTATE_AVX_MPX_MASK },
> + { "i386/i386-avx-avx512-linux.xml", X86_XSTATE_AVX_AVX512_MASK },
> + { "i386/i386-avx-mpx-avx512-pku-linux.xml",
> + X86_XSTATE_AVX_MPX_AVX512_PKU_MASK },
About these xml files. Is your idea that:
#1 - we remove these xml description files at some point, keeping only
the description fragments which are currently xi:included by the xml
files above?
#2 - or, we'll still continue adding new xml files and grow this
list here?
#3 - or, we'll keep the existing xml files as representative / legacy,
and use them in the unit tests going forward, just to make sure
the machinery builds correct descriptions?
(I don't expect to answer to be #2, I just put it there for completeness.)
> + };
> +
> + for (auto &a : array)
> + {
> + auto tdesc = i386_linux_read_description (a.mask);
> +
> + selftests::record_xml_tdesc (a.xml_file_name, tdesc);
> + }
> +#endif /* GDB_SELF_TEST */
> }
>
Thanks,
Pedro Alves
Pedro Alves <palves@redhat.com> writes:
>> +
>> + struct xml_and_mask array[] = {
>> + { "i386/i386-linux.xml", X86_XSTATE_SSE_MASK },
>> + { "i386/i386-mmx-linux.xml", X86_XSTATE_X87_MASK },
>> + { "i386/i386-avx-linux.xml", X86_XSTATE_AVX_MASK },
>> + { "i386/i386-mpx-linux.xml", X86_XSTATE_MPX_MASK },
>> + { "i386/i386-avx-mpx-linux.xml", X86_XSTATE_AVX_MPX_MASK },
>> + { "i386/i386-avx-avx512-linux.xml", X86_XSTATE_AVX_AVX512_MASK },
>> + { "i386/i386-avx-mpx-avx512-pku-linux.xml",
>> + X86_XSTATE_AVX_MPX_AVX512_PKU_MASK },
>
> About these xml files. Is your idea that:
>
> #1 - we remove these xml description files at some point, keeping only
> the description fragments which are currently xi:included by the xml
> files above?
> #2 - or, we'll still continue adding new xml files and grow this
> list here?
> #3 - or, we'll keep the existing xml files as representative / legacy,
> and use them in the unit tests going forward, just to make sure
> the machinery builds correct descriptions?
>
> (I don't expect to answer to be #2, I just put it there for completeness.)
In short, #2 and #3.
IIUC, you are not against "continue adding new xml files" in #2, you are
against that we have to maintain the list here as we add new xml files.
Even we finished the transition for all ports to dynamic tdesc creation,
we still need to add new xml files to features/ directory for new
hardware features, because GDBserver needs them (regformats/*.dat need
them). We may add i386/i386-avx-mpx-avx512-linux.xml, GDB
doesn't need it, but GDBserver still needs it. Given we need to add
such .xml file, it is better to grow the list so that we can do the
test. So the answer is #2. Suppose one day, we don't need these XML
files to generate regformats/*.dat for GDBserver, we can remove all
i386-*linux.xml files except i386-avx-mpx-avx512-pku-linux.xml, but I
still want to move them to somewhere in testsuite directory to keep them
as tests. Then, it becomes #3.
The purpose of this test is to make sure these lazily/dynamically
created tdesc (using create_feature_org_gnu_gdb_XXX functions) equal to
the tdesc created from XML files. The dynamic tdesc creation is still
new, needs more tests.
During the process that we change other targets (amd64, arm, mips, etc)
to the dynamic tdesc creation, the lists like this are expected to be
added for each target.
When we finish the transition to dynamic tdesc creation for all ports,
and dynamic tdesc creation is quite stable, we can remove all these
tests and lists, but I prefer to keeping the tests.
Before we remove these tests and lists, we still need to update the
lists when,
1) add a new port with target description, a new list should be added
in its -tdep.c file.
2) add a new xml target description for the existing port,
Let me think about how to generate this list instead of manually
maintain this list.
On 05/18/2017 04:12 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>>> +
>>> + struct xml_and_mask array[] = {
>>> + { "i386/i386-linux.xml", X86_XSTATE_SSE_MASK },
>>> + { "i386/i386-mmx-linux.xml", X86_XSTATE_X87_MASK },
>>> + { "i386/i386-avx-linux.xml", X86_XSTATE_AVX_MASK },
>>> + { "i386/i386-mpx-linux.xml", X86_XSTATE_MPX_MASK },
>>> + { "i386/i386-avx-mpx-linux.xml", X86_XSTATE_AVX_MPX_MASK },
>>> + { "i386/i386-avx-avx512-linux.xml", X86_XSTATE_AVX_AVX512_MASK },
>>> + { "i386/i386-avx-mpx-avx512-pku-linux.xml",
>>> + X86_XSTATE_AVX_MPX_AVX512_PKU_MASK },
>>
>> About these xml files. Is your idea that:
>>
>> #1 - we remove these xml description files at some point, keeping only
>> the description fragments which are currently xi:included by the xml
>> files above?
>> #2 - or, we'll still continue adding new xml files and grow this
>> list here?
>> #3 - or, we'll keep the existing xml files as representative / legacy,
>> and use them in the unit tests going forward, just to make sure
>> the machinery builds correct descriptions?
>>
>> (I don't expect to answer to be #2, I just put it there for completeness.)
>
> In short, #2 and #3.
>
> IIUC, you are not against "continue adding new xml files" in #2, you are
> against that we have to maintain the list here as we add new xml files.
Well, I wasn't ever expecting we'd end up with #2. I guess I don't see
the point of going through all of this if we end up with having to add
manually-written xml descriptions covering all feature combinations, anyway?
I could see keeping the xml files pushed in the tree, but make them
generated instead? It could be gdb that generates them from that
xml_and_mask array. We'd share the code with gdbserver, so that gdbserver
would generate xml files on the fly to send to gdb.
Or we could do the generation completely outside gdb, with some file
describing the potential combinations (in spirit similar to that xml_and_mask
array), much like we generate the syscall xml files.
> Even we finished the transition for all ports to dynamic tdesc creation,
> we still need to add new xml files to features/ directory for new
> hardware features, because GDBserver needs them (regformats/*.dat need
> them). We may add i386/i386-avx-mpx-avx512-linux.xml, GDB
> doesn't need it, but GDBserver still needs it. Given we need to add
> such .xml file, it is better to grow the list so that we can do the
> test. So the answer is #2. Suppose one day, we don't need these XML
> files to generate regformats/*.dat for GDBserver, we can remove all
> i386-*linux.xml files except i386-avx-mpx-avx512-pku-linux.xml, but I
> still want to move them to somewhere in testsuite directory to keep them
> as tests. Then, it becomes #3.
OK. But if there's no plan to do the gdbserver side of the work,
I kind of call into question the more-complicated state we're getting
into, for an indeterminately long time.
> The purpose of this test is to make sure these lazily/dynamically
> created tdesc (using create_feature_org_gnu_gdb_XXX functions) equal to
> the tdesc created from XML files. The dynamic tdesc creation is still
> new, needs more tests.
>
> During the process that we change other targets (amd64, arm, mips, etc)
> to the dynamic tdesc creation, the lists like this are expected to be
> added for each target.
>
> When we finish the transition to dynamic tdesc creation for all ports,
> and dynamic tdesc creation is quite stable, we can remove all these
> tests and lists, but I prefer to keeping the tests.
I think most ports are not troublesome, WRT to explosion of
target description feature set combinations. Maybe a better approach
would be to fully transition an architecture all the way to
dynamic generation, gdb and gdbserver?
>
> Before we remove these tests and lists, we still need to update the
> lists when,
>
> 1) add a new port with target description, a new list should be added
> in its -tdep.c file.
> 2) add a new xml target description for the existing port,
>
> Let me think about how to generate this list instead of manually
> maintain this list.
Thanks,
Pedro Alves
Pedro Alves <palves@redhat.com> writes:
> Well, I wasn't ever expecting we'd end up with #2. I guess I don't see
> the point of going through all of this if we end up with having to add
> manually-written xml descriptions covering all feature combinations, anyway?
>
These changes are about GDB, but we still need manually-written xml
files because GDBserver still needs it. There are still some benefits,
- GDB binary becomes smaller, see
https://sourceware.org/ml/gdb-patches/2017-05/msg00299.html
- We don't have to generate a lot of gdb/features/*.c files, so don't
need to include them. In i386-linux, we only need
features/i386/i386-avx-mpx-avx512-pku-linux.c and all other
i386*linux.c can be removed.
> I could see keeping the xml files pushed in the tree, but make them
> generated instead? It could be gdb that generates them from that
> xml_and_mask array. We'd share the code with gdbserver, so that gdbserver
> would generate xml files on the fly to send to gdb.
>
> Or we could do the generation completely outside gdb, with some file
> describing the potential combinations (in spirit similar to that xml_and_mask
> array), much like we generate the syscall xml files.
>
I try to stop using the approach we pre-generate them (both xml files
and c files).
>> Even we finished the transition for all ports to dynamic tdesc creation,
>> we still need to add new xml files to features/ directory for new
>> hardware features, because GDBserver needs them (regformats/*.dat need
>> them). We may add i386/i386-avx-mpx-avx512-linux.xml, GDB
>> doesn't need it, but GDBserver still needs it. Given we need to add
>> such .xml file, it is better to grow the list so that we can do the
>> test. So the answer is #2. Suppose one day, we don't need these XML
>> files to generate regformats/*.dat for GDBserver, we can remove all
>> i386-*linux.xml files except i386-avx-mpx-avx512-pku-linux.xml, but I
>> still want to move them to somewhere in testsuite directory to keep them
>> as tests. Then, it becomes #3.
>
> OK. But if there's no plan to do the gdbserver side of the work,
> I kind of call into question the more-complicated state we're getting
> into, for an indeterminately long time.
I do plan to do GDBserver side, but I don't have a clear picture on how
to do it yet. I post this RFC, because this is only about GDB. This
series can make GDB better and keep GDBserver unchanged, it is still a
progress to me.
>
>> The purpose of this test is to make sure these lazily/dynamically
>> created tdesc (using create_feature_org_gnu_gdb_XXX functions) equal to
>> the tdesc created from XML files. The dynamic tdesc creation is still
>> new, needs more tests.
>>
>> During the process that we change other targets (amd64, arm, mips, etc)
>> to the dynamic tdesc creation, the lists like this are expected to be
>> added for each target.
>>
>> When we finish the transition to dynamic tdesc creation for all ports,
>> and dynamic tdesc creation is quite stable, we can remove all these
>> tests and lists, but I prefer to keeping the tests.
>
> I think most ports are not troublesome, WRT to explosion of
> target description feature set combinations. Maybe a better approach
> would be to fully transition an architecture all the way to
> dynamic generation, gdb and gdbserver?
ARM, MIPS, I386 and S390 have this problem to different extents. I can
take GDBserver into account at this stage. I did it last month, but
gave up because it was too brain-damaging. Let me try again.
@@ -681,27 +681,50 @@ i386_linux_core_read_xcr0 (bfd *abfd)
const struct target_desc *
i386_linux_read_description (uint64_t xcr0)
{
- switch ((xcr0 & X86_XSTATE_ALL_MASK))
+ if (xcr0 == 0)
+ return NULL;
+
+ static struct target_desc *i386_linux_tdescs \
+ [2/*X87*/][2/*SSE*/][2/*AVX*/][2/*MPX*/][2/*AVX512*/][2/*PKRU*/] = {};
+ struct target_desc **tdesc;
+
+ tdesc = &i386_linux_tdescs[(xcr0 & X86_XSTATE_X87) ? 1 : 0]
+ [(xcr0 & X86_XSTATE_SSE) ? 1 : 0]
+ [(xcr0 & X86_XSTATE_AVX) ? 1 : 0]
+ [(xcr0 & X86_XSTATE_MPX) ? 1 : 0]
+ [(xcr0 & X86_XSTATE_AVX512) ? 1 : 0]
+ [(xcr0 & X86_XSTATE_PKRU) ? 1 : 0];
+
+ if (*tdesc == NULL)
{
- case X86_XSTATE_AVX_MPX_AVX512_PKU_MASK:
- return tdesc_i386_avx_mpx_avx512_pku_linux;
- case X86_XSTATE_AVX_AVX512_MASK:
- return tdesc_i386_avx_avx512_linux;
- case X86_XSTATE_MPX_MASK:
- return tdesc_i386_mpx_linux;
- case X86_XSTATE_AVX_MPX_MASK:
- return tdesc_i386_avx_mpx_linux;
- case X86_XSTATE_AVX_MASK:
- return tdesc_i386_avx_linux;
- case X86_XSTATE_SSE_MASK:
- return tdesc_i386_linux;
- case X86_XSTATE_X87_MASK:
- return tdesc_i386_mmx_linux;
- default:
- break;
+ *tdesc = allocate_target_description ();
+ set_tdesc_architecture (*tdesc, bfd_scan_arch ("i386"));
+ set_tdesc_osabi (*tdesc, osabi_from_tdesc_string ("GNU/Linux"));
+
+ long regnum = 0;
+
+ if (xcr0 & X86_XSTATE_X87)
+ regnum = create_feature_org_gnu_gdb_i386_core_i386 (*tdesc, regnum);
+
+ if (xcr0 & X86_XSTATE_SSE)
+ regnum = create_feature_org_gnu_gdb_i386_sse (*tdesc, regnum);
+
+ regnum = create_feature_org_gnu_gdb_i386_linux (*tdesc, regnum);
+
+ if (xcr0 & X86_XSTATE_AVX)
+ regnum = create_feature_org_gnu_gdb_i386_avx (*tdesc, regnum);
+
+ if (xcr0 & X86_XSTATE_MPX)
+ regnum = create_feature_org_gnu_gdb_i386_mpx (*tdesc, regnum);
+
+ if (xcr0 & X86_XSTATE_AVX512)
+ regnum = create_feature_org_gnu_gdb_i386_avx512 (*tdesc, regnum);
+
+ if (xcr0 & X86_XSTATE_PKRU)
+ regnum = create_feature_org_gnu_gdb_i386_pkeys (*tdesc, regnum);
}
- return NULL;
+ return *tdesc;
}
/* Get Linux/x86 target description from core dump. */
@@ -1101,4 +1124,30 @@ _initialize_i386_linux_tdep (void)
initialize_tdesc_i386_avx_mpx_linux ();
initialize_tdesc_i386_avx_avx512_linux ();
initialize_tdesc_i386_avx_mpx_avx512_pku_linux ();
+
+#if GDB_SELF_TEST
+ struct xml_and_mask
+ {
+ const char *xml_file_name;
+ uint64_t mask;
+ };
+
+ struct xml_and_mask array[] = {
+ { "i386/i386-linux.xml", X86_XSTATE_SSE_MASK },
+ { "i386/i386-mmx-linux.xml", X86_XSTATE_X87_MASK },
+ { "i386/i386-avx-linux.xml", X86_XSTATE_AVX_MASK },
+ { "i386/i386-mpx-linux.xml", X86_XSTATE_MPX_MASK },
+ { "i386/i386-avx-mpx-linux.xml", X86_XSTATE_AVX_MPX_MASK },
+ { "i386/i386-avx-avx512-linux.xml", X86_XSTATE_AVX_AVX512_MASK },
+ { "i386/i386-avx-mpx-avx512-pku-linux.xml",
+ X86_XSTATE_AVX_MPX_AVX512_PKU_MASK },
+ };
+
+ for (auto &a : array)
+ {
+ auto tdesc = i386_linux_read_description (a.mask);
+
+ selftests::record_xml_tdesc (a.xml_file_name, tdesc);
+ }
+#endif /* GDB_SELF_TEST */
}