| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
This commit tweaks displaced_step_finish & friends to pass down a
target_waitstatus instead of a gdb_signal. This is needed because a
patch later in the step-over-{thread-exit,clone] series will want to
make displaced_step_buffers::finish handle
TARGET_WAITKIND_THREAD_EXITED. It also helps with the
TARGET_WAITKIND_THREAD_CLONED patch later in that same series.
It's also a bit more logical this way, as we don't have to pass down
signals when the thread didn't actually stop for a signal. So we can
also think of it as a clean up.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338
Change-Id: I4c5d338647b028071bc498c4e47063795a2db4c0
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
The comment on the gdbarch_displaced_step_fixup gdbarch method
indicates that this method is optional and that GDB will perform some
default if this method is not supplied. As such we define a predicate
gdbarch_displaced_step_fixup_p.
It may have been true at one point that the fixup method was optional,
but it is no longer true. If this method is not defined and GDB tries
to complete a displaced step, then GDB is going to crash.
Additionally the gdbarch_displaced_step_fixup_p predicate is not used
anywhere in GDB.
In this commit I have removed the gdbarch_displaced_step_fixup_p
predicate, and I have updated the validation check for the
gdbarch_displaced_step_fixup method; if the
gdbarch_displaced_step_copy_insn method is defined then the fixup
method must also be defined.
I believe I've manually checked all the current places where
gdbarch_displaced_step_copy_insn is defined and they all also define
the fixup method, so this change should cause no problems for anyone.
There should be no user visible changes after this commit.
Approved-By: Pedro Alves <pedro@palves.net>
|
|
The gdbarch::max_insn_length field is used mostly to support displaced
stepping; it controls the size of the buffers allocated for the
displaced-step instruction, and is also used when first copying the
instruction, and later, when fixing up the instruction, in order to
read in and parse the instruction being stepped.
However, it has started to be used in other places in GDB, for
example, it's used in the Python disassembler API, and it is used on
amd64 as part of branch-tracing instruction classification.
The problem is that the value assigned to max_insn_length is not
always the maximum instruction length, but sometimes is a multiple of
that length, as required to support displaced stepping, see rs600,
ARM, and AArch64 for examples of this.
It seems to me that we are overloading the meaning of the
max_insn_length field, and I think that could potentially lead to
confusion.
I propose that we add a new gdbarch field,
gdbarch::displaced_step_buffer_length, this new field will do
exactly what it says on the tin; represent the required displaced step
buffer size. The max_insn_length field can then do exactly what it
claims to do; represent the maximum length of a single instruction.
As some architectures (e.g. i386, and amd64) only require their
displaced step buffers to be a single instruction in size, I propose
that the default for displaced_step_buffer_length will be the
value of max_insn_length. Architectures than need more buffer space
can then override this default as needed.
I've updated all architectures to setup the new field if appropriate,
and I've audited all calls to gdbarch_max_insn_length and switched to
gdbarch_displaced_step_buffer_length where appropriate.
There should be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This commit switches the default value for the 'invalid' field from
False to True. All components that previous set the invalid field to
True explicitly have had the field removed.
I think that True is a good choice for the default, this means that we
now get the validity checks by default, and if anyone adds a new
Component they need to make a choice to add an 'invalid=False' line
and disable the validation.
The flip side of this is that 'invalid=False' seems to be far more
common than 'invalid=True'. But I don't see a huge problem with this,
we shouldn't be aiming to reduce our typing, rather we should choose
based on which is least likely to introduce bugs. I think assuming
that we should do a validity check will achieve that.
Some additional components need to have an 'invalid=False' line added
to their definition, these are components that have a predefault
value, which is sufficient; the tdep code doesn't need to replace this
value if it doesn't want to.
Without adding the 'invalid=False' these components would be
considered to be invalid if they have not changed from their
predefault value -- but the predefault is fine.
There's no change in the generated code after this commit, so there
will be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
I noticed that there are a bunch of 'predefault="0"' lines in
gdbarch_components.py, and that some (just some, not all) of these are
not needed.
The gdbarch is already zero initialized, but these lines seem to
exists so that we can know when to compare against "0" and when to
compare against "NULL". At least, this seems to be useful in some
places in the generated code.
Specifically, if we remove the predefault="0" line from the
max_insn_length component then we end up generating a line like:
gdb_assert (gdbarch->max_insn_length != NULL);
which doesn't compile as we compare a ULONGEST to NULL.
In this commit I remove all the predefault="0" lines that I claim are
obviously not needed. These are lines for components that are not
Values (i.e. the component holds a function pointer anyway), or for
Value components that hold a pointer type, in which case using NULL is
fine.
The only changes after this commit are some fields that have nullptr
as their initial value, and gcore_bfd_target now compares to NULL not
0 in gdbarch_gcore_bfd_target_p, which, given the field is of type
'const char *', seems like an improvement.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
For some reason the following value components of gdbarch:
bfloat16_format
half_format
float_format
double_format
long_double_format
so_ops
All use a postdefault but no predefault to set the default value for
the component.
As the postdefault values for these components are all constant
pointers that don't depend on other fields within the gdbarch, then I
don't see any reason why we couldn't use a predefault instead.
So lets do that.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This commit ensures that the 'invalid' property of all components is
either True, False, or a string.
Additionally, this commit allows a component to have both a predicate
and for the 'invalid' property to be a string.
Removing the option for 'invalid' to be None allows us to simplify the
algorithms in gdbarch.py a little.
Allowing a component to have both a predicate and an 'invalid' string
means that we can validate the value that a tdep sets into a field,
but also allow a predicate to ensure that the field has changed from
the default.
This functionality isn't going to be used in this series, but I have
tested it locally and believe that it would work, and this might make
it easier for others to add new components in the future.
In gdbarch_types.py, I've updated the type annotations to show that
the 'invalid' field should not be None, and I've changed the default
for this field from None to False.
The change to using False as the default is temporary. Later in this
series I'm going to change the default to True, but we need more fixes
before that can be done.
Additionally, in gdbarch_types.py I've removed an assert from
Component.get_predicate. This assert ensured that we didn't have the
predicate field set to True and the invalid field set to a string.
However, no component currently uses this configuration, and after
this commit, that combination is now supported, so the assert can be
removed.
As a consequence of the gdbarch_types.py changes we see some
additional comments generated in gdbarch.c about verification being
skipped due to the invalid field being False. This comment is inline
with plenty of other getters that also have a similar comment. Plenty
of the getters do have validation, so I think it is reasonable to have
a comment noting that the validation has been skipped for a specific
reason, rather than due to some bug.
In gdbarch_components.py I've had to add 'invalid=True' for two
components: gcore_bfd_target and max_insn_length, without this the
validation in the gdbarch getter would disappear.
And in gdbarch.py I've reworked the logic for generating the
verify_gdbarch function, and for generating the getter functions.
The logic for generating the getter functions is still not ideal, I
ended up having to add this additional logic block:
elif c.postdefault is not None and c.predefault is not None:
print(" /* Check variable changed from pre-default. */", file=f)
print(f" gdb_assert (gdbarch->{c.name} != {c.predefault});", file=f)
which was needed to ensure we continued to generate the same code as
before, without this the fact that invalid is now False when it would
previously have been None, meant that we dropped the gdb_assert in
favour of a comment like:
print(f" /* Skip verify of {c.name}, invalid_p == 0 */", file=f)
which is clearly not a good change. We could potentially look at
improving this in a later commit, but I don't plan to do that in this
series.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Restructure how gdbarch.py generates the verify_gdbarch function.
Previously the postdefault handling was bundled together with the
validation. This means that a field can't have both a postdefault,
and set its invalid attribute to a string.
This doesn't seem reasonable to me, I see no reason why a field can't
have both a postdefault (used when the tdep doesn't set the field),
and an invalid expression, which can be used to validate the value
that a tdep might set.
In this commit I restructure the verify_gdbarch generation code to
allow the above, there is no change in the actual generated code in
this commit, that will come in later commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Following on from the previous commit, this commit removes yet more
'invalid=True' lines from gdbarch_components.py where the invalid
setting has no effect.
Due to the algorithm used in gdbarch.py for generated verify_gdbarch,
if a component has a postdefault value then no invalid check will ever
be generated for the component, as such setting 'invalid=True' on the
component is pointless. This commit removes the setting of invalid.
There is no change in the generated code after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Due to the algorithm used to generate verify_gdbarch in gdbarch.py, if
a component has a predicate, then a validation check will never be
generated.
There are a bunch of components that are declared with both a
predicate AND have 'invalid=True' set. The 'invalid=True' has no
effect.
In this commit I clean things up by removing all these additional
'invalid=True' lines. There's no change in any of the generated files
after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Add type annotations to gdbarch*.py to fix all errors shown by pyright.
There is one change in copyright.py too, to fix this one:
/home/simark/src/binutils-gdb/gdb/gdbarch.py
/home/simark/src/binutils-gdb/gdb/gdbarch.py:206:13 - error: Type of "copyright" is partially unknown
Type of "copyright" is "(tool: Unknown, description: Unknown) -> str" (reportUnknownMemberType)
Change-Id: Ia109b53e267f6e2f5bd79a1288d0d5c9508c9ac4
Reviewed-By: Tom Tromey <tom@tromey.com>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
|
|
Editing gdbarch-components.py is not an experience in an editor that is
minimally smart about Python. Because gdbarch-components.py is read and
exec'd by gdbarch.py, it doesn't import the Info / Method / Function /
Value types. And because these types are defined in gdbarch.py, it
can't import them, as that would make a cyclic dependency.
Solve this by introducing a third file, gdbarch_types.py, to define
these types. Make gdbarch.py and gdbarch-components.py import it.
Also, replace the read & exec of gdbarch-components.py by a regular
import. For this to work though, gdbarch-components.py needs to be
renamed to gdbarch_components.py.
Change-Id: Ibe994d56ef9efcc0698b3ca9670d4d9bf8bbb853
Reviewed-By: Tom Tromey <tom@tromey.com>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
|
