Zoran Zaric [Mon, 7 Dec 2020 19:00:31 +0000 (19:00 +0000)]
Add support for nested composite locations
After allowing a location description to be placed on a DWARF stack,
in an effort to achieve a full composability of the DWARF expression,
it is necessary to enable forming of a nested composite location
descriptions.
To be able do this, a new operation DW_OP_LLVM_piece_end needs to be
introduced, along with some additional rules on the way how the
composite location description is formed using the existing DW_OP_piece
and DW_OP_bit_piece operations. These new rules are fully compatible
with the composite forming rules from the DWARF 5 standard.
More details on the new operation and added rules can be found here:
Zoran Zaric [Mon, 7 Dec 2020 19:00:30 +0000 (19:00 +0000)]
Add support for DW_OP_LLVM_undefined operation
For the DW_OP_piece and DW_OP_bit_piece operations, in the DWARF 5
standard, it is stated that if the location description (of that piece)
is empty, then the piece is describing an undefined location
description.
The act of allowing any location description to be placed on a DWARF
stack means that now a new operations can be defined which could pop
more then one location description from a DWARF stack.
This means that the old rule is not really applicable any more and a
new operation that explicitly pushes an undefined location description
on the DWARF stack is needed.
This new rule however is fully backward compatibility as described
in the document found on:
Under the new definitions for the DW_OP_piece and DW_OP_bit_piece
operations.
gdb/ChangeLog:
* compile/compile-loc2c.c (compute_stack_depth_worker): Add
support for new DW_OP_LLVM_undefined operations.
* dwarf2/expr.c (dwarf_expr_context::execute_stack_op): Add
support for new DW_OP_LLVM_undefined operations.
* dwarf2/loc.c (dwarf2_get_symbol_read_needs): Add support
for new DW_OP_LLVM_undefined operations.
include/ChangeLog:
* dwarf2.def (DW_OP): New DW_OP_LLVM_undefined operations
enumeration.
Zoran Zaric [Mon, 7 Dec 2020 19:00:29 +0000 (19:00 +0000)]
Add DWARF operations for byte and bit offset
Currently in DWARF, there are only two ways to specify an offset for a
location description.
For a memory location description, the location description can be
first converted to a DWARF value, after which an arithmetic operation
can be applied to it. This however, only works while there are no
address spaces involved, that are not mapped to a general address space
(CORE_ADDR). Another limitation is that there is no way to specify a
bit offset to that location description.
Second way of specifying an offset to a location description is more
universal and involves wrapping that location description in a
composite piece, where piece itself has a bit/byte offset defined. The
problem with this approach is that both DW_OP_piece and DW_OP_bit_piece
define an offset as a DWARF operation operand, which means that an
offset needs to be a constant value encoded into the DWARF expression.
By adding three new operations (DW_OP_LLVM_offset,
DW_OP_LLVM_offset_constu and DW_OP_LLVM_bit_offset) these restrictions
are now lifted.
Detailed descriptions of these new operations can be found here:
The same document also explores an idea of extending the
DW_OP_push_object_address operation to allow pushing any location
description on the DWARF stack. This together with the new bit/byte
offset operations, generalizes DWARF to work with bit fields and could
replace the odd passed-in buffer mechanics in a more elegant way.
There seem to be a difference in views on what the big endian machine
register byte ordering should be. On one hand, some would expect for
a register to behave in the same way as memory, but on another, there
seems to be an existing implementation for (IBM big endian based
machines) which seems to be viewing registers differently, depending
if the register location description is part of a composite piece or
not. More on this topic can be found here:
Unfortunately, the gdb current implementation favors the second option,
which feels like a target specific implementation.
Because of this, I've decided to not favor a specific implementation
in the added test for new DWARF operations (dw2-llvm-offset.exp), so
the test is restricted to only run on little endian platforms.
gdb/ChangeLog:
* ada-lang.c (coerce_unspec_val_to_type): Add source bit offset
argument to the value_contents_copy_raw call.
* compile/compile-loc2c.c (compute_stack_depth_worker): Add new
DWARF operations support.
* dwarf2/expr.c (rw_closure_value): Add bit offset support.
(dwarf_expr_context::dwarf_entry_to_gdb_value): Add source bit
offset argument to the value_contents_copy_raw call.
(dwarf_expr_context::execute_stack_op): Add new DWARF
operations support.
* dwarf2/loc.c (dwarf2_get_symbol_read_needs): Add new DWARF
operations support.
* findvar.c (read_frame_register_value): Add source bit offset
argument to the value_contents_copy_raw call.
* valops.c (read_value_memory): Add bit offset support.
(value_assign): Add bit offset support.
(value_repeat): Add bit offset support.
(value_array): Add source bit offset argument to the
value_contents_copy_raw call.
(value_slice): Add source bit offset argument to the
value_contents_copy_raw call.
* value.c (value_contents_copy_raw): Add source bit offset
support.
(value_contents_copy): Add source bit offset argument to
value_contents_copy_raw call.
(value_primitive_field): Add source bit offset argument to the
value_contents_copy_raw call.
(value_from_component): Add source bit offset argument to the
value_contents_copy_raw call.
(value_fetch_lazy_memory): Add bit offset argument to the
read_value_memory call.
(value_fetch_lazy_register): Add source bit offset argument to
the value_contents_copy call.
* value.h (value_contents_copy): Add source bit offset
argument.
include/ChangeLog:
* dwarf2.def (DW_OP_DUP): New DWARF operations enumeration.
gdb/testsuite/ChangeLog:
* lib/dwarf.exp: Add support for new DW_OP_LLVM_offset_constu
DWARF operation.
* gdb.dwarf2/dw2-llvm-offset.exp: New test.
Zoran Zaric [Mon, 7 Dec 2020 19:00:28 +0000 (19:00 +0000)]
Add support for any location description in CFI
One of the main benefits of allowing location description to be on the
DWARF stack is that now CFI expression based register rules can be
defined using a location description operations. This allows a register
of one frame to be saved in any location, including any composite
location.
To fully support this feature, the execute_stack_op function in
dwarf2/frame.c needs to return a single struct value object instead of
just an address.
Function put_frame_register_bytes also needs to change to support any
location description.
This support is a one of the key features to truly support optimized
code.
gdb/ChangeLog:
* dwarf2/frame.c (execute_stack_op): Change to return a struct
value object.
(dwarf2_frame_cache): Change to call new execute_stack_op
definition.
(dwarf2_frame_prev_register): Change to call new execute_stack_op
definition.
* frame.c (put_frame_register_bytes): Add support for writing to
composite location description.
Zoran Zaric [Mon, 7 Dec 2020 19:00:27 +0000 (19:00 +0000)]
Remove DWARF expression composition check
The dwarf_expr_require_composition function reports an error if the
last operation is not a leaf node of the DWARF expression. This was
previously used to prevent location description operations to be used
freely in the DWARF expression.
With the new approach, everything all operations are treated the same
and everything is composable, so there is no need for the previous
restriction in the expression evaluator.
gdb/ChangeLog:
* dwarf2/expr.c (dwarf_expr_context::execute_stack_op): Remove
the use of dwarf_expr_require_composition.
Zoran Zaric [Mon, 7 Dec 2020 19:00:26 +0000 (19:00 +0000)]
Add frame info check to DW_OP_reg operations
After enabling location description to be on a DWARF stack, it is now
needed to check the frame context information validity when creating a
register location description.
gdb/ChangeLog:
* dwarf2/expr.c (dwarf_expr_context::execute_stack_op): Add
check_frame_info call for DW_OP_reg operations.
Zoran Zaric [Mon, 7 Dec 2020 19:00:25 +0000 (19:00 +0000)]
Move read_addr_from_reg function to frame.c
read_addr_from_reg function is now only called from frame.c file, this
means that the function can safely be moved there.
gdb/ChangeLog:
* dwarf2/expr.c (read_addr_from_reg): Move function to frame.c.
* dwarf2/expr.h (read_addr_from_reg): Remove function.
* dwarf2/frame.c (read_addr_from_reg): Add function from
expr.c.
Zoran Zaric [Mon, 7 Dec 2020 19:00:24 +0000 (19:00 +0000)]
Rename and update the piece_closure structure
Class that describes a computed_lval closure needs to be update to fit
better with the new dwarf_entry set of classes. This also means that a
pieced_value_funcs interface with that closure, needs to be renamed and
updated accordingly.
Considering that a closure is designed to describe a computed location
description, it makes sense to rename piece_closure to a
computed_closure.
gdb/ChangeLog:
* dwarf2/expr.c (struct piece_closure): Change to
computed_closure class.
(allocate_piece_closure): Remove function.
(rw_pieced_value): Rename to rw_closure_value and change to use
computed_closure class.
(read_pieced_value): Rename to read_closure_value and change to
use computed_closure class.
(write_pieced_value): Rename to write_closure_value and change
to use computed_closure class.
(check_pieced_synthetic_pointer): Rename to
check_synthetic_pointer and change to use computed_closure
class.
(indirect_pieced_value): Rename to indirect_closure_value and
change to use computed_closure class.
(coerce_pieced_ref): Rename to coerce_closure_ref and change
to use computed_closure class.
(copy_pieced_value_closure): Rename to copy_value_closure and
change to use computed_closure class.
(free_pieced_value_closure): Rename to free_value_closure and
change to use computed_closure class.
(dwarf_expr_context::gdb_value_to_dwarf_entry): Change to use
computed_closure class.
(dwarf_expr_context::dwarf_entry_to_gdb_value): Change to use
computed_closure class.
Zoran Zaric [Mon, 7 Dec 2020 19:00:23 +0000 (19:00 +0000)]
Remove dwarf_expr_context from expr.h interface
After the switch to the new evaluator implementation, it is now
possible to completely remove the dwarf_expr_context class from the
expr.h interface and encapsulate it inside the expr.c file.
The new interface consists of a new function called dwarf2_eval_exp
that takes a DWARF expression stream, initial DWARF stack elements (in
a form of a vector of a struct value objects), evaluation context and
expected result type information. Function returns an evaluation result
in a form of a struct value object.
Currently, there is ever only one initial stack element provided to the
evaluator and that element is always a memory address, so having a
vector of struct value object might seems like an overkill.
In reality this new flexibility allows implementation of a new DWARF
attribute extensions that could provide any number of initial stack
elements to describe any location description or value.
gdb/ChangeLog:
* dwarf2/expr.c (dwarf2_eval_exp): New function.
(struct dwarf_expr_context): Move from expr.h.
(dwarf_expr_context::push_address): Remove function.
* dwarf2/expr.h (struct dwarf_expr_context): Move to expr.c.
* dwarf2/frame.c (execute_stack_op): Now calls dwarf2_eval_exp.
* dwarf2/loc.c (dwarf2_evaluate_loc_desc_full): Now calls
dwarf2_eval_exp.
(dwarf2_locexpr_baton_eval): Now calls dwarf2_eval_exp.
Zoran Zaric [Mon, 7 Dec 2020 19:00:22 +0000 (19:00 +0000)]
Change DWARF stack to use new dwarf_entry classes
To replace existing DWARF stack element (dwarf_stack_value) with
dwarf_entry class based objects, a support for conversion between
struct value and the new classes is needed. The reason for this is
that dwarf_entry based classes are not designed to be visible outside
the expr.c file. This makes the DWARF expression evaluator more self
contained. This can be beneficial if there is ever a need to have a
DWARF support in gdbserver.
Once the conversion support is added, the current DWARF stack element
can easily be swapped out.
gdb/ChangeLog:
* dwarf2/expr.c (dwarf_value_equal_op): New function.
(dwarf_value_less_op): New function.
(struct piece_closure): Change to use dwarf_entry based
classes.
(allocate_piece_closure): Change to use dwarf_entry based
classes.
(rw_pieced_value): Change to use dwarf_entry based classes.
(check_pieced_synthetic_pointer): Change to use dwarf_entry
based classes.
(check_synthetic_pointer_location): New function.
(indirect_pieced_value): Change to use dwarf_entry based
classes.
(indirect_from_location): New function.
(coerce_pieced_ref): Change to use dwarf_entry based classes.
(free_pieced_value_closure): Change to use dwarf_entry based
classes.
(dwarf_expr_context::~dwarf_expr_context): Instantiate
dwarf_entry_factory object.
(dwarf_expr_context::push): Change to use dwarf_entry based
classes.
(dwarf_expr_context::push_address): Change to use dwarf_entry
based classes.
(dwarf_expr_context::fetch): Change to use dwarf_entry based
classes.
(dwarf_expr_context::read_mem): Remove method.
(dwarf_expr_context::fetch_result): Change to use dwarf_entry
based classes.
(dwarf_expr_context::dwarf_entry_deref): New method.
(dwarf_expr_context::gdb_value_to_dwarf_entry): New method.
(dwarf_expr_context::dwarf_entry_to_gdb_value): New method.
(dwarf_expr_context::fetch_address): Change to use dwarf_entry
based classes.
(dwarf_expr_context::fetch_in_stack_memory): Change to use
dwarf_entry based classes.
(dwarf_expr_context::add_piece): Change to use dwarf_entry based
classes.
(dwarf_expr_context::execute_stack_op): Change to use dwarf_entry
based classes.
* dwarf2/expr.h (class dwarf_entry): New declaration.
(class dwarf_entry_factory): New declaration.
(enum dwarf_value_location): Remove enumeration.
(struct dwarf_expr_piece): Remove structure.
(struct dwarf_stack_value): Remove structure.
(struct dwarf_expr_context): Change to use dwarf_entry based
classes. Add dwarf_entry_factory object.
Zoran Zaric [Mon, 7 Dec 2020 19:00:21 +0000 (19:00 +0000)]
Add dwarf_entry factory class to expr.c
This patch introduces a new class in charge of creating, tracking and
destroying dwarf_entry class based objects.
gdb/ChangeLog:
* dwarf2/expr.c (ill_formed_expression): New function.
(value_to_gdb_value): New function.
(class dwarf_entry_factory): New class.
(dwarf_entry_factory::~dwarf_entry_factory): New method.
(dwarf_entry_factory::record_entry): New method.
(dwarf_entry_factory::create_value): New method.
(dwarf_entry_factory::create_undefined): New method.
(dwarf_entry_factory::create_memory): New method.
(dwarf_entry_factory::create_register): New method.
(dwarf_entry_factory::create_implicit): New method.
(dwarf_entry_factory::create_implicit_pointer): New method.
(dwarf_entry_factory::create_composite): New method.
(dwarf_entry_factory::entry_to_location): New method.
(dwarf_entry_factory::entry_to_value): New method.
(dwarf_entry_factory::value_binary_op): New method.
(dwarf_entry_factory::value_negation_op): New method.
(dwarf_entry_factory::value_complement_op): New method.
(dwarf_entry_factory::value_cast_op): New method.
Zoran Zaric [Mon, 7 Dec 2020 19:00:20 +0000 (19:00 +0000)]
Add new location description access interface
After adding interface for register and memory location access, a new
top level interface for accessing any location, described by the
dwarf_location class based objects, can now be defined.
Also, the address_type method is now needed to be outside of the
dwarf_stack_value class to allow creation of the DWARF generic type
independently of that class.
* dwarf2/expr.c (read_from_location): New function.
(write_to_location): New function.
(address_type): New function.
* dwarf2/expr.h (address_type): Exposed function.
The document describes a couple of main issues found when using the
current DWARF 5 version to describe optimized code for SIMD and SIMT
architectures.
Without going much into details described in the document, the main
point is that DWARF version 5 does not allow a proper support for
address spaces and it does not allow a location description to be part
of a DWARF expression, unless it is a leaf node of that expression.
Both issues can be solved in a clean way by introducing a new set of
classes that describe all entry types which can be placed on a DWARF
stack, while keeping a full backward compatibility with the previous
standard version. These entry types can now be either a typed value
or any location description.
Currently, the result of an expression evaluation is kept in a separate
data structure, while with the new approach, it will be always found as
a top element of the DWARF stack. The reason why this approach is
backward compatible is because in version 5, a location description
is only allowed to be a leaf node of the expression or a composite
piece.
Question here is, why do we need a new set of classes and why not just
use the struct value instead?
As it stands, there are couple of issues with using the struct value to
describe a DWARF stack element:
- It is not designed to represent a DWARF location description
specifically, instead it behaves more like unified debug information
format that represents an actual target resource. One example of this
is accessing data of a struct value register location description,
where if the amount of data accessed is larger then the register,
results in accessing more then one register. In DWARF this is not a
valid behavior and locations that span more then one register should be
described as a composite location description.
- There is a tight coupling between struct value and it's type
information, regardless if the data represented is describing a value
(not_lval) or a location description. While the type information
dictates how the data is accessed for a struct value case, in DWARF,
location description doesn't have a type so data access is not bound by
it.
- DWARF values only support much simpler base types, while struct value
can be linked to any type. Admittedly, new classes are still using the
same struct value infrastructure for a value based operations at the
moment, but that is planned to change in the near future.
- struct value register location description requires a frame id
information which makes them unsuitable for CFA expression evaluation.
So, there seems to be a lack of separation of concerns in the design
of a struct value infrastructure, while the new classes are handling
one specific purpose and are completely encapsulated in the expr.c.
Additional benefit of this design is that it makes a step in a
right direction for being able to evaluate DWARF expressions on a
gdbserver side in the near future, which sounds like a desirable thing.
It is also worth mentioning that this new location description
representation is based on a bit granularity (the bit_suboffset class
member) even though the DWARF standard has a very limited support for
it (mostly used for DW_OP_bit_piece operation).
By allowing any location description to define a bit sub-offset of the
location, we are able to give more options for supporting of new
concepts (like the existing packed arrays in Ada language).
In this patch, a new set of classes that describe a DWARF stack element
are added. The new classes are:
- Value - describes a numerical value with a DWARF base type.
- Location description - describes a DWARF location description.
- Undefined location - describes a location that is not defined.
- Memory location - describes a location in memory.
- Register location - describes a register location in a frame
context.
- Implicit location - describes a location that implicitly holds a
value that it describes.
- Implicit pointer - describes a concept of an implicit pointer to
a source variable.
- Composite location - describes a location that is composed from
pieces of other location descriptions.
For now, these classes are just defined, and they are planned to be
used by the following patches.
gdb/ChangeLog:
* dwarf2/expr.c (class dwarf_entry): New class.
(class dwarf_value): New class.
(class dwarf_location): New class.
(class dwarf_undefined): New class.
(class dwarf_memory): New class.
(class dwarf_register): New class.
(class dwarf_implicit): New class.
(class dwarf_implicit_pointer): New class.
(class dwarf_composite): New class.
* value.c (pack_unsigned_long): Expose function.
* value.h (pack_unsigned_long): Expose function.
Zoran Zaric [Mon, 7 Dec 2020 19:00:18 +0000 (19:00 +0000)]
Add new memory access interface to expr.c
DWARF expression evaluator is currently using a few different
interfaces for memory access: write_memory_with_notification,
read_value_memory, read_memory.
They all seem incosistent, while some of them even need a struct
value typed argument to be present.
This patch is simplifying that interface by replacing it with two new
low level functions: read_from_memory and write_to_memory.
The advantage of this new interface is that it behaves in the same way
as the register access interface from the previous patch. Both of these
have the same error returning policy, which will be usefull for the
following patches.
* dwarf2/expr.c (xfer_from_memory): New function.
(read_from_memory): New function.
(write_to_memory): New function.
(rw_pieced_value): Now calls the read_from_memory and
write_to_memory functions.
Zoran Zaric [Mon, 7 Dec 2020 19:00:17 +0000 (19:00 +0000)]
Add new register access interface to expr.c
DWARF expression evaluator is currently using get_frame_register_bytes
and put_frame_register_bytes interface for register access.
The problem with evaluator using this interface is that it allows a
bleed out register access. This means that if the caller specifies a
larger amount of data then the size of a specified register, the
operation will continue accessing the neighboring registers until a
full amount of data has been reached.
DWARF specification does not define this behavior, so a new simplified
register access interface is needed instead.
* dwarf2/expr.c (read_from_register): New function.
(write_to_register): New function.
(rw_pieced_value): Now calls the read_from_register and
write_to_register functions.
Zoran Zaric [Mon, 7 Dec 2020 19:00:16 +0000 (19:00 +0000)]
Add as_lval argument to expression evaluator
There are cases where the result of the expression evaluation is
expected to be in a form of a value and not location description.
One place that has this requirement is dwarf_entry_parameter_to_value
function, but more are expected in the future. Until now, this
requirement was fulfilled by extending the evaluated expression with
a DW_OP_stack_value operation at the end.
New implementation, introduces a new evaluation argument instead.
Where constructor should only ever require a target architecture
information. This information is held in per object file
(dwarf2_per_objfile) structure, so it makes sense to keep that
structure as a constructor argument. It also makes sense to get the
address size from that structure, but unfortunately that interface
doesn't exist at the moment, so the dwarf_expr_context class user
needs to provide that information.
The push_address method is used to push a CORE_ADDR as a value on
top of the DWARF stack before the evaluation. This method can be
later changed to push any struct value object on the stack.
The eval_exp method is the method that evaluates a DWARF expression
and provides the evaluation result, in a form of a single struct
value object that describes a location. To do this, the method requires
a context of the evaluation, as well as expected result type
information. If the type information is not provided, the DWARF generic
type will be used instead.
gdb/ChangeLog:
* dwarf2/expr.c (dwarf_expr_context::dwarf_expr_context): Add
address size argument.
(dwarf_expr_context::read_mem): Change to use property_addr_info
structure.
(dwarf_expr_context::eval_exp): New function.
(dwarf_expr_context::execute_stack_op): Change to use
property_addr_info structure.
* dwarf2/expr.h (struct dwarf_expr_context): New eval_exp
declaration. Change eval and fetch_result method to private.
* dwarf2/frame.c (execute_stack_op): Change to call eval_exp
method.
* dwarf2/loc.c (dwarf2_evaluate_loc_desc_full): Change to call
eval_exp method.
(dwarf2_locexpr_baton_eval): Change to call eval_exp method.
Zoran Zaric [Mon, 7 Dec 2020 19:00:14 +0000 (19:00 +0000)]
Make DWARF evaluator return a single struct value
The patch is addressing the issue of class users writing and reading
the internal data of the dwarf_expr_context class.
At this point, all conditions are met for the DWARF evaluator to return
an evaluation result in a form of a single struct value object.
gdb/ChangeLog:
* dwarf2/expr.c (pieced_value_funcs): Chenge to static
function.
(allocate_piece_closure): Change to static function.
(dwarf_expr_context::fetch_result): New function.
* dwarf2/expr.h (struct piece_closure): Remove declaration.
(struct dwarf_expr_context): fetch_result new declaration.
fetch, fetch_address and fetch_in_stack_memory members move
to private.
(allocate_piece_closure): Remove.
* dwarf2/frame.c (execute_stack_op): Change to use
fetch_result.
* dwarf2/loc.c (dwarf2_evaluate_loc_desc_full): Change to use
fetch_result.
(dwarf2_locexpr_baton_eval): Change to use fetch_result.
Zoran Zaric [Mon, 7 Dec 2020 19:00:13 +0000 (19:00 +0000)]
Move piece_closure and its support to expr.c
Following 5 patches series is trying to clean up the interface of the
DWARF expression evaluator class (dwarf_expr_context).
After merging all expression evaluators into one class, the next
logical step is to make a clean user interface for that class. To do
that, we first need to address the issue of class users writing and
reading the internal data of the class directly.
Fixing the case of writing is simple, it makes sense for an evaluator
instance to be per architecture basis. Currently, the best separation
seems to be per object file, so having that data (dwarf2_per_objfile)
as a constructor argument makes sense. It also makes sense to get the
address size from that object file, but unfortunately that interface
does not exist at the moment.
Luckily, address size information is already available to the users
through other means. As a result, the address size also needs to be a
class constructor argument, at least until a better interface for
acquiring that information from an object file is implemented.
The rest of the user written data comes down to a context of an
evaluated expression (compilation unit context, frame context and
passed in buffer context) and a source type information that a result
of evaluating expression is representing. So, it makes sense for all of
these to be arguments of an evaluation method.
To address the problem of reading the dwarf_expr_context class
internal data, we first need to understand why it is implemented that
way?
This is actualy a question of which existing class can be used to
represent both values and a location descriptions and why it is not
used currently?
The answer is in a struct value class/structure, but the problem is
that before the evaluators were merged, only one evaluator had an
infrastructure to resolve composite and implicit pointer location
descriptions.
After the merge, we are now able to use the struct value to represent
any result of the expression evaluation. It also makes sense to move
all infrastructure for those location descriptions to the expr.c file
considering that that is the only place using that infrastructure.
What we are left with in the end is a clean public interface of the
dwarf_expr_context class containing:
The idea with this particular patch is to move piece_closure structure
and the interface that handles it (lval_funcs) to expr.c file.
While implicit pointer location descriptions are still not useful in
the CFI context (of the AMD's DWARF standard extensions), the composite
location descriptions are certainly necessary to describe a results of
specific compiler optimizations.
Considering that a piece_closure structure is used to represent both,
there was no benefit in splitting them.
gdb/ChangeLog:
* dwarf2/expr.c (struct piece_closure): Add from loc.c.
(allocate_piece_closure): Add from loc.c.
(bits_to_bytes): Add from loc.c.
(rw_pieced_value): Add from loc.c.
(read_pieced_value): Add from loc.c.
(write_pieced_value): Add from loc.c.
(check_pieced_synthetic_pointer): Add from loc.c.
(indirect_pieced_value): Add from loc.c.
(coerce_pieced_ref): Add from loc.c.
(copy_pieced_value_closure): Add from loc.c.
(free_pieced_value_closure): Add from loc.c.
(sect_variable_value): Add from loc.c.
* dwarf2/loc.c (sect_variable_value): Move to expr.c.
(struct piece_closure): Move to expr.c.
(allocate_piece_closure): Move to expr.c.
(bits_to_bytes): Move to expr.c.
(rw_pieced_value): Move to expr.c.
(read_pieced_value): Move to expr.c.
(write_pieced_value): Move to expr.c.
(check_pieced_synthetic_pointer): Move to expr.c.
(indirect_pieced_value): Move to expr.c.
(coerce_pieced_ref): Move to expr.c.
(copy_pieced_value_closure): Move to expr.c.
(free_pieced_value_closure): Move to expr.c.
* dwarf2/loc.h (invalid_synthetic_pointer): Expose function.
Zoran Zaric [Mon, 7 Dec 2020 19:00:12 +0000 (19:00 +0000)]
Merge evaluate_for_locexpr_baton evaluator
The evaluate_for_locexpr_baton is the last derived class from the
dwarf_expr_context class. It's purpose is to support the passed in
buffer functionality.
Although, it is not really necessary to merge this class with it's
base class, doing that simplifies new expression evaluator design.
Considering that this functionality is going around the DWARF standard,
it is also reasonable to expect that with a new evaluator design and
extending the push object address functionality to accept any location
description, there will be no need to support passed in buffers.
Alternatively, it would also makes sense to abstract the interaction
between the evaluator and a given resource in the near future. The
passed in buffer would then be a specialization of that abstraction.
gdb/ChangeLog:
* dwarf2/expr.c (dwarf_expr_context::read_mem): Merge with
evaluate_for_locexpr_baton implementation.
* dwarf2/loc.c (class evaluate_for_locexpr_baton): Remove
class.
(evaluate_for_locexpr_baton::read_mem): Move to
dwarf_expr_context.
(dwarf2_locexpr_baton_eval): Instantiate dwarf_expr_context
instead of evaluate_for_locexpr_baton class.
Zoran Zaric [Mon, 7 Dec 2020 19:00:11 +0000 (19:00 +0000)]
Remove empty frame and full evaluators
There are no virtual methods that require different specialization in
dwarf_expr_context class. This means that derived classes
dwarf_expr_executor and dwarf_evaluate_loc_desc are not needed any
more.
As a result of this, the evaluate_for_locexpr_baton class base class
is now the dwarf_expr_context class.
There might be a need for a better class hierarchy when we know more
about the direction of the future DWARF versions and gdb extensions,
but that is out of the scope of this patch series.
Zoran Zaric [Mon, 7 Dec 2020 19:00:09 +0000 (19:00 +0000)]
Move push_dwarf_reg_entry_value to expr.c
Following the idea of merging the evaluators, the
push_dwarf_reg_entry_value method can be moved from
dwarf_expr_executor and dwarf_evaluate_loc_desc classes
to their base class dwarf_expr_context.
Zoran Zaric [Mon, 7 Dec 2020 19:00:08 +0000 (19:00 +0000)]
Move read_mem to dwarf_expr_context
Following the idea of merging the evaluators, the read_mem method can
be moved from dwarf_expr_executor and dwarf_evaluate_loc_desc classes
to their base class dwarf_expr_context.
gdb/ChangeLog:
* dwarf2/expr.c (dwarf_expr_context::read_mem): Move from
dwarf_evaluate_loc_desc.
* dwarf2/frame.c (dwarf_expr_executor::read_mem): Remove
method.
* dwarf2/loc.c (dwarf_evaluate_loc_desc::read_mem): Move to
dwarf_expr_context.
Zoran Zaric [Mon, 7 Dec 2020 19:00:07 +0000 (19:00 +0000)]
Move get_object_address to dwarf_expr_context
Following the idea of merging the evaluators, the get_object_address
and can be moved from dwarf_expr_executor and dwarf_evaluate_loc_desc
classes to their base class dwarf_expr_context.
gdb/ChangeLog:
* dwarf2/expr.c (dwarf_expr_context::get_object_address): Move
from dwarf_evaluate_loc_desc.
(class dwarf_expr_context): Add object address member to
dwarf_expr_context.
* dwarf2/expr.h (dwarf_expr_context::get_frame_pc): Remove
method.
* dwarf2/frame.c (dwarf_expr_executor::get_object_address):
Remove method.
* dwarf2/loc.c (dwarf_evaluate_loc_desc::get_object_address):
move to dwarf_expr_context.
(class dwarf_evaluate_loc_desc): Move object address member to
dwarf_expr_context.
Zoran Zaric [Mon, 7 Dec 2020 19:00:06 +0000 (19:00 +0000)]
Move dwarf_call to dwarf_expr_context
Following the idea of merging the evaluators, the dwarf_call and
get_frame_pc method can be moved from dwarf_expr_executor and
dwarf_evaluate_loc_desc classes to their base class dwarf_expr_context.
Once this is done, the get_frame_pc can be replace with lambda
function.
Zoran Zaric [Mon, 7 Dec 2020 19:00:05 +0000 (19:00 +0000)]
Move compilation unit info to dwarf_expr_context
This patch moves the compilation unit context information and support
from dwarf_expr_executor and dwarf_evaluate_loc_desc to
dwarf_expr_context evaluator. The idea is to report an error when a
given operation requires a compilation unit information to be resolved,
which is not available.
gdb/ChangeLog:
* dwarf2/expr.c (ensure_have_per_cu): New function.
(dwarf_expr_context::dwarf_expr_context): Add compilation unit
context information.
(dwarf_expr_context::get_base_type): Move from
dwarf_evaluate_loc_desc.
(dwarf_expr_context::get_addr_index): Remove method.
(dwarf_expr_context::dwarf_variable_value): Remove method.
(dwarf_expr_context::execute_stack_op): Call compilation unit
context info check. Inline get_addr_index and
dwarf_variable_value methods.
* dwarf2/expr.h (struct dwarf_expr_context): Add compilation
context info. Remove get_addr_index and dwarf_variable_value.
* dwarf2/frame.c (dwarf_expr_executor::get_addr_index): Remove
method.
(dwarf_expr_executor::dwarf_variable_value): Remove method.
* dwarf2/loc.c (sect_variable_value): Expose function.
(dwarf_evaluate_loc_desc::get_addr_index): Remove method.
(dwarf_evaluate_loc_desc::dwarf_variable_value): Remove method.
(class dwarf_evaluate_loc_desc): Move compilation unit context
information to dwarf_expr_context class.
* dwarf2/loc.h (sect_variable_value): Expose function.
Zoran Zaric [Mon, 7 Dec 2020 19:00:04 +0000 (19:00 +0000)]
Remove get_frame_cfa from dwarf_expr_context
Following the idea of merging the evaluators, the get_frame_cfa method
can be moved from dwarf_expr_executor and dwarf_evaluate_loc_desc
classes to their base class dwarf_expr_context. Once this is done,
it becomes apparent that the method is only called once and it can be
inlined.
It is also necessary to check if the frame context information was
provided before the DW_OP_call_frame_cfa operation is executed.
gdb/ChangeLog:
* dwarf2/expr.c (dwarf_expr_context::get_frame_cfa): Remove
method.
(dwarf_expr_context::execute_stack_op): Call frame context info
check for DW_OP_call_frame_cfa. Remove use of get_frame_cfa.
* dwarf2/expr.h (dwarf_expr_context::get_frame_cfa): Remove
method.
* dwarf2/frame.c (dwarf_expr_context::get_frame_cfa): Remove
method.
* dwarf2/loc.c (dwarf_expr_context::get_frame_cfa): Remove
method.
Zoran Zaric [Mon, 7 Dec 2020 19:00:03 +0000 (19:00 +0000)]
Move frame context info to dwarf_expr_context
Following 15 patches in this patch series is cleaning up the design of
the DWARF expression evaluator (dwarf_expr_context) to make future
extensions of that evaluator easier and cleaner to implement.
There are three subclasses of the dwarf_expr_context class
(dwarf_expr_executor, dwarf_evaluate_loc_desc and
evaluate_for_locexpr_baton). Here is a short description of each class:
- dwarf_expr_executor is evaluating a DWARF expression in a context
of a Call Frame Information. The overridden methods of this subclass
report an error if a specific DWARF operation, represented by that
method, is not allowed in a CFI context. The source code of this
subclass lacks the support for composite as well as implicit pointer
location description.
- dwarf_evaluate_loc_desc can evaluate any expression with no
restrictions. All of the methods that this subclass overrides are
actually doing what they are intended to do. This subclass contains
a full support for all location description types.
- evaluate_for_locexpr_baton subclass is a specialization of the
dwarf_evaluate_loc_desc subclass and it's function is to add
support for passed in buffers. This seems to be a way to go around
the fact that DWARF standard lacks a bit offset support for memory
location descriptions as well as using any location description for
the push object address functionality.
It all comes down to this question: what is a function of a DWARF
expression evaluator?
Is it to evaluate the expression in a given context or to check the
correctness of that expression in that context?
Currently, the only reason why there is a dwarf_expr_executor subclass
is to report an invalid DWARF expression in a context of a CFI, but is
that what the evaluator is supposed to do considering that the evaluator
is not tied to a given DWARF version?
There are more and more vendor and GNU extensions that are not part of
the DWARF standard, so is it that impossible to expect that some of the
extensions could actually lift the previously imposed restrictions of
the CFI context? Not to mention that every new DWARF version is lifting
some restrictions anyway.
The thing that makes more sense for an evaluator to do, is to take the
context of an evaluation and checks the requirements of every operation
evaluated against that context. With this approach, the evaluator would
report an error only if parts of the context, necessary for the
evaluation, are missing.
If this approach is taken, then the unification of the
dwarf_evaluate_loc_desc, dwarf_expr_executor and dwarf_expr_context
is the next logical step. This makes a design of the DWARF expression
evaluator cleaner and allows more flexibility when supporting future
vendor and GNU extensions.
Additional benefit here is that now all evaluators have access to all
location description types, which means that a vendor extended CFI
rules could support composite location description as well. This also
means that a new evaluator interface can be changed to return a single
struct value (that describes the result of the evaluation) instead of
a caller poking around the dwarf_expr_context internal data for answers
(like it is done currently).
This patch starts the merging proccess by moving the frame context
information and support from dwarf_expr_executor and
dwarf_evaluate_loc_desc to dwarf_expr_context evaluator. The idea
is to report an error when a given operation requires a frame
information to be resolved, if that information is not present.
gdb/ChangeLog:
* dwarf2/expr.c (ensure_have_frame): New function.
(read_addr_from_reg): Add from frame.c.
(dwarf_expr_context::dwarf_expr_context): Add frame info to
dwarf_expr_context.
(dwarf_expr_context::read_addr_from_reg): Remove.
(dwarf_expr_context::get_reg_value): Move from
dwarf_evaluate_loc_desc.
(dwarf_expr_context::get_frame_base): Move from
dwarf_evaluate_loc_desc.
(dwarf_expr_context::execute_stack_op): Call frame context info
check. Remove use of read_addr_from_reg method.
* dwarf2/expr.h (struct dwarf_expr_context): Add frame info
member, read_addr_from_reg, get_reg_value and get_frame_base
declaration.
(read_addr_from_reg): Move to expr.c.
* dwarf2/frame.c (read_addr_from_reg): Move to
dwarf_expr_context.
(dwarf_expr_executor::read_addr_from_reg): Remove.
(dwarf_expr_executor::get_frame_base): Remove.
(dwarf_expr_executor::get_reg_value): Remove.
(execute_stack_op): Use read_addr_from_reg function instead of
read_addr_from_reg method.
* dwarf2/loc.c (dwarf_evaluate_loc_desc::get_frame_base): Move
to dwarf_expr_context.
(dwarf_evaluate_loc_desc::get_reg_value): Move to
dwarf_expr_context.
(dwarf_evaluate_loc_desc::read_addr_from_reg): Remove.
(dwarf2_locexpr_baton_eval):Use read_addr_from_reg function
instead of read_addr_from_reg method.
Zoran Zaric [Mon, 7 Dec 2020 19:00:02 +0000 (19:00 +0000)]
Replace the symbol needs evaluator with a parser
This patch addresses a design problem with the symbol_needs_eval_context
class. It exposes the problem by introducing two new testsuite test
cases.
To explain the issue, I first need to explain the dwarf_expr_context
class that the symbol_needs_eval_context class derives from.
The intention behind the dwarf_expr_context class is to commonize the
DWARF expression evaluation mechanism for different evaluation
contexts. Currently in gdb, the evaluation context can contain some or
all of the following information: architecture, object file, frame and
compilation unit.
Depending on the information needed to evaluate a given expression,
there are currently three distinct DWARF expression evaluators:
 - Frame: designed to evaluate an expression in the context of a call
  frame information (dwarf_expr_executor class). This evaluator doesn't
  need a compilation unit information.
 - Location description: designed to evaluate an expression in the
  context of a source level information (dwarf_evaluate_loc_desc
  class). This evaluator expects all information needed for the
  evaluation of the given expression to be present.
 - Symbol needs: designed to answer a question about the parts of the
  context information required to evaluate a DWARF expression behind a
  given symbol (symbol_needs_eval_context class). This evaluator
  doesn't need a frame information.
The functional difference between the symbol needs evaluator and the
others is that this evaluator is not meant to interact with the actual
target. Instead, it is supposed to check which parts of the context
information are needed for the given DWARF expression to be evaluated by
the location description evaluator.
The idea is to take advantage of the existing dwarf_expr_context
evaluation mechanism and to fake all required interactions with the
actual target, by returning back dummy values. The evaluation result is
returned as one of three possible values, based on operations found in a
given expression:
- SYMBOL_NEEDS_NONE,
- SYMBOL_NEEDS_REGISTERS and
- SYMBOL_NEEDS_FRAME.
The problem here is that faking results of target interactions can yield
an incorrect evaluation result.
For example, if we have a conditional DWARF expression, where the
condition depends on a value read from an actual target, and the true
branch of the condition requires a frame information to be evaluated,
while the false branch doesn't, fake target reads could conclude that a
frame information is not needed, where in fact it is. This wrong
information would then cause the expression to be actually evaluated (by
the location description evaluator) with a missing frame information.
This would then crash the debugger.
The gdb.dwarf2/symbol_needs_eval_fail.exp test introduces this
scenario, with the following DWARF expression:
                  DW_OP_addr $some_variable
                  DW_OP_deref
                  DW_OP_bra 4 # conditional jump to DW_OP_bregx
                  DW_OP_lit0
                  DW_OP_skip 3 # jump to DW_OP_stack_value
                  DW_OP_bregx $dwarf_regnum 0
                  DW_OP_stack_value
This expression describes a case where some variable dictates the
location of another variable. Depending on a value of some_variable, the
variable whose location is described by this expression is either read
from a register or it is defined as a constant value 0. In both cases,
the value will be returned as an implicit location description on the
DWARF stack.
Currently, when the symbol needs evaluator fakes a memory read from the
address behind the some_variable variable, the constant value 0 is used
as the value of the variable A, and the check returns the
SYMBOL_NEEDS_NONE result.
This is clearly a wrong result and it causes the debugger to crash.
The scenario might sound strange to some people, but it comes from a
SIMD/SIMT architecture where $some_variable is an execution mask. In
any case, it is a valid DWARF expression, and GDB shouldn't crash while
evaluating it. Also, a similar example could be made based on a
condition of the frame base value, where if that value is concluded to
be 0, the variable location could be defaulted to a TLS based memory
address.
The gdb.dwarf2/symbol_needs_eval_timeout.exp test introduces a second
scenario. This scenario is a bit more abstract due to the DWARF
assembler lacking the CFI support, but it exposes a different
manifestation of the same problem. Like in the previous scenario, the
DWARF expression used in the test is valid:
                      DW_OP_lit1
                      DW_OP_addr $some_variable
                      DW_OP_deref
                      DW_OP_skip 4 # jump to DW_OP_fbreg
                      DW_OP_drop
                      DW_OP_fbreg 0
                      DW_OP_dup
                      DW_OP_lit0
                      DW_OP_eq
                      DW_OP_bra -9 # conditional jump to DW_OP_drop
                      DW_OP_stack_value
Similarly to the previous scenario, the location of a variable A is an
implicit location description with a constant value that depends on a
value held by a global variable. The difference from the previous case
is that DWARF expression contains a loop instead of just one branch. The
end condition of that loop depends on the expectation that a frame base
value is never zero. Currently, the act of faking the target reads will
cause the symbol needs evaluator to get stuck in an infinite loop.
Somebody could argue that we could change the fake reads to return
something else, but that would only hide the real problem.
The general impression seems to be that the desired design is to have
one class that deals with parsing of the DWARF expression, while there
are virtual methods that deal with specifics of some operations.
Using an evaluator mechanism here doesn't seem to be correct, because
the act of evaluation relies on accessing the data from the actual
target with the possibility of skipping the evaluation of some parts of
the expression.
To better explain the proposed solution for the issue, I first need to
explain a couple more details behind the current design:
There are multiple places in gdb that handle DWARF expression parsing
for different purposes. Some are in charge of converting the expression
to some other internal representation (decode_location_expression,
disassemble_dwarf_expression and dwarf2_compile_expr_to_ax), some are
analysing the expression for specific information
(compute_stack_depth_worker) and some are in charge of evaluating the
expression in a given context (dwarf_expr_context::execute_stack_op
and decode_locdesc).
The problem is that all those functions have a similar (large) switch
statement that handles each DWARF expression operation. The result of
this is a code duplication and harder maintenance.
As a step into the right direction to solve this problem (at least for
the purpose of a DWARF expression evaluation) the expression parsing was
commonized inside of an evaluator base class (dwarf_expr_context). This
makes sense for all derived classes, except for the symbol needs
evaluator (symbol_needs_eval_context) class.
As described previously the problem with this evaluator is that if the
evaluator is not allowed to access the actual target, it is not really
evaluating.
Instead, the desired function of a symbol needs evaluator seems to fall
more into expression analysis category. This means that a more natural
fit for this evaluator is to be a symbol needs analysis, similar to the
existing compute_stack_depth_worker analysis.
Another problem is that using a heavyweight mechanism of an evaluator
to do an expression analysis seems to be an unneeded overhead. It also
requires a more complicated design of the parent class to support fake
target reads.
The reality is that the whole symbol_needs_eval_context class can be
replaced with a lightweight recursive analysis function, that will give
more correct result without compromising the design of the
dwarf_expr_context class.
The downside of this approach is adding of one more similar switch
statement, but at least this way the new symbol needs analysis will be
a lightweight mechnism and it will provide a correct result for any
given DWARF expression.
A more desired long term design would be to have one class that deals
with parsing of the DWARF expression, while there would be a virtual
methods that deal with specifics of some DWARF operations. Then that
class would be used as a base for all DWARF expression parsing mentioned
at the beginning.
This however, requires a far bigger changes that are out of the scope
of this patch series.
To support the new implementation, I have also added two new self tests,
found in a file dwarf2/loc.c (named symbol_needs_cond_nonzero and
symbol_needs_cond_zero), which expose a similar problem to the one
described in the first testsuite test case. The difference is that the
global some_variable is replaced with a use of a
DW_OP_push_object_address operation. The idea is for the symbol needs
evaluator to return the SYMBOL_NEEDS_FRAME value, regardless of the
evaluation of that operation.
The new analysis requires the DWARF location description for the
argc argument of the niam function to change in the assembly file
gdb.python/amd64-py-framefilter-invalidarg.S. Originally, expression
ended with a 0 value byte, which was never reached by the symbol needs
evaluator, because it was detecting a stack underflow when evaluating
the operation before. The new approach does not simulate a DWARF
stack anymore, so the 0 value byte needs to be removed because it
makes the DWARF expression invalid.
gdb/ChangeLog:
* dwarf2/loc.c (class symbol_needs_eval_context): Remove.
(dwarf2_get_symbol_read_needs): New function.
(dwarf2_loc_desc_get_symbol_read_needs): Remove.
(locexpr_get_symbol_read_needs): Use
dwarf2_get_symbol_read_needs.
(symbol_needs_cond_zero): New function.
(symbol_needs_cond_nonzero): New function.
(_initialize_dwarf2loc): Register selftests.
gdb/testsuite/ChangeLog:
* gdb.python/amd64-py-framefilter-invalidarg.S : Update argc
DWARF location expression.
* lib/dwarf.exp (_location): Handle DW_OP_fbreg.
* gdb.dwarf2/symbol_needs_eval.c: New file.
* gdb.dwarf2/symbol_needs_eval_fail.exp: New file.
* gdb.dwarf2/symbol_needs_eval_timeout.exp: New file.
gdbsupport: Use LOCALAPPDATA to determine cache dir
Use the LOCALAPPDATA environment variable to determine the cache dir
when running on Windows with native command line, otherwise nasty
warning "Couldn't determine a path for index cached directory" appears.
The problem is that we're expecting a $hex for the value of self_id, but
instead get <optimized out>.
Looking at the debug info for self_id:
...
<1><12a1f>: Abbrev Number: 84 (DW_TAG_subprogram)
<12a20> DW_AT_name : system__tasking__stages__task_wrapper
...
<2><12a35>: Abbrev Number: 61 (DW_TAG_formal_parameter)
<12a36> DW_AT_name : self_id
<12a40> DW_AT_location : 0x459e (location list)
...
it refers to location information here:
... 0000459e08053060080531ac (DW_OP_fbreg: 0) 000045aa0805327c080532a5 (DW_OP_fbreg: 0) 000045b60805332008053324 (DW_OP_fbreg: 0)
...
while the pc used to retrieve the location information is 0x080531c5:
...
$ gdb -batch outputs/gdb.ada/mi_task_arg/task_switch \
-ex "break 57" -ex run -ex bt
...
#0 task_switch.break_me () at task_switch.adb:57
#1 0x0804aaae in task_switch.caller (<_task>=0x808abf0) \
at task_switch.adb:51
#2 0x080531c5 in system.tasking.stages.task_wrapper \
(self_id=<optimized out>) at s-tassta.adb:1295
...
which indeed falls outside of the ranges listed in the location info.
Fix this by accepting <optimized out> as valid value of self_id.
Tested on x86_64-linux.
gdb/testsuite/ChangeLog:
2020-12-08 Tom de Vries <tdevries@suse.de>
* gdb.ada/mi_task_arg.exp: Accept <optimized out> as valid value of
self_id.
smart_rename is capable of handling symlinks by copying and it also
tries to preserve ownership and permissions of files when they're
overwritten during the rename. This is useful in objcopy where the
file properties need to be preserved.
However because smart_rename does this using file names, it leaves a
race window between renames and permission fixes. This change removes
this race window by using file descriptors from the original BFDs that
were used to manipulate these files wherever possible.
The file that is to be renamed is also passed as a file descriptor so
that we use fchown/fchmod on the file descriptor, thus making sure
that we only modify the file we have opened to write. Further, in
case the file is to be overwritten (as is the case in ar or objcopy),
the permissions that need to be restored are taken from the file
descriptor that was opened for input so that integrity of the file
status is maintained all the way through to the rename.
binutils/
* rename.c
* ar.c
(write_archive) [!defined (_WIN32) || defined (__CYGWIN32__)]:
Initialize TARGET_STAT and OFD to pass to SMART_RENAME.
* arsup.c
(ar_save) [defined (_WIN32) || defined (__CYGWIN32__)]:
Likewise.
* bucomm.h (smart_rename): Add new arguments to declaration.
* objcopy.c
(strip_main)[defined (_WIN32) || defined (__CYGWIN32__)]:
Initialize COPYFD and pass to SMART_RENAME.
(copy_main) [defined (_WIN32) || defined (__CYGWIN32__)]:
Likewise.
* rename.c (try_preserve_permissions): New function.
(smart_rename): Use it and add new arguments.
Get file state from the descriptor opened by copy_file for the input
BFD. This ensures continuity in the view of the input file through
the descriptor. At the moment it is only to preserve timestamps
recorded at the point that we opened the file for input but in the
next patch this state will also be used to preserve ownership and
permissions wherever applicable.
binutils/
* objcopy.c (copy_file): New argument IN_STAT. Return stat of
ibfd through it.
(strip_main): Remove redundant stat calls. adjust copy_file
calls.
(copy_main): Likewise.
The purpose of creating a temporary file securely using mkstemp is
defeated if it is closed in make_tempname and reopened later for use;
it is as good as using mktemp. Get the file descriptor instead and
then use it to create the BFD object.
bfd/
* opncls.c (bfd_fdopenw): New function.
* bfd-in2.h: Regenerate.
binutils/
* bucomm.c (make_tempname): Add argument to return file
descriptor.
* bucomm.h (make_tempname): Likewise.
* ar.c: Include libbfd.h.
(write_archive): Adjust for change in make_tempname. Call
bfd_fdopenw instead of bfd_openw.
* objcopy.c: Include libbfd.h.
(copy_file): New argument OFD. Use bfd_fdopenw instead of
bfd_openw.
(strip_main): Adjust for change in make_tempname and
copy_file.
(copy_main): Likewise.
Alan Modra [Mon, 7 Dec 2020 06:46:46 +0000 (17:16 +1030)]
[GOLD] gcc-11 stringop-overflow warning
I'm unsure why this is deserving of a warning. Not writing the most
efficient code surely can't be a real problem, but that is what
https://gcc.gnu.org/bugzilla//show_bug.cgi?id=88059#c1 seems to say.
plugin.cc:528:10: error: 'char* strncpy(char*, const char*, size_t)' specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
528 | strncpy(tempdir, dir_template, len);
| ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
plugin.cc:526:22: note: length computed here
526 | size_t len = strlen(dir_template) + 1;
| ~~~~~~^~~~~~~~~~~~~~
* plugin.cc (Plugin_recorder::init): Replace strncpy with memcpy.
Alan Modra [Mon, 7 Dec 2020 03:33:47 +0000 (14:03 +1030)]
elf32-csky.c:3932:19: error: comparison is always false
It looks like csky missed out on an edit for 706704c8834. Not that it
matters very much. There doesn't appear to be any csky reloc howto
that sets the negate bit.
gdb/breakpoint: add flags to 'condition' and 'break' commands to force condition
introduced the '-force-condition' flag to the 'break' command. This
flag was defined as a keyword like 'thread', 'task', and 'if'.
However, it starts with '-'. This difference caused an uncovered case
when tab-completing a seemingly complete linespec.
Below, we see "-force-condition" in the completion list, where both
the options and the keywords are listed:
(gdb) break -function main <TAB>
-force-condition -function -label -line -qualified
-source if task thread
But tab-completing '-' lists only options:
(gdb) break -function main -<TAB>
-function -label -line -qualified -source
This patch fixes the problem by adding keywords to the completion
list, so that we see:
gdb/linespec: relax the position of the '-force-condition' flag
The break command's "-force-condition" flag is currently required to
be followed by the "if" keyword. This prevents flexibility when using
other keywords, e.g. "thread":
(gdb) break main -force-condition thread 1 if foo
Function "main -force-condition" not defined.
Make breakpoint pending on future shared library load? (y or [n]) n
Remove the requirement that "-force-condition" is always followed by
an "if", so that more flexibility is obtained when positioning
keywords.
gdb/ChangeLog:
2020-12-07 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* linespec.c (linespec_lexer_lex_keyword): The "-force-condition"
keyword may be followed by any keyword.
* breakpoint.c (find_condition_and_thread): Advance 'tok' by
'toklen' in the case for "-force-condition".
gdb/testsuite/ChangeLog:
2020-12-07 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdb.linespec/keywords.exp: Add tests to check positional
flexibility of "-force-condition".
gdb/main: execute breakpoint commands for '-iex' and '-ex' commands
Suppose we have the script file below:
break main
commands
print 123
end
run
If started with this script file, GDB executes the breakpoint command:
$ gdb -q -x myscript --args ./test
Reading symbols from ./test...
Breakpoint 1 at 0x114e: file test.c, line 2.
Breakpoint 1, main () at test.c:2
2 return 0;
$1 = 123
(gdb)
However, if we remove the "run" line from the script and pass it with
the '-ex' option instead, the command is not executed:
$ gdb -q -x myscript_no_run --args ./test
Reading symbols from ./test...
Breakpoint 1 at 0x114e: file test.c, line 2.
Starting program: /path/to/test
Breakpoint 1, main () at test.c:2
2 return 0;
(gdb)
If the user enters a command at this point, the breakpoint command
is executed, yielding weird output:
$ gdb -q -x myscript_no_run --args ./test
Reading symbols from ./test...
Breakpoint 1 at 0x114e: file test.c, line 2.
Starting program: /path/to/test
Breakpoint 1, main () at test.c:2
2 return 0;
(gdb) print "a"
$1 = "a"
$2 = 123
When consuming script files, GDB runs bp actions after executing a
command. See `command_handler` in event-top.c:
if (c[0] != '#')
{
execute_command (command, ui->instream == ui->stdin_stream);
/* Do any commands attached to breakpoint we stopped at. */
bpstat_do_actions ();
}
However, for '-ex' commands, `bpstat_do_actions` is not invoked.
Hence, the misaligned output explained above occurs. To fix the
problem, add a call to `bpstat_do_actions` after executing a command.
gdb/ChangeLog:
2020-12-07 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* main.c (catch_command_errors): Add a flag parameter; invoke
`bpstat_do_actions` if the flag is set.
(execute_cmdargs): Update a call to `catch_command_errors`.
gdb/testsuite/ChangeLog:
2020-12-07 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdb.base/bp-cmds-run-with-ex.c: New file.
* gdb.base/bp-cmds-run-with-ex.exp: New file.
* gdb.base/bp-cmds-run-with-ex.gdb: New file.
* gdb.gdb/python-interrupts.exp: Update the call to
'catch_command_errors' with the new argument.
* gdb.gdb/python-selftest.exp: Ditto.
Tom de Vries [Mon, 7 Dec 2020 08:07:32 +0000 (09:07 +0100)]
[gdb/ada] Handle shrink resize in replace_operator_with_call
In replace_operator_with_call, we resize the elts array like this:
...
exp->nelts = exp->nelts + 7 - oplen;
exp->resize (exp->nelts);
...
Although all the current callers ensure that the new size is bigger, it could
also be smaller, in which case the following memmove possibly reads out of
bounds:
...
memmove (exp->elts + pc + 7, exp->elts + pc + oplen,
EXP_ELEM_TO_BYTES (save_nelts - pc - oplen));
...
Fix this by doing the resize after the memmove in case the new size is
smaller.
Giancarlo Frix [Sun, 6 Dec 2020 10:27:52 +0000 (14:27 +0400)]
s390: Fix BC instruction breakpoint handling
This fixes a long-lived bug in the s390 port.
When trying to step over a breakpoint set on a BC (branch on condition)
instruction with displaced stepping on IBM Z, gdb would incorrectly
adjust the pc regardless of whether or not the branch was taken. Since
the branch target is an absolute address, this would cause the inferior
to jump around wildly whenever the branch was taken, either crashing it
or causing it to behave unpredictably.
It turns out that the logic to handle BC instructions correctly was in
the code, but that the enum value representing its opcode has always
been incorrect.
This patch corrects the enum value to the actual opcode, fixing the
stepping problem. The enum value is also used in the prologue analysis
code, so this also fixes a minor bug where more of the prologue would
be read than was necessary.
gdb/ChangeLog:
PR breakpoints/27009
* s390-tdep.h (op_bc): Correct BC opcode value.
Joel Brobecker [Sun, 6 Dec 2020 04:56:59 +0000 (23:56 -0500)]
gmp-utils: protect gdb_mpz exports against out-of-range values
The gdb_mpz class currently provides a couple of methods which
essentially export an mpz_t value into either a buffer, or an integral
type. The export is based on using the mpz_export function which
we discovered can be a bit treacherous if used without caution.
In particular, the initial motivation for this patch was to catch
situations where the mpz_t value was so large that it would not fit
in the destination area. mpz_export does not know the size of
the buffer, and therefore can happily write past the end of our buffer.
While designing a solution to the above problem, I also discovered
that we also needed to be careful when exporting signed numbers.
In particular, numbers which are larger than the maximum value
for a given signed type size, but no so large as to fit in the
*unsigned* version with the same size, would end up being exported
incorrectly. This is related to the fact that mpz_export ignores
the sign of the value being exportd, and assumes an unsigned export.
Thus, for such large values, the appears as if mpz_export is able
to fit our value into our buffer, but in fact, it does not.
Also, I noticed that gdb_mpz::write wasn't taking its unsigned_p
parameter, which was a hole.
For all these reasons, a new low-level private method called
"safe_export" has been added to class gdb_mpz, whose goal is
to perform all necessary checks and manipulations for a safe
and correct export. As a bonus, this method allows us to factorize
the handling of negative value exports.
The gdb_mpz::as_integer and gdb_mpz::write methods are then simplified
to take advantage of this new safe_export method.
gdb/ChangeLog:
* gmp-utils.h (gdb_mpz::safe_export): New private method.
(gdb_mpz::as_integer): Reimplement using gdb_mpz::safe_export.
* gmp-utils.c (gdb_mpz::write): Rewrite using gdb_mpz::safe_export.
(gdb_mpz::safe_export): New method.
* unittests/gmp-utils-selftests .c (gdb_mpz_as_integer):
Update function description.
(check_as_integer_raises_out_of_range_error): New function.
(gdb_mpz_as_integer_out_of_range): New function.
(_initialize_gmp_utils_selftests): Register
gdb_mpz_as_integer_out_of_range as a selftest.
VAX/BFD: Do not warn about GOT addend mismatches if no GOT entry is made
Match the condition used in `elf_vax_instantiate_got_entries' for the
creation of GOT entries in the processing of R_VAX_GOT32 relocations in
`elf_vax_check_relocs', removing incorrect warnings about a GOT addend
mismatch like:
./ld-new: tmpdir/got-local-ref-off-r.o: warning: GOT addend of 1 to `bar_hidden' does not match previous GOT addend of 0
./ld-new: tmpdir/got-local-ref-off-r.o: warning: GOT addend of 2 to `bar_hidden' does not match previous GOT addend of 0
and corresponding failures with the test cases newly added here:
FAIL: GOT test (executable hidden reference with offset)
FAIL: GOT test (executable visible reference with offset)
for symbols that are considered local for reasons other than having been
forced local with a version script, which is usually the ELF visibility.
Correct code is produced regardless, but the warning breaks `-Werror'
compilation and may upset people regardless.
Interestingly this shows with executable links only, because in shared
library links code from `elf_link_add_object_symbols' triggers:
/* If the symbol already has a dynamic index, but
visibility says it should not be visible, turn it into
a local symbol. */
switch (ELF_ST_VISIBILITY (h->other))
{
case STV_INTERNAL:
case STV_HIDDEN:
(*bed->elf_backend_hide_symbol) (info, h, TRUE);
dynsym = FALSE;
break;
}
that sets `h->forced_local' like with a version script.
Add suitable test cases including disassembly to verify correct code has
been produced where no warnings have been issued, and that warnings do
get issued where necessary. Do not verify (broken) code produced in the
latter case; we should probably make the warning an error, or preferably
actually start supporting GOT references with different addends as they
appear feasible with explicitly relocated GOT that we use.
bfd/
* elf32-vax.c (elf_vax_check_relocs) <R_VAX_GOT32>: Use
SYMBOL_REFERENCES_LOCAL rather than `h->forced_local' to check
whether the symbol referred is local or not.
ld/
* testsuite/ld-vax-elf/got-local-exe-off-hidden.dd: New test
dump.
* testsuite/ld-vax-elf/got-local-exe-off-visible.dd: New test
dump.
* testsuite/ld-vax-elf/got-local-lib-off-hidden.dd: New test
dump.
* testsuite/ld-vax-elf/got-local-lib-off-visible.ed: New test
dump.
* testsuite/ld-vax-elf/got-local-off-external.ed: New test dump.
* testsuite/ld-vax-elf/got-local-exe-off.xd: New test dump.
* testsuite/ld-vax-elf/got-local-lib-off.xd: New test dump.
* testsuite/ld-vax-elf/got-local.ld: New test linker script.
* testsuite/ld-vax-elf/got-local-aux-off.s: New test source.
* testsuite/ld-vax-elf/got-local-def-off.s: New test source.
* testsuite/ld-vax-elf/got-local-ref-off-external.s: New test
source.
* testsuite/ld-vax-elf/got-local-ref-off-hidden.s: New test
source.
* testsuite/ld-vax-elf/got-local-ref-off-visible.s: New test
source.
* testsuite/ld-vax-elf/vax-elf.exp: Run the new tests.
Joel Brobecker [Sat, 5 Dec 2020 08:03:48 +0000 (03:03 -0500)]
Fix TARGET_CHAR_BIT/HOST_CHAR_BIT confusion in gmp-utils.c
In a couple of gdb_mpz methods, we are computing the number of
bits in a gdb::array_view of gdb_byte. Since gdb_byte is defined
using a host-side type (see common-types.h), the number of bits
in a gdb_byte should be HOST_CHAR_BIT, not TARGET_CHAR_BIT.
gdb/ChangeLog:
* gmp-utils.c (gdb_mpz::read): Use HOST_CHAR_BIT instead of
TARGET_CHAR_BIT.
(gdb_mpz::write): Likewise.
H.J. Lu [Sat, 5 Dec 2020 02:54:47 +0000 (18:54 -0800)]
x86-64: Convert load to mov only for GOTPCRELX relocations
Since converting load to mov needs to rewrite the REX byte and we don't
know if there is a REX byte with GOTPCREL relocation, do it only for
GOTPCRELX relocations.
bfd/
PR ld/27016
* elf64-x86-64.c (elf_x86_64_convert_load_reloc): Convert load
to mov only for GOTPCRELX relocations.
ld/
PR ld/27016
* testsuite/ld-x86-64/x86-64.exp: Run pr27016a and pr27016b.
* testsuite/ld-x86-64/pr27016a.d: New file.
* testsuite/ld-x86-64/pr27016a.s: Likewise.
* testsuite/ld-x86-64/pr27016b.d: Likewise.
* testsuite/ld-x86-64/pr27016b.s: Likewise.
Simon Marchi [Fri, 4 Dec 2020 21:44:55 +0000 (16:44 -0500)]
gdb: use two displaced step buffers on amd64/Linux
As observed on a binary compiled on AMD64 Ubuntu 20.04, against glibc
2.31 (I think it's the libc that provides this startup code, right?),
there are enough bytes at the executable's entry point to hold more than
one displaced step buffer. gdbarch_max_insn_length is 16, and the
code at _start looks like:
Even though there's a _start_c symbol, it all appears to be code that
runs once at the very beginning of the program, so it looks fine if the
two buffers occupy [0x1048, 0x1068).
One important thing I discovered while doing this is that when debugging
a dynamically-linked executable, breakpoints in the shared library
loader are hit before executing the _start code, and these breakpoints
may be displaced-stepped. So it's very important that the buffer bytes
are restored properly after doing the displaced steps, otherwise the
_start code will be corrupted once we try to execute it.
Another thing that made me think about is that library constructors (as
in `__attribute__((constructor))`) run before _start. And they are free
to spawn threads. What if one of these threads executes a displaced
step, therefore changing the bytes at _start, while the main thread
executes _start? That doesn't sound good and I don't know how we could
prevent it. But this is a problem that predates the current patch.
Even when stress-testing the implementation, by making many threads do
displaced steps over and over, I didn't see a significant performance (I
confirmed that the two buffers were used by checking the "set debug
displaced" logs though). However, this patch mostly helps make the
feature testable by anybody with an AMD64/Linux machine, so I think it's
useful.
gdb/ChangeLog:
* amd64-linux-tdep.c (amd64_linux_init_abi): Pass 2 as the
number of displaced step buffers.
Simon Marchi [Fri, 4 Dec 2020 21:43:56 +0000 (16:43 -0500)]
gdb: make displaced stepping implementation capable of managing multiple buffers
The displaced_step_buffer class, introduced in the previous patch,
manages access to a single displaced step buffer. Change it into
displaced_step_buffers (note the plural), which manages access to
multiple displaced step buffers.
When preparing a displaced step for a thread, it looks for an unused
buffer.
For now, all users still pass a single displaced step buffer, so no real
behavior change is expected here. The following patch makes a user pass
more than one buffer, so the functionality introduced by this patch is
going to be useful in the next one.
Simon Marchi [Fri, 4 Dec 2020 21:43:56 +0000 (16:43 -0500)]
gdb: change linux gdbarch data from post to pre-init
The following patch will need to fill a field in linux_gdbarch_data
while the gdbarch is being built. linux_gdbarch_data is currently
allocated as a post-init gdbarch data, meaning it's not possible to fill
it before the gdbarch is completely initialized. Change it to a
pre-init gdbarch data to allow this.
The init_linux_gdbarch_data function doesn't use the created gdbarch,
it only allocates the linux_gdbarch_data structure on the gdbarch's
obstack, so the change is trivial.
gdb/ChangeLog:
* linux-tdep.c (init_linux_gdbarch_data): Change parameter to
obkstack.
(_initialize_linux_tdep): Register pre-init gdb data instead of
post-init.
Today, GDB only allows a single displaced stepping operation to happen
per inferior at a time. There is a single displaced stepping buffer per
inferior, whose address is fixed (obtained with
gdbarch_displaced_step_location), managed by infrun.c.
In the case of the AMD ROCm target [1] (in the context of which this
work has been done), it is typical to have thousands of threads (or
waves, in SMT terminology) executing the same code, hitting the same
breakpoint (possibly conditional) and needing to to displaced step it at
the same time. The limitation of only one displaced step executing at a
any given time becomes a real bottleneck.
To fix this bottleneck, we want to make it possible for threads of a
same inferior to execute multiple displaced steps in parallel. This
patch builds the foundation for that.
In essence, this patch moves the task of preparing a displaced step and
cleaning up after to gdbarch functions. This allows using different
schemes for allocating and managing displaced stepping buffers for
different platforms. The gdbarch decides how to assign a buffer to a
thread that needs to execute a displaced step.
On the ROCm target, we are able to allocate one displaced stepping
buffer per thread, so a thread will never have to wait to execute a
displaced step.
On Linux, the entry point of the executable if used as the displaced
stepping buffer, since we assume that this code won't get used after
startup. From what I saw (I checked with a binary generated against
glibc and musl), on AMD64 we have enough space there to fit two
displaced stepping buffers. A subsequent patch makes AMD64/Linux use
two buffers.
In addition to having multiple displaced stepping buffers, there is also
the idea of sharing displaced stepping buffers between threads. Two
threads doing displaced steps for the same PC could use the same buffer
at the same time. Two threads stepping over the same instruction (same
opcode) at two different PCs may also be able to share a displaced
stepping buffer. This is an idea for future patches, but the
architecture built by this patch is made to allow this.
Now, the implementation details. The main part of this patch is moving
the responsibility of preparing and finishing a displaced step to the
gdbarch. Before this patch, preparing a displaced step is driven by the
displaced_step_prepare_throw function. It does some calls to the
gdbarch to do some low-level operations, but the high-level logic is
there. The steps are roughly:
- Ask the gdbarch for the displaced step buffer location
- Save the existing bytes in the displaced step buffer
- Ask the gdbarch to copy the instruction into the displaced step buffer
- Set the pc of the thread to the beginning of the displaced step buffer
Similarly, the "fixup" phase, executed after the instruction was
successfully single-stepped, is driven by the infrun code (function
displaced_step_finish). The steps are roughly:
- Restore the original bytes in the displaced stepping buffer
- Ask the gdbarch to fixup the instruction result (adjust the target's
registers or memory to do as if the instruction had been executed in
its original location)
The displaced_step_inferior_state::step_thread field indicates which
thread (if any) is currently using the displaced stepping buffer, so it
is used by displaced_step_prepare_throw to check if the displaced
stepping buffer is free to use or not.
This patch defers the whole task of preparing and cleaning up after a
displaced step to the gdbarch. Two new main gdbarch methods are added,
with the following semantics:
- gdbarch_displaced_step_prepare: Prepare for the given thread to
execute a displaced step of the instruction located at its current PC.
Upon return, everything should be ready for GDB to resume the thread
(with either a single step or continue, as indicated by
gdbarch_displaced_step_hw_singlestep) to make it displaced step the
instruction.
- gdbarch_displaced_step_finish: Called when the thread stopped after
having started a displaced step. Verify if the instruction was
executed, if so apply any fixup required to compensate for the fact
that the instruction was executed at a different place than its
original pc. Release any resources that were allocated for this
displaced step. Upon return, everything should be ready for GDB to
resume the thread in its "normal" code path.
The displaced_step_prepare_throw function now pretty much just offloads
to gdbarch_displaced_step_prepare and the displaced_step_finish function
offloads to gdbarch_displaced_step_finish.
The gdbarch_displaced_step_location method is now unnecessary, so is
removed. Indeed, the core of GDB doesn't know how many displaced step
buffers there are nor where they are.
To keep the existing behavior for existing architectures, the logic that
was previously implemented in infrun.c for preparing and finishing a
displaced step is moved to displaced-stepping.c, to the
displaced_step_buffer class. Architectures are modified to implement
the new gdbarch methods using this class. The behavior is not expected
to change.
The other important change (which arises from the above) is that the
core of GDB no longer prevents concurrent displaced steps. Before this
patch, start_step_over walks the global step over chain and tries to
initiate a step over (whether it is in-line or displaced). It follows
these rules:
- if an in-line step is in progress (in any inferior), don't start any
other step over
- if a displaced step is in progress for an inferior, don't start
another displaced step for that inferior
After starting a displaced step for a given inferior, it won't start
another displaced step for that inferior.
In the new code, start_step_over simply tries to initiate step overs for
all the threads in the list. But because threads may be added back to
the global list as it iterates the global list, trying to initiate step
overs, start_step_over now starts by stealing the global queue into a
local queue and iterates on the local queue. In the typical case, each
thread will either:
- have initiated a displaced step and be resumed
- have been added back by the global step over queue by
displaced_step_prepare_throw, because the gdbarch will have returned
that there aren't enough resources (i.e. buffers) to initiate a
displaced step for that thread
Lastly, if start_step_over initiates an in-line step, it stops
iterating, and moves back whatever remaining threads it had in its local
step over queue to the global step over queue.
Two other gdbarch methods are added, to handle some slightly annoying
corner cases. They feel awkwardly specific to these cases, but I don't
see any way around them:
- gdbarch_displaced_step_copy_insn_closure_by_addr: in
arm_pc_is_thumb, arm-tdep.c wants to get the closure for a given
buffer address.
- gdbarch_displaced_step_restore_all_in_ptid: when a process forks
(at least on Linux), the address space is copied. If some displaced
step buffers were in use at the time of the fork, we need to restore
the original bytes in the child's address space.
These two adjustments are also made in infrun.c:
- prepare_for_detach: there may be multiple threads doing displaced
steps when we detach, so wait until all of them are done
- handle_inferior_event: when we handle a fork event for a given
thread, it's possible that other threads are doing a displaced step at
the same time. Make sure to restore the displaced step buffer
contents in the child for them.
Simon Marchi [Fri, 4 Dec 2020 21:43:55 +0000 (16:43 -0500)]
gdb: move displaced stepping types to displaced-stepping.{h,c}
Move displaced-stepping related stuff unchanged to displaced-stepping.h
and displaced-stepping.c. This helps make the following patch a bit
smaller and easier to read.
Simon Marchi [Fri, 4 Dec 2020 21:43:54 +0000 (16:43 -0500)]
gdb: pass inferior to get_linux_inferior_data
Pass to get_linux_inferior_data the inferior for which we want to obtain
the linux-specific data, rather than assuming the current inferior.
This helps slightly reduce the diff in the upcoming main patch.
Update the sole caller to pass the current inferior.
gdb/ChangeLog:
* linux-tdep.c (get_linux_inferior_data): Add inferior
parameter.
(linux_vsyscall_range): Pass current inferior.
Simon Marchi [Fri, 4 Dec 2020 21:43:54 +0000 (16:43 -0500)]
gdb: introduce status enum for displaced step prepare/finish
This is a preparatory patch to reduce the size of the diff of the
upcoming main patch. It introduces enum types for the return values of
displaced step "prepare" and "finish" operations. I find that this
expresses better the intention of the code, rather than returning
arbitrary integer values (-1, 0 and 1) which are difficult to remember.
That makes the code easier to read.
I put the new enum types in a new displaced-stepping.h file, because I
introduce that file in a later patch anyway. Putting it there avoids
having to move it later.
There is one change in behavior for displaced_step_finish: it currently
returns 0 if the thread wasn't doing a displaced step and 1 if the
thread was doing a displaced step which was executed successfully. It
turns out that this distinction is not needed by any caller, so I've
merged these two cases into "_OK", rather than adding an extra
enumerator.
gdb/ChangeLog:
* infrun.c (displaced_step_prepare_throw): Change return type to
displaced_step_prepare_status.
(displaced_step_prepare): Likewise.
(displaced_step_finish): Change return type to
displaced_step_finish_status.
(resume_1): Adjust.
(stop_all_threads): Adjust.
* displaced-stepping.h: New file.
Simon Marchi [Fri, 4 Dec 2020 21:43:54 +0000 (16:43 -0500)]
gdb: rename displaced_step_fixup to displaced_step_finish
This is a preparatory patch to reduce a little bit the diff size of the
main patch later in this series. It renames the displaced_step_fixup
function in infrun.c to displaced_step_finish.
The rationale is to better differentiate the low and high level
operations.
We first have the low level operation of writing an instruction to a
displaced buffer, called "copy_insn". The mirror low level operation to
fix up the state after having executed the instruction is "fixup". The
high level operation of preparing a thread for a displaced step (which
includes doing the "copy_insn" and some more bookkeeping) is called
"prepare" (as in displaced_step_prepare). The mirror high level
operation to cleaning up after a displaced step (which includes doing
the "fixup" and some more bookkeeping) is currently also called "fixup"
(as in displaced_step_fixup), just like the low level operation.
I think that choosing a different name for the low and high level
cleanup operation makes it clearer, hence "finish".
Simon Marchi [Fri, 4 Dec 2020 21:43:53 +0000 (16:43 -0500)]
gdb: rename displaced_step_closure to displaced_step_copy_insn_closure
Since we're going to introduce other "displaced step" functions and
another kind of displaced step closure, make it clear that this is the
return type of the gdbarch_displaced_step_copy_insn function.
Simon Marchi [Fri, 4 Dec 2020 21:43:53 +0000 (16:43 -0500)]
gdb: rename things related to step over chains
Rename step_over_queue_head to global_thread_step_over_chain_head, to
make it more obvious when reading code that we are touching the global
queue. Rename all functions that operate on it to have "global" in
their name, to make it clear on which chain they operate on. Also, in a
subsequent patch, we'll need both global and non-global versions of
these functions, so it will be easier to do the distinction if they are
named properly.
Normalize the naming to use "chain" everywhere instead of sometimes
"queue", sometimes "chain".
I also reworded a few comments in gdbthread.h. They implied that the
step over chain is per-inferior, when in reality there is only one
global chain, not one per inferior, as far as I understand.
gdb/ChangeLog:
* gdbthread.h (thread_step_over_chain_enqueue): Rename to...
(global_thread_step_over_chain_enqueue): ... this. Update all
users.
(thread_step_over_chain_remove): Rename to...
(global_thread_step_over_chain_remove): ... this. Update all
users.
(thread_step_over_chain_next): Rename to...
(global_thread_step_over_chain_next): ... this. Update all
users.
* infrun.h (step_over_queue_head): Rename to...
(global_thread_step_over_chain_head): ... this. Update all
users.
* infrun.c (step_over_queue_head): Rename to...
(global_thread_step_over_chain_head): ... this. Update all
users.
* thread.c (step_over_chain_remove): Rename to...
(thread_step_over_chain_remove): ... this. Update all users.
(thread_step_over_chain_next): Rename to...
(global_thread_step_over_chain_next): ... this. Update all
users.
(thread_step_over_chain_enqueue): Rename to...
(global_thread_step_over_chain_enqueue): ... this. Update all
users.
(thread_step_over_chain_remove): Rename to...
(global_thread_step_over_chain_remove): ... this. Update all
users.
Simon Marchi [Fri, 4 Dec 2020 21:43:53 +0000 (16:43 -0500)]
gdb: get rid of get_displaced_stepping_state
Remove function get_displaced_stepping_state. When it was introduced,
inferiors' displaced stepping state was kept in a linked list in
infrun.c, so it was handy. Nowadays, the state is kept inside struct
inferior directly, so we can just access it directly instead.
gdb/ChangeLog:
* infrun.c (get_displaced_stepping_state): Remove, change
callers to access the field directly.
Simon Marchi [Fri, 4 Dec 2020 21:43:52 +0000 (16:43 -0500)]
gdb: restore displaced step buffer bytes when another thread forks
In handle_inferior_event, where we handle forks, we make sure to restore
the bytes of the displaced stepping buffer in the child's address
space. However, we only do it when the forking thread was the one
doing a displaced step. It could happen that a thread forks while
another one is doing a displaced step. In this case, we also need to
restore the bytes in the child.
Move the byte-restoring code outside of the condition that checks
whether the event thread was displaced stepping.
gdb/ChangeLog:
* infrun.c (handle_inferior_event): Restore displaced step
buffer bytes in child process when handling fork, even if fork
happened in another thread than the displaced-stepping one.
Simon Marchi [Fri, 4 Dec 2020 21:43:52 +0000 (16:43 -0500)]
gdb: clear inferior displaced stepping state and in-line step-over info on exec
When a process does an exec, all its program space is replaced with the
newly loaded executable. All non-main threads disappear and the main
thread starts executing at the entry point of the new executable.
Things can go wrong if a displaced step operation is in progress while
we process the exec event.
If the main thread is the one executing the displaced step: when that
thread (now executing in the new executable) stops somewhere (say, at a
breakpoint), displaced_step_fixup will run and clear up the state. We
will execute the "fixup" phase for the instruction we single-stepped in
the old program space. We are now in a completely different context,
so doing the fixup may corrupt the state.
If it is a non-main thread that is doing the displaced step: while
handling the exec event, GDB deletes the thread_info representing that
thread (since the thread doesn't exist in the inferior after the exec).
But inferior::displaced_step_state::step_thread will still point to it.
When handling events later, this condition, in displaced_step_fixup,
will likely never be true:
/* Was this event for the thread we displaced? */
if (displaced->step_thread != event_thread)
return 0;
... since displaced->step_thread points to a deleted thread (unless that
storage gets re-used for a new thread_info, but that wouldn't be good
either). This effectively makes the displaced stepping buffer occupied
for ever. When a thread in the new program space will want to do a
displaced step, it will wait for ever.
I think we simply need to reset the displaced stepping state of the
inferior on exec. Everything execution-related that existed before the
exec is now gone.
Similarly, if a thread does an in-line step over an exec syscall
instruction, nothing clears the in-line step over info when the event is
handled. So it the in-line step over info stays there indefinitely, and
things hang because we can never start another step over. To fix this,
I added a call to clear_step_over_info in infrun_inferior_execd.
Add a test with a program with two threads that does an exec. The test
includes the following axes:
- whether it's the leader thread or the other thread that does the exec.
- whether the exec'r and exec'd program have different text segment
addresses. This is to hopefully catch cases where the displaced
stepping info doesn't get reset, and GDB later tries to restore bytes
of the old address space in the new address space. If the mapped
addresses are different, we should get some memory error. This
happens without the patch applied:
$ ./gdb -q -nx --data-directory=data-directory testsuite/outputs/gdb.threads/step-over-exec/step-over-exec-execr-thread-leader-diff-text-segs-true -ex "b main" -ex r -ex "b my_execve_syscall if 0" -ex "set displaced-stepping on"
...
Breakpoint 1, main (argc=1, argv=0x7fffffffde38) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.threads/step-over-exec.c:69
69 argv0 = argv[0];
Breakpoint 2 at 0x60133a: file /home/simark/src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S, line 34.
(gdb) c
Continuing.
[New Thread 0x7ffff7c62640 (LWP 1455423)]
Leader going in exec.
Exec-ing /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-exec/step-over-exec-execr-thread-leader-diff-text-segs-true-execd
[Thread 0x7ffff7c62640 (LWP 1455423) exited]
process 1455418 is executing new program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-exec/step-over-exec-execr-thread-leader-diff-text-segs-true-execd
Error in re-setting breakpoint 2: Function "my_execve_syscall" not defined.
No unwaited-for children left.
(gdb) n
Single stepping until exit from function _start,
which has no line number information.
Cannot access memory at address 0x6010d2
(gdb)
- Whether displaced stepping is allowed or not, so that we end up
testing both displaced stepping and in-line stepping on arches that do
support displaced stepping (otherwise, it just tests in-line stepping
twice I suppose)
To be able to precisely put a breakpoint on the syscall instruction, I
added a small assembly file (lib/my-syscalls.S) that contains minimal
Linux syscall wrappers. I prefer that to the strategy used in
gdb.base/step-over-syscall.exp, which is to stepi into the glibc wrapper
until we find something that looks like a syscall instruction, I find
that more predictable.
gdb/ChangeLog:
* infrun.c (infrun_inferior_execd): New function.
(_initialize_infrun): Attach inferior_execd observer.
Simon Marchi [Fri, 4 Dec 2020 21:43:51 +0000 (16:43 -0500)]
gdb: add inferior_execd observable
I want to add another action (clearing displaced stepping state) that
happens when an inferior execs. I think it would be cleaner to have an
observer for this event, rather than have infrun know about each other
sub-component.
Replace the calls to solib_create_inferior_hook and
jit_inferior_created_hook in follow_exec by observers.
Tom de Vries [Fri, 4 Dec 2020 21:35:07 +0000 (22:35 +0100)]
[gdb] Fix heap-buffer-overflow in completion_tracker::build_completion_result
When building gdb with address sanitizer and running test-case
gdb.base/completion.exp, we run into:
...
==5743==ERROR: AddressSanitizer: heap-buffer-overflow on address \
0x60200025c02f at pc 0x000000cd9d64 bp 0x7fff3297da30 sp 0x7fff3297da28
READ of size 1 at 0x60200025c02f thread T0
#0 0xcd9d63 in completion_tracker::build_completion_result(char const*, \
int, int) gdb/completer.c:2258
...
0x60200025c02f is located 1 bytes to the left of 1-byte region \
[0x60200025c030,0x60200025c031)
...
This can be reproduced using just:
...
$ gdb
(gdb) p/d[TAB]
...
The problem is in this code in completion_tracker::build_completion_result:
...
bool completion_suppress_append
= (suppress_append_ws ()
|| match_list[0][strlen (match_list[0]) - 1] == ' ');
...
If strlen (match_list[0]) == 0, then we access match_list[0][-1].
Fix this by testing if the memory access is in bounds before doing the memory
access.
... to remove other typedefs that are no longer necessary now that gdb
uses C++.
I didn't remove absolutely every one -- I didn't touch the tdep files.
However, I removed many of them. In some cases, I removed an existing
different struct tag.
Simon Marchi [Fri, 4 Dec 2020 20:08:54 +0000 (15:08 -0500)]
gdb/testsuite: make declare_labels use better default label names
When using the single-element form of argument to declare_labels, the
generated label (in the assembly file) is of the format ".LlabelN",
where N is a number.
I propose making it use the name of the label by default. Calling:
declare_labels foo
will generate the ".LfooN" in the assembly file (again, where N is a
number). When debugging the output of the DWARF assembler, it makes it
easier to map labels to the source. Also, when defining the same label
twice by mistake in the Tcl code (like I d id), it's easier to track the
error from the message to the root cause:
-/home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/implptrpiece/implptrpiece-dw.S:62: Error: symbol `.Llabel5' is already defined
+/home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/implptrpiece/implptrpiece-dw.S:62: Error: symbol `.Lvar_label5' is already defined
This doesn't change anything for the test cases, it just makes the
assembly output a bit nicer.
gdb/testsuite/ChangeLog:
* lib/dwarf.exp (declare_labels): Use name as text if text is
not provided.
Tom de Vries [Fri, 4 Dec 2020 12:36:48 +0000 (13:36 +0100)]
[gdb/tdep] Handle static field in i386_16_byte_align_p
When running test-case on gdb.cp/many-args.exp with target board unix/-m32, I
run into:
...
(gdb) p check_val (ref_val, ref_val, ... , ref_val, ref_val)^M
$1 = false^M
(gdb) FAIL: gdb.cp/many-args.exp: check passing many structures
...
The test source contains struct ss:
...
typedef int v4si __attribute__ ((vector_size (16)));
struct ss
{
static v4si static_field;
unsigned char aa;
};
...
and i386_16_byte_align_p returns true for this type.
Fix this by skipping static fields in i386_16_byte_align_p.
Tom de Vries [Fri, 4 Dec 2020 12:36:48 +0000 (13:36 +0100)]
[gdb/testsuite] Fix control-flow in gdb.reverse/insn-reverse.exp
In gdb.reverse/insn-reverse.exp, we have loop containing a call to
gdb_test_multiple, which itself contains a break:
...
for {} {$count < 500} {incr count} {
...
gdb_test_multiple "x/i \$pc" "" {
...
break
}
...
On SLE-11 with:
...
$ runtest --version
Expect version is 5.44.1.11
Tcl version is 8.5
Framework version is 1.4.4
...
the break doesn't seem to have the effect of breaking out of the loop.
The break does have the effect of terminating evaluation of the expect clause,
which means we don't set insn_array, after which we run into:
...
ERROR: tcl error sourcing src/gdb/testsuite/gdb.reverse/insn-reverse.exp.
ERROR: can't read "insn_array(5)": no such element in array
...
Tom de Vries [Fri, 4 Dec 2020 12:36:47 +0000 (13:36 +0100)]
[gdb/testsuite] Fix count usage in gdb.reverse/insn-reverse.exp
Consider the test-case gdb.reverse/insn-reverse.exp.
After the loop setting count, the valid entries in various arrays range from 0
to $count - 1 inclusive.
Then $count is decremented:
...
incr count -1
...
after which the valid entries range from 0 to $count inclusive.
The first subsequent loop handles that properly:
...
for {set i $count} {$i >= 0} {incr i -1} {
...
but the following loop does not, because it treats $count as exclusive bound:
...
for {set i 0} {$i < $count} {incr i} {
...
Fix this by removing the incr, and using $count - 1 as starting value in the
first loop.
gdb/testsuite/ChangeLog:
2020-12-04 Tom de Vries <tdevries@suse.de>
* gdb.reverse/insn-reverse.exp: Fix count handling.
Tom de Vries [Fri, 4 Dec 2020 12:36:47 +0000 (13:36 +0100)]
[gdb/testsuite] Handle SIGILL in gdb.reverse/insn-reverse.exp
Consider test-case gdb.reverse/insn-reverse.exp.
It runs a number of subtests, dependent on the architecture, f.i. for
x86_64 it runs subtests rdrand and rdseed.
For each subtest, it checks whether the subtest is supported and otherwise
bails out of that subtest.
However, there may be a problem with the support test or the information it
relies on, and if it states that a subtest is supported while it is actually
not, we may run into a SIGILL, as f.i. described in PR21166, which results in
tcl errors like this:
...
ERROR: tcl error sourcing src/gdb/testsuite/gdb.reverse/insn-reverse.exp.
ERROR: can't read "insn_array(5)": no such element in array
...
We can emulate this by inserting a sigfpe in function rdrand in
insn-reverse-x86.c, like this:
...
volatile int a = 0; volatile int b = 1; volatile int c = b / a;
...
The problem is that the loop in the test-case attempts to stepi over of all
insn in rdrand, but because of the signal it will never get to the last insn.
Handle this by detecting that the stepi made no progress, and bailing out of
the loop.
Furthermore, make running of the subtests independent, such that a SIGILL in
subtest rdrand does not affect running of subtest rdseed.
Tested on x86_64-linux.
gdb/testsuite/ChangeLog:
2020-12-04 Tom de Vries <tdevries@suse.de>
* gdb.reverse/insn-reverse.c (test_nr): New var.
(usage, parse_args): New function.
(main): Call parse_args. Only run test for test_nr.
* gdb.reverse/insn-reverse.exp: Detect lack of progress in stepi loop
and bail out. Run subtests individually, using an inferior arg
specifying the subtest.
Andreas Krebbel [Fri, 4 Dec 2020 08:00:43 +0000 (09:00 +0100)]
IBM Z: Add risbgz and risbgnz extended mnemonics
These two extended mnemonics are documented in the Principles of
Operations manual but currently not supported by Binutils. They
provide aliases for already supported instructions with the zero flag
being set. The flag otherwise is mingled into one of the immediate
operands what makes asm code much harder to read.
opcodes/
* s390-opc.txt: Add risbgz and risbgnz.
* s390-opc.c (U6_26): New operand type.
(INSTR_RIE_RRUUU2, MASK_RIE_RRUUU2): New instruction format and
mask.
gas/
* testsuite/gas/s390/zarch-z10.s: Add tests for risbgz.
* testsuite/gas/s390/zarch-z10.d: Add regexp for risbgz.
* testsuite/gas/s390/zarch-zEC12.s: Add tests for risbgnz.
* testsuite/gas/s390/zarch-zEC12.d: Add regexp for risbgnz.
Alan Modra [Thu, 3 Dec 2020 05:40:37 +0000 (16:10 +1030)]
asan: readelf: memory leaks
This tidies some code used by readelf, hopefully fixing some
intermittent oss-fuzz bug reports that likely could only be reproduced
by feeding readelf two or more object files on the command line. The
second and subsequent file may see non-zero state in .bss variables,
and non-initial values in .data variables. This patch fixes some of
those, and moves some .data variables to .rodata.
* dwarf.c (frame_display_row): Do without static variable "sloc".
(cu_tu_indexes_read): Move to file scope.
(free_debug_memory): Reset it here, along with level_type_signed.
Free and clear a number of other static variables.
* readelf.c (arm_attr_public_tag <table>): Constify, updating..
(arm_attr_tag_*): ..all these uses.
(process_mips_specific): Free "rels" on error path.
Alan Modra [Wed, 2 Dec 2020 02:33:23 +0000 (13:03 +1030)]
PR26978, Inconsistency for strong foo@v1 and weak foo@@v1
Prior to this patch
ld -shared --version-script=pr26979.ver pr26978a.o pr26978b.o
results in
ld: pr26978b.o: in function `foo_v1':
(.text+0x0): multiple definition of `foo@v1'
ld: pr26978b.o:(*IND*+0x0): multiple definition of `foo'
while
ld -shared --version-script=pr26979.ver pr26978b.o pr26978a.o
results in no error, but some odd dynamic symbols.
... 0 NOTYPE GLOBAL DEFAULT 7 foo@v1
... 0 NOTYPE WEAK DEFAULT 7 foo@@v1
When linking an undecorated reference to foo against such a shared
library, ld complains about multiple definitions of foo@v1 while gold
creates a dynamic reference to foo@v1. That results in foo@v1 being
used at runtime.
While we could error in both cases, it is reasonable to say foo@v1 and
foo@@v1 are in fact the same symbol. (Same name, same version. The
only real difference is that foo@@v1 satisfies a reference to plain
foo, while foo@v1 does not.) Just as merging a weak undecorated sym
with a strong sym results in the strong sym prevailing, so should the
strong foo@v1 prevail. And since there is a definition that satisfies
plain foo, the foo@@v1 variety of dynamic symbol should be emitted at
the foo@v1 value. That makes the testcase that currently links
continue to produce a shared library, and that shared library can now
be used by both ld and gold with the same runtime behaviour as when
using gold with the odd dynamic symbol library.
bfd/
PR 26978
* elflink.c (_bfd_elf_add_default_symbol): Handle the case where
a new weak sym@@ver should be overridden by an existing sym@ver.
(elf_link_add_object_symbols): Don't _bfd_elf_add_default_symbol
for a new weak sym@ver when sym@@ver already exists.
* linker.c (link_action): Choose MIND for previous indirect,
current def, rather than MDEF.
(_bfd_generic_link_add_one_symbol <MIND>): Handle redefinition of
weak indirect symbol.
ld/
* testsuite/ld-elf/pr26978a.d,
* testsuite/ld-elf/pr26978a.s,
* testsuite/ld-elf/pr26978b.d,
* testsuite/ld-elf/pr26978b.s: New tests.
Simon Marchi [Thu, 3 Dec 2020 20:47:56 +0000 (15:47 -0500)]
gdb: fix logic of find_comp_unit and set_comp_unit
The logic in find_comp_unit and set_comp_unit is reversed. When the BFD
requires relocation, we want to put the comp_unit structure in the
map where the comp_unit objects are not shared, that is the one indexed
by objfile. If the BFD does not require relocation, then, we can share
a single comp_unit structure for all users of that BFD, so we want to
put it in the BFD-indexed map. The comments on top of
dwarf2_frame_bfd_data and dwarf2_frame_objfile_data make that clear.
Fix it by swapping the two in find_comp_unit and set_comp_unit.
I don't have a test for this, because I don't see how to write one in a
reasonable amount of time.
gdb/ChangeLog:
PR gdb/26876
* dwarf2/frame.c (find_comp_unit, set_comp_unit): Reverse use of
dwarf2_frame_bfd_data and dwarf2_frame_objfile_data.
Andreas Krebbel [Thu, 3 Dec 2020 15:31:15 +0000 (16:31 +0100)]
IBM Z: Add support for HLASM extended mnemonics
Add extended mnemonics used in the HLASM assembler. All of them are
just aliases for instructions we already support and help when
assembling code which was written for the HLASM assembler.
The HLASM mnemonics are documented here:
https://www.ibm.com/support/knowledgecenter/SSENW6_1.6.0/com.ibm.hlasm.v1r6.asm/asmr1023.pdf
See the 'Branching with extended mnemonic codes' chapter.
objdump will still print the existing mnemonics with the exception of
relative nop branches (i.e. conditional branches with an empty
condition code mask). Now we have jnop and jgnop which will be used
by objdump when possible.
The same change have been applied to the LLVM assembler:
https://reviews.llvm.org/D92185
Andrew Burgess [Mon, 23 Nov 2020 18:03:32 +0000 (18:03 +0000)]
gdb/riscv: rewrite target description validation, add rv32e support
This commit started as adding rv32e support to gdb. The rv32e
architecture is a cut-down rv32i, it only has 16 x-registers compared
to the usual 32, and an rv32e target should not have any floating
point registers.
In order to add this I needed to adjust the target description
validation checks that are performed from riscv_gdbarch_init, and I
finally got fed up with the current scheme of doing these checks and
rewrote this code.
Unfortunately the rv32e changes are currently mixed in with the
rewrite of the validation scheme. I could split these apart if anyone
is really interested in seeing these two ideas as separate patches.
The main idea behind this change is that where previously I tried to
have a purely data driven approach, a set of tables one for each
expected feature, and then a single generic function that would
validate a feature given a table, I have created a new class for each
feature. Each class has its own check member function which allows
the logic for how to check each feature to be different. I think the
new scheme is much easier to follow.
There are some other changes that I made to the validation code as
part of this commit.
I've relaxed some of the checks related to the floating point CSRs.
Previously the 3 CSRs fflags, frm, and fcsr all had to be present in
either the fpu feature or the csr feature. This requirement is now
relaxed, if the CSRs are not present then gdb will not reject the
target description. My thinking here is that there's no gdb
functionality that specifically requires these registers, and so, if a
target offers a description without these registers nothing else in
gdb should stop working.
And as part of the rv32e support targets now only have to provide the
first 16 x-registers and $pc. The second half of the x-registers (x16
-> x31) are now optional.
gdb/ChangeLog:
* arch/riscv.c: Include 'rv32e-xregs.c'.
(riscv_create_target_description): Update to handle rv32e.
* arch/riscv.h (struct riscv_gdbarch_features) <embedded>: New
member variable.
<operator==>: Update to account for new field.
<hash>: Likewise.
* features/Makefile (FEATURE_XMLFILES): Add riscv/rv32e-xregs.xml.
* features/riscv/rv32e-xregs.c: Generated.
* features/riscv/rv32e-xregs.xml: New file.
* riscv-tdep.c (riscv_debug_breakpoints): Move from later in the
file.
(riscv_debug_infcall): Likewise.
(riscv_debug_unwinder): Likewise.
(riscv_debug_gdbarch): Likewise.
(enum riscv_register_required_status): Delete.
(struct riscv_register_feature): Add constructor, delete default
constructor, copy, and assign constructors.
(struct riscv_register_feature::register_info) <required>: Delete.
<check>: Update comment and arguments.
(struct riscv_register_feature) <name>: Change to member function.
<prefer_first_name>: Delete.
<tdesc_feature>: New member function.
<registers>: Rename to...
<m_registers>: ...this.
<m_feature_name>: New member variable.
(riscv_register_feature::register_info::check): Update arguments.
(riscv_xreg_feature): Rewrite as class, create a single static
instance of the class.
(riscv_freg_feature): Likewise.
(riscv_virtual_feature): Likewise.
(riscv_csr_feature): Likewise.
(riscv_create_csr_aliases): Has become a member function inside
riscv_csr_feature class.
(riscv_abi_embedded): New function definition.
(riscv_register_name): Adjust to use new feature objects.
(struct riscv_call_info) <riscv_call_info>: Check for rv32e abi,
and adjust available argument registers.
(riscv_features_from_gdbarch_info): Check for EF_RISCV_RVE flag.
(riscv_check_tdesc_feature): Delete.
(riscv_tdesc_unknown_reg): Adjust to use new feature objects.
(riscv_gdbarch_init): Delete target description checking code, and
instead call to the new feature objects to perform the checks.
Reorder handling of no abi information case, allows small code
simplification.
(_initialize_riscv_tdep): Remove call, this is now done in the
riscv_csr_feature constructor.
* riscv-tdep.h (riscv_abi_embedded): Declare.
RISC-V GDB was changed to make use of the DECLARE_CSR_ALIAS macro to
define register aliases for some CSRs. Actually, only one alias was
created 'dscratch' as an alias for 'dscratch0'. All of the other
DECLARE_CSR_ALIAS lines (from include/opcode/riscv-opc.h) were
filtered out.
RISC-V: Support debug and float CSR as the unprivileged ones.
Changes were made to include/opcode/riscv-opc.h so that GDB no longer
created even the dscratch alias.
This caused a test failure in gdb.arch/riscv-tdesc-regs.exp.
In looking at how to address this failure I think that the best
strategy is, for now at least, to just remove the code that tries to
create aliases with DECLARE_CSR_ALIAS.
My thoughts are that:
1. At least some of the aliases are for CSRs where the register now
has a completely different use. Being able to reference the CSR
using a completely inappropriate name just seems confusing. This
was solved by the filtering added in the first commit referenced
above. But we certainly don't want to blindly add all aliases.
2. Names presented in a target description are always honoured, so
if a user has a legacy target then they should just start sending a
target description with their legacy register names in, this problem
is then solved.
3. It's easy enough to figure out which CSRs a target has with the
info registers command, so missing an alias shouldn't be a big
issue.
4. Allowing users to use names for registers that differ from the
names the target announces doesn't feel like a critical feature. If
in the future targets want multiple names for a register then maybe
we could/should extend target descriptions to allow the target to
send aliases as well as the primary name.... but that can wait for
another day.
So in this commit I remove the use of DECLARE_CSR_ALIAS, and remove
the test that was failing.
gdb/ChangeLog:
* riscv-tdep.c (riscv_create_csr_aliases): Remove use of
DECLARE_CSR_ALIAS.
Andrew Burgess [Tue, 24 Nov 2020 18:08:25 +0000 (18:08 +0000)]
gdb/riscv: place unknown csrs into the correct register groups
Unknown riscv CSRs should not be in the 'general' group, but should be
in the system and csr register groups.
To see this in action connect to QEMU, this target advertises two
registers dscratch and mucounteren which are unknown to GDB (these are
legacy CSRs). Before this commit these registers would show up in the
output of:
(gdb) info registers
....
dscratch Could not fetch register "dscratch"; remote failure reply 'E14'
mucounteren Could not fetch register "mucounteren"; remote failure reply 'E14'
Ignore the errors, this is just a QEMU annoyance, it advertises these
CSRs, but doesn't actually let GDB read them. These registers don't
show up in the output of either:
(gdb) info registers csr
(gdb) info registers system
After this commit this situation is reveresed, which makes more sense
to me.
gdb/ChangeLog:
* riscv-tdep.c (riscv_is_unknown_csr): New function,
implementation moved from riscv_register_reggroup_p.
(riscv_register_reggroup_p): Update group handling for unknown
CSRs.
gdb/testsuite/ChangeLog:
* gdb.arch/riscv-tdesc-regs.exp (get_expected_result): New proc,
update test to use this.
Search for DWZ files in debug-file-directories as well
When Debian (and Ubuntu) builds its binaries, it (still) doesn't use
dwz's "--relative" option. This causes their debuginfo files to
carry a .gnu_debugaltlink section containing a full pathname to the
DWZ alt debug file, like this:
$ readelf -wk /usr/bin/cat
Contents of the .gnu_debugaltlink section:
Separate debug info file: /usr/lib/debug/.dwz/x86_64-linux-gnu/coreutils.debug
Build-ID (0x14 bytes):
ee 76 5d 71 97 37 ce 46 99 44 32 bb e8 a9 1a ef 99 96 88 db
This usually works OK, because most of the debuginfo files installed
via apt will be present in /usr/lib/debug anyway. However, imagine
the following scenario:
- You are using /usr/bin/cat, it crashes on you and generates a
corefile.
- You don't want/need to "apt install" the debuginfo file for
coreutils from the repositories. Instead, you already have the
debuginfo files in a separate directory (e.g., $HOME/dbgsym).
- You start GDB and "set debug-file-directory $HOME/dbgsym/usr/lib/debug".
You then get the following message:
$ gdb -ex 'set debug-file-directory ./dbgsym/usr/lib/debug' -ex 'file /bin/cat' -ex 'core-file ./cat.core'
GNU gdb (Ubuntu 10.1-0ubuntu1) 10.1
...
Reading symbols from /bin/cat...
Reading symbols from /home/sergio/gdb/dbgsym/usr/lib/debug/.build-id/bc/06d3bee37b8c7e67b31cb2689cb351102ae73b.debug...
could not find '.gnu_debugaltlink' file for /home/sergio/gdb/dbgsym/usr/lib/debug/.build-id/bc/06d3bee37b8c7e67b31cb2689cb351102ae73b.debug
This error happens because GDB is trying to locate the build-id
link (inside /home/sergio/gdb/dbgsym/usr/lib/debug/.build-id) for the
DWZ alt debug file, which doesn't exist. Arguably, this is a problem
with how dh_dwz works in Debian, and it's something I'm also planning
to tackle. But, back at the problem at hand.
Besides not being able to find the build-id link in the directory
mentioned above, GDB also tried to open the DWZ alt file using its
filename. The problem here is that, since we don't have the distro's
debuginfo installed, it can't find anything under /usr/lib/debug that
satisfies it.
It occurred to me that a good way to workaround this problem is to
actually try to locate the DWZ alt debug file inside the
debug-file-directories (that were likely provided by the user). So
this is what the proposed patch does.
The idea here is simple: get the filename extracted from the
.gnu_debugaltlink section, and manipulate it in order to replace the
initial part of the path (everything before "/.dwz/") by whatever
debug-file-directories the user might have provided.
I talked with Mark Wielaard and he agrees this is a sensible approach.
In fact, apparently this is something that eu-readelf also does.
I regtested this code, and no regressions were found.
* dwarf2/read.c (dwz_search_other_debugdirs): New function.
(dwarf2_get_dwz_file): Convert 'filename' to a
std::string. Use dwz_search_other_debugdirs to search for DWZ
files in the debug-file-directories provided by the user as well.
Tom Tromey [Wed, 2 Dec 2020 00:22:05 +0000 (17:22 -0700)]
Use new+delete for struct expression
In another series I'm working on, it is necessary to manage
"struct expression" with new and delete. Because the patch is
straightforward and could be extracted, I've done so here.
gdb/ChangeLog
2020-12-01 Tom Tromey <tom@tromey.com>
* parse.c (expr_builder::expr_builder): Initialize expout.
(expr_builder::release): Use expression::resize.
(expression::expression, expression::~expression)
(expression::resize): New methods.
(write_exp_elt): Use expression::resize.
(prefixify_expression): Update.
(increase_expout_size): Use expression::resize.
* expression.h (struct expression): Add constructor, destructor.
<resize>: New method.
(expression_up): Change type.
projects / thirdparty / binutils-gdb.git / log
