[1/3] Remove 'read' call from dwz_file::read_string
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Test passed
|
Commit Message
dwz_file::read_string calls 'read' on the section, but this isn't
needed as the sections have all been pre-read.
This patch makes this change, and refactors dwz_file a bit to make
this more obvious -- by making it clear that only the "static
constructor" can create a dwz_file.
---
gdb/dwarf2/dwz.c | 8 +++++---
gdb/dwarf2/dwz.h | 25 +++++++++++++------------
gdb/dwarf2/read.c | 2 +-
3 files changed, 19 insertions(+), 16 deletions(-)
Comments
On Sun, Mar 23, 2025 at 8:21 PM Tom Tromey <tom@tromey.com> wrote:
> dwz_file::read_string calls 'read' on the section, but this isn't
> needed as the sections have all been pre-read.
>
> This patch makes this change, and refactors dwz_file a bit to make
> this more obvious -- by making it clear that only the "static
> constructor" can create a dwz_file.
> ---
>
>
Hi Tom,
I tested this on aarch64, Fedora 40 and this change causes no regressions.
Tested-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
On 2025-03-23 15:20, Tom Tromey wrote:
> dwz_file::read_string calls 'read' on the section, but this isn't
> needed as the sections have all been pre-read.
In the long term, to remove unnecessary 'read' calls in a safe way, I
was thinking of introducing some kind of 'dwarf2_read_section_info' type
that would be a thin wrapper around a `dwarf2_section_info`, but one
that you could only obtain by calling `dwarf2_section_info::read`. That
would be a kind of "token" to make sure that the section is safe to read
from. A bit like when a function accepts an std::lock_guard object, it
is guaranteed that the lock is taken.
> @@ -261,7 +263,7 @@ dwarf2_read_dwz_file (dwarf2_per_objfile *per_objfile)
> error (_("could not find '.gnu_debugaltlink' file for %s"),
> per_bfd->filename ());
>
> - auto result = std::make_unique<dwz_file> (std::move (dwz_bfd));
> + dwz_file_up result (new dwz_file (std::move (dwz_bfd)));
Why this change?
Otherwise, LGTM.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Simon
>> - auto result = std::make_unique<dwz_file> (std::move (dwz_bfd));
>> + dwz_file_up result (new dwz_file (std::move (dwz_bfd)));
Simon> Why this change?
make_unique can't access a private constructor.
It's a bit of a wart on C++ IMO.
Tom
@@ -34,7 +34,9 @@
const char *
dwz_file::read_string (struct objfile *objfile, LONGEST str_offset)
{
- str.read (objfile);
+ /* This must be true because the sections are read in when the
+ dwz_file is created. */
+ gdb_assert (str.readin);
if (str.buffer == NULL)
error (_("DW_FORM_GNU_strp_alt used without .debug_str "
@@ -177,7 +179,7 @@ dwz_search_other_debugdirs (std::string &filename, bfd_byte *buildid,
/* See dwz.h. */
void
-dwarf2_read_dwz_file (dwarf2_per_objfile *per_objfile)
+dwz_file::read_dwz_file (dwarf2_per_objfile *per_objfile)
{
bfd_size_type buildid_len_arg;
size_t buildid_len;
@@ -261,7 +263,7 @@ dwarf2_read_dwz_file (dwarf2_per_objfile *per_objfile)
error (_("could not find '.gnu_debugaltlink' file for %s"),
per_bfd->filename ());
- auto result = std::make_unique<dwz_file> (std::move (dwz_bfd));
+ dwz_file_up result (new dwz_file (std::move (dwz_bfd)));
for (asection *sec : gdb_bfd_sections (result->dwz_bfd))
locate_dwz_sections (per_objfile->objfile, result->dwz_bfd.get (),
@@ -31,10 +31,12 @@ struct dwarf2_per_objfile;
struct dwz_file
{
- dwz_file (gdb_bfd_ref_ptr &&bfd)
- : dwz_bfd (std::move (bfd))
- {
- }
+ /* Open the separate '.dwz' debug file, if needed. This will set
+ the appropriate field in the per-BFD structure. If the DWZ file
+ exists, the relevant sections are read in as well. Throws an
+ error if the .gnu_debugaltlink section exists but the file cannot
+ be found. */
+ static void read_dwz_file (dwarf2_per_objfile *per_objfile);
const char *filename () const
{
@@ -64,16 +66,15 @@ struct dwz_file
return a pointer to the string. */
const char *read_string (struct objfile *objfile, LONGEST str_offset);
-};
-using dwz_file_up = std::unique_ptr<dwz_file>;
+private:
-/* Open the separate '.dwz' debug file, if needed. This just sets the
- appropriate field in the per-BFD structure. If the DWZ file
- exists, the relevant sections are read in as well. Throws an error
- if the .gnu_debugaltlink section exists but the file cannot be
- found. */
+ explicit dwz_file (gdb_bfd_ref_ptr &&bfd)
+ : dwz_bfd (std::move (bfd))
+ {
+ }
+};
-extern void dwarf2_read_dwz_file (dwarf2_per_objfile *per_objfile);
+using dwz_file_up = std::unique_ptr<dwz_file>;
#endif /* GDB_DWARF2_DWZ_H */
@@ -1289,7 +1289,7 @@ dwarf2_has_info (struct objfile *objfile,
BFD, to avoid races. */
try
{
- dwarf2_read_dwz_file (per_objfile);
+ dwz_file::read_dwz_file (per_objfile);
}
catch (const gdb_exception_error &err)
{