* [PATCH v3 1/7] gdb: introduce symtab_create_debug_printf
2022-04-28 3:35 [PATCH v3 0/7] Fix printing macros Simon Marchi
@ 2022-04-28 3:35 ` Simon Marchi
2022-04-28 15:49 ` Tom Tromey
2022-04-28 3:35 ` [PATCH v3 2/7] gdb: add debug prints in buildsym.c Simon Marchi
` (6 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2022-04-28 3:35 UTC (permalink / raw)
To: gdb-patches
Introduce symtab_create_debug_printf and symtab_create_debug_printf_v,
to print the debug messages enabled by "set debug symtab-create".
Change-Id: I442500903f72d4635c2dd9eaef770111f317dc04
---
gdb/elfread.c | 15 ++++-----------
gdb/minsyms.c | 16 +++++-----------
gdb/psymtab.c | 14 +++++++-------
gdb/symfile.c | 22 +++++++++-------------
gdb/symtab.h | 11 +++++++++++
5 files changed, 36 insertions(+), 42 deletions(-)
diff --git a/gdb/elfread.c b/gdb/elfread.c
index b136c605b1ae..b6dea7ece2be 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1047,12 +1047,8 @@ elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags,
asymbol **symbol_table = NULL, **dyn_symbol_table = NULL;
asymbol *synthsyms;
- if (symtab_create_debug)
- {
- gdb_printf (gdb_stdlog,
- "Reading minimal symbols of objfile %s ...\n",
- objfile_name (objfile));
- }
+ symtab_create_debug_printf ("reading minimal symbols of objfile %s",
+ objfile_name (objfile));
/* If we already have minsyms, then we can skip some work here.
However, if there were stabs or mdebug sections, we go ahead and
@@ -1064,9 +1060,7 @@ elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags,
&& ei->mdebugsect == NULL
&& ei->ctfsect == NULL)
{
- if (symtab_create_debug)
- gdb_printf (gdb_stdlog,
- "... minimal symbols previously read\n");
+ symtab_create_debug_printf ("minimal symbols were previously read");
return;
}
@@ -1170,8 +1164,7 @@ elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags,
reader.install ();
- if (symtab_create_debug)
- gdb_printf (gdb_stdlog, "Done reading minimal symbols.\n");
+ symtab_create_debug_printf ("done reading minimal symbols");
}
/* Scan and build partial symbols for a symbol file.
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index cbd0ad223928..f4cb89590ff0 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1184,11 +1184,9 @@ minimal_symbol_reader::record_full (gdb::string_view name,
if (ms_type == mst_file_text && startswith (name, "__gnu_compiled"))
return (NULL);
- if (symtab_create_debug >= 2)
- gdb_printf (gdb_stdlog,
- "Recording minsym: %-21s %18s %4d %.*s\n",
- mst_str (ms_type), hex_string (address), section,
- (int) name.size (), name.data ());
+ symtab_create_debug_printf_v ("recording minsym: %-21s %18s %4d %.*s",
+ mst_str (ms_type), hex_string (address), section,
+ (int) name.size (), name.data ());
if (m_msym_bunch_index == BUNCH_SIZE)
{
@@ -1389,12 +1387,8 @@ minimal_symbol_reader::install ()
if (m_msym_count > 0)
{
- if (symtab_create_debug)
- {
- gdb_printf (gdb_stdlog,
- "Installing %d minimal symbols of objfile %s.\n",
- m_msym_count, objfile_name (m_objfile));
- }
+ symtab_create_debug_printf ("installing %d minimal symbols of objfile %s",
+ m_msym_count, objfile_name (m_objfile));
/* Allocate enough space, into which we will gather the bunches
of new and existing minimal symbols, sort them, and then
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index 402d6085fe61..57703fe215e6 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1384,7 +1384,7 @@ partial_symtab::partial_symtab (const char *filename_,
filename = objfile_per_bfd->intern (filename_);
- if (symtab_create_debug)
+ if (symtab_create_debug >= 1)
{
/* Be a bit clever with debugging messages, and don't print objfile
every time, only when it changes. */
@@ -1395,13 +1395,13 @@ partial_symtab::partial_symtab (const char *filename_,
if (last_bfd_name.empty () || last_bfd_name != this_bfd_name)
{
last_bfd_name = this_bfd_name;
- gdb_printf (gdb_stdlog,
- "Creating one or more psymtabs for %s ...\n",
- this_bfd_name);
+
+ symtab_create_debug_printf ("creating one or more psymtabs for %s",
+ this_bfd_name);
}
- gdb_printf (gdb_stdlog,
- "Created psymtab %s for module %s.\n",
- host_address_to_string (this), filename);
+
+ symtab_create_debug_printf ("created psymtab %s for module %s",
+ host_address_to_string (this), filename);
}
}
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 6f546f5b0591..b9ecef045377 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2794,13 +2794,13 @@ allocate_symtab (struct compunit_symtab *cust, const char *filename)
if (last_objfile_name.empty () || last_objfile_name != this_objfile_name)
{
last_objfile_name = this_objfile_name;
- gdb_printf (gdb_stdlog,
- "Creating one or more symtabs for objfile %s ...\n",
- this_objfile_name);
+
+ symtab_create_debug_printf_v
+ ("creating one or more symtabs for objfile %s", this_objfile_name);
}
- gdb_printf (gdb_stdlog,
- "Created symtab %s for module %s.\n",
- host_address_to_string (symtab), filename);
+
+ symtab_create_debug_printf_v ("created symtab %s for module %s",
+ host_address_to_string (symtab), filename);
}
/* Add it to CUST's list of symtabs. */
@@ -2833,13 +2833,9 @@ allocate_compunit_symtab (struct objfile *objfile, const char *name)
cu->set_debugformat ("unknown");
- if (symtab_create_debug)
- {
- gdb_printf (gdb_stdlog,
- "Created compunit symtab %s for %s.\n",
- host_address_to_string (cu),
- cu->name);
- }
+ symtab_create_debug_printf_v ("created compunit symtab %s for %s",
+ host_address_to_string (cu),
+ cu->name);
return cu;
}
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 8c2837cfa8b0..854167dde452 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2600,6 +2600,17 @@ void fixup_section (struct general_symbol_info *ginfo,
extern unsigned int symtab_create_debug;
+/* Print a "symtab-create" debug statement. */
+
+#define symtab_create_debug_printf(fmt, ...) \
+ debug_prefixed_printf_cond (symtab_create_debug >= 1, "symtab-create", fmt, ##__VA_ARGS__)
+
+/* Print a verbose "symtab-create" debug statement, only if
+ "set debug symtab-create" is set to 2 or higher. */
+
+#define symtab_create_debug_printf_v(fmt, ...) \
+ debug_prefixed_printf_cond (symtab_create_debug >= 2, "symtab-create", fmt, ##__VA_ARGS__)
+
extern unsigned int symbol_lookup_debug;
extern bool basenames_may_differ;
--
2.35.2
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH v3 2/7] gdb: add debug prints in buildsym.c
2022-04-28 3:35 [PATCH v3 0/7] Fix printing macros Simon Marchi
2022-04-28 3:35 ` [PATCH v3 1/7] gdb: introduce symtab_create_debug_printf Simon Marchi
@ 2022-04-28 3:35 ` Simon Marchi
2022-04-28 15:50 ` Tom Tromey
2022-04-28 3:35 ` [PATCH v3 3/7] gdb/dwarf: pass compilation directory to line header Simon Marchi
` (5 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2022-04-28 3:35 UTC (permalink / raw)
To: gdb-patches
Add a few debug prints in buildsym.c that were helpful to me in writing
this series.
Change-Id: If10a818feaee3ce1b78a2a254013b62dd578002b
---
gdb/buildsym.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index c54e6586cedb..feef5038773f 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -496,6 +496,8 @@ buildsym_compunit::start_subfile (const char *name)
{
/* See if this subfile is already registered. */
+ symtab_create_debug_printf ("name = %s", name);
+
for (subfile *subfile = m_subfiles; subfile; subfile = subfile->next)
{
std::string subfile_name_holder;
@@ -516,6 +518,8 @@ buildsym_compunit::start_subfile (const char *name)
if (FILENAME_CMP (subfile_name, name) == 0)
{
+ symtab_create_debug_printf ("found existing symtab with name %s (%s)",
+ subfile->name.c_str (), subfile_name);
m_current_subfile = subfile;
return;
}
@@ -744,6 +748,9 @@ buildsym_compunit::watch_main_source_file_lossage ()
Copy its line_vector and symtab to the main subfile
and then discard it. */
+ symtab_create_debug_printf ("using subfile %s as the main subfile",
+ mainsub_alias->name.c_str ());
+
mainsub->line_vector_entries
= std::move (mainsub_alias->line_vector_entries);
mainsub->symtab = mainsub_alias->symtab;
--
2.35.2
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH v3 3/7] gdb/dwarf: pass compilation directory to line header
2022-04-28 3:35 [PATCH v3 0/7] Fix printing macros Simon Marchi
2022-04-28 3:35 ` [PATCH v3 1/7] gdb: introduce symtab_create_debug_printf Simon Marchi
2022-04-28 3:35 ` [PATCH v3 2/7] gdb: add debug prints in buildsym.c Simon Marchi
@ 2022-04-28 3:35 ` Simon Marchi
2022-04-28 15:48 ` Tom Tromey
2022-04-28 3:35 ` [PATCH v3 4/7] gdb/dwarf: pass a file_entry to line_header::file_file_name Simon Marchi
` (4 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2022-04-28 3:35 UTC (permalink / raw)
To: gdb-patches
The following patch changes line_header::file_file_name to prepend the
compilation directory to the file name, if needed. For that, the line
header needs to know about the compilation directory. Prepare for that
by adding a constructor that takes it as a parameter, and passing the
value down everywhere needed. Add a second constructor for the special
case of building a line_header for doing a hash table lookup, since that
case doesn't require a compilation directory value.
Change-Id: Iba3ba0293e4e2d13a64b257cf9a3094684d54330
---
| 5 ++--
| 26 +++++++++++++++---
gdb/dwarf2/read.c | 27 ++++++++++---------
.../gdb.dwarf2/dw2-compdir-oldgcc.exp | 2 +-
4 files changed, 42 insertions(+), 18 deletions(-)
--git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 63230847568c..13379851b9b6 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -264,7 +264,8 @@ line_header_up
dwarf_decode_line_header (sect_offset sect_off, bool is_dwz,
dwarf2_per_objfile *per_objfile,
struct dwarf2_section_info *section,
- const struct comp_unit_head *cu_header)
+ const struct comp_unit_head *cu_header,
+ const char *comp_dir)
{
const gdb_byte *line_ptr;
unsigned int bytes_read, offset_size;
@@ -281,7 +282,7 @@ dwarf_decode_line_header (sect_offset sect_off, bool is_dwz,
return 0;
}
- line_header_up lh (new line_header ());
+ line_header_up lh (new line_header (comp_dir));
lh->sect_off = sect_off;
lh->offset_in_dwz = is_dwz;
--git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h
index 59b317d0d727..53db3a1d5aa8 100644
--- a/gdb/dwarf2/line-header.h
+++ b/gdb/dwarf2/line-header.h
@@ -68,8 +68,18 @@ struct file_entry
which contains the following information. */
struct line_header
{
- line_header ()
- : offset_in_dwz {}
+ /* COMP_DIR is the value of the DW_AT_comp_dir attribute of the compilation
+ unit in the context of which we are reading this line header, or nullptr
+ if not applicable. */
+ line_header (const char *comp_dir)
+ : m_comp_dir (comp_dir != nullptr ? comp_dir : "")
+ {}
+
+ /* This constructor should only be used to create line_header intances to do
+ hash table lookups. */
+ line_header (sect_offset sect_off, bool offset_in_dwz)
+ : sect_off (sect_off),
+ offset_in_dwz (offset_in_dwz)
{}
/* Add an entry to the include directory table. */
@@ -161,6 +171,11 @@ struct line_header
number FILE in this object's file name table. */
std::string file_file_name (int file) const;
+ /* Return the compilation directory of the compilation unit in the context of
+ which this line header is read. Return nullptr if non applicable. */
+ const char *comp_dir () const
+ { return !m_comp_dir.empty () ? m_comp_dir.c_str () : nullptr; }
+
private:
/* The include_directories table. Note these are observing
pointers. The memory is owned by debug_line_buffer. */
@@ -171,6 +186,10 @@ struct line_header
before, and is 0 in DWARF 5 and later). So the client should use
file_name_at method for access. */
std::vector<file_entry> m_file_names;
+
+ /* Compilation directory of the compilation unit in the context of which this
+ line header is read. */
+ std::string m_comp_dir;
};
typedef std::unique_ptr<line_header> line_header_up;
@@ -191,6 +210,7 @@ file_entry::include_dir (const line_header *lh) const
extern line_header_up dwarf_decode_line_header
(sect_offset sect_off, bool is_dwz, dwarf2_per_objfile *per_objfile,
- struct dwarf2_section_info *section, const struct comp_unit_head *cu_header);
+ struct dwarf2_section_info *section, const struct comp_unit_head *cu_header,
+ const char *comp_dir);
#endif /* DWARF2_LINE_HEADER_H */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d146d5250669..56f58a004af6 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -943,7 +943,8 @@ static struct die_info *die_specification (struct die_info *die,
struct dwarf2_cu **);
static line_header_up dwarf_decode_line_header (sect_offset sect_off,
- struct dwarf2_cu *cu);
+ struct dwarf2_cu *cu,
+ const char *comp_dir);
static void dwarf_decode_lines (struct line_header *,
struct dwarf2_cu *,
@@ -2738,6 +2739,8 @@ dw2_get_file_names_reader (const struct die_reader_specs *reader,
line_header_up lh;
sect_offset line_offset {};
+ file_and_directory &fnd = find_file_and_directory (comp_unit_die, cu);
+
attr = dwarf2_attr (comp_unit_die, DW_AT_stmt_list, cu);
if (attr != nullptr && attr->form_is_unsigned ())
{
@@ -2757,11 +2760,9 @@ dw2_get_file_names_reader (const struct die_reader_specs *reader,
return;
}
- lh = dwarf_decode_line_header (line_offset, cu);
+ lh = dwarf_decode_line_header (line_offset, cu, fnd.get_comp_dir ());
}
- file_and_directory &fnd = find_file_and_directory (comp_unit_die, cu);
-
int offset = 0;
if (!fnd.is_unknown ())
++offset;
@@ -9416,11 +9417,11 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu)
static void
handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
+ const file_and_directory &fnd,
CORE_ADDR lowpc) /* ARI: editCase function */
{
dwarf2_per_objfile *per_objfile = cu->per_objfile;
struct attribute *attr;
- struct line_header line_header_local;
hashval_t line_header_local_hash;
void **slot;
int decode_mapping;
@@ -9449,8 +9450,7 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
xcalloc, xfree));
}
- line_header_local.sect_off = line_offset;
- line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
+ line_header line_header_local (line_offset, cu->per_cu->is_dwz);
line_header_local_hash = line_header_hash (&line_header_local);
if (per_objfile->line_header_hash != NULL)
{
@@ -9471,7 +9471,8 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
/* dwarf_decode_line_header does not yet provide sufficient information.
We always have to call also dwarf_decode_lines for it. */
- line_header_up lh = dwarf_decode_line_header (line_offset, cu);
+ line_header_up lh = dwarf_decode_line_header (line_offset, cu,
+ fnd.get_comp_dir ());
if (lh == NULL)
return;
@@ -9549,7 +9550,7 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
lnp_state_machine::check_line_address) will fail to properly
exclude an entry that was removed via --gc-sections. */
if (lowpc != highpc)
- handle_DW_AT_stmt_list (die, cu, lowpc);
+ handle_DW_AT_stmt_list (die, cu, fnd, lowpc);
/* Process all dies in compilation unit. */
if (die->child != NULL)
@@ -9623,7 +9624,7 @@ dwarf2_cu::setup_type_unit_groups (struct die_info *die)
if (attr != NULL && attr->form_is_unsigned ())
{
sect_offset line_offset = (sect_offset) attr->as_unsigned ();
- lh = dwarf_decode_line_header (line_offset, this);
+ lh = dwarf_decode_line_header (line_offset, this, nullptr);
}
if (lh == NULL)
{
@@ -19664,7 +19665,8 @@ get_debug_line_section (struct dwarf2_cu *cu)
and must not be freed. */
static line_header_up
-dwarf_decode_line_header (sect_offset sect_off, struct dwarf2_cu *cu)
+dwarf_decode_line_header (sect_offset sect_off, struct dwarf2_cu *cu,
+ const char *comp_dir)
{
struct dwarf2_section_info *section;
dwarf2_per_objfile *per_objfile = cu->per_objfile;
@@ -19681,7 +19683,8 @@ dwarf_decode_line_header (sect_offset sect_off, struct dwarf2_cu *cu)
}
return dwarf_decode_line_header (sect_off, cu->per_cu->is_dwz,
- per_objfile, section, &cu->header);
+ per_objfile, section, &cu->header,
+ comp_dir);
}
/* Subroutine of dwarf_decode_lines to simplify it.
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-compdir-oldgcc.exp b/gdb/testsuite/gdb.dwarf2/dw2-compdir-oldgcc.exp
index 0766b9688aeb..0601d26f4e86 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-compdir-oldgcc.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-compdir-oldgcc.exp
@@ -30,7 +30,7 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
# Here should be GDB-computed "Compilation directory is".
gdb_test "list gcc42" ".*"
-gdb_test "info source" "\r\nCurrent source file is dw2-compdir-oldgcc42.S\r\nCompilation directory is /dir/d\r\n.*" \
+gdb_test "info source" "\r\nCurrent source file is /dir/d/dw2-compdir-oldgcc42.S\r\nCompilation directory is /dir/d\r\n.*" \
"info source gcc42"
# Here should not be GDB-computed "Compilation directory is".
--
2.35.2
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 3/7] gdb/dwarf: pass compilation directory to line header
2022-04-28 3:35 ` [PATCH v3 3/7] gdb/dwarf: pass compilation directory to line header Simon Marchi
@ 2022-04-28 15:48 ` Tom Tromey
2022-04-28 15:59 ` Simon Marchi
0 siblings, 1 reply; 21+ messages in thread
From: Tom Tromey @ 2022-04-28 15:48 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
Simon> + /* COMP_DIR is the value of the DW_AT_comp_dir attribute of the compilation
Simon> + unit in the context of which we are reading this line header, or nullptr
Simon> + if not applicable. */
Simon> + line_header (const char *comp_dir)
Simon> + : m_comp_dir (comp_dir != nullptr ? comp_dir : "")
Simon> + {}
I think a single-argument constructor should be 'explicit'.
Simon> + /* Compilation directory of the compilation unit in the context of which this
Simon> + line header is read. */
Simon> + std::string m_comp_dir;
Is there a need to copy the value here?
I didn't check in this case, but often in the DWARF reader, a string
will come directly from some mapped section, and in that case there
aren't lifetime issues -- and so it's fine to just store the pointer.
Tom
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 3/7] gdb/dwarf: pass compilation directory to line header
2022-04-28 15:48 ` Tom Tromey
@ 2022-04-28 15:59 ` Simon Marchi
0 siblings, 0 replies; 21+ messages in thread
From: Simon Marchi @ 2022-04-28 15:59 UTC (permalink / raw)
To: Tom Tromey, Simon Marchi via Gdb-patches
On 2022-04-28 11:48, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Simon> + /* COMP_DIR is the value of the DW_AT_comp_dir attribute of the compilation
> Simon> + unit in the context of which we are reading this line header, or nullptr
> Simon> + if not applicable. */
> Simon> + line_header (const char *comp_dir)
> Simon> + : m_comp_dir (comp_dir != nullptr ? comp_dir : "")
> Simon> + {}
>
> I think a single-argument constructor should be 'explicit'.
Fixed. I should also initialize the bitfield field offset_in_dwz, fixed
that too.
>
> Simon> + /* Compilation directory of the compilation unit in the context of which this
> Simon> + line header is read. */
> Simon> + std::string m_comp_dir;
>
> Is there a need to copy the value here?
> I didn't check in this case, but often in the DWARF reader, a string
> will come directly from some mapped section, and in that case there
> aren't lifetime issues -- and so it's fine to just store the pointer.
I thought there might be lifetime issues.
read_file_scope owns a file_and_directory object, which contains a
possibly computed comp dir. That object's lifetime is read_file_scope,
but the line_header created there (by calling handle_DW_AT_stmt_list)
lives on, it is stored in dwarf2_cu::line_header.
However, I see that we call fnd.intern_comp_dir prior to that, so the
returned string should actually be one with objfile lifetime, it should
be fine. I'll change it to a const char*, we'll see if it causes any
test to fail (I build with ASan, so it should point out any
use-after-free).
Thanks,
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 4/7] gdb/dwarf: pass a file_entry to line_header::file_file_name
2022-04-28 3:35 [PATCH v3 0/7] Fix printing macros Simon Marchi
` (2 preceding siblings ...)
2022-04-28 3:35 ` [PATCH v3 3/7] gdb/dwarf: pass compilation directory to line header Simon Marchi
@ 2022-04-28 3:35 ` Simon Marchi
2022-05-03 20:12 ` Bruno Larsen
2022-04-28 3:35 ` [PATCH v3 5/7] gdb: add "id" fields to identify symtabs and subfiles Simon Marchi
` (3 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2022-04-28 3:35 UTC (permalink / raw)
To: gdb-patches
In the following patch, there will be some callers of file_file_name
that will already have access to the file_entry object for which they
want the file name. It would be inefficient to have them pass an index,
only for line_header::file_file_name to re-lookup the same file_entry
object. Change line_header::file_file_name to accept a file_entry
object reference, instead of an index to look up.
I think this change makes sense in any case. Callers that have an index
can first obtain a file_entry using line_header::file_name_at or
line_header::file_names.
When passing a file_entry object, we can assume that the file_entry's
index is valid, unlike when passing an index. So, push the special case
about an invalid index to the sole current caller of file_file_name,
macro_start_file. I think that error belongs there anyway, since it
specifically talks about "bad file number in macro information".
This requires recording the file index in the file_entry structure, so
add that.
Change-Id: Ic6e44c407539d92b7863d7ba82405ade17f384ad
---
| 47 ++++++++++++----------------------------
| 10 ++++++---
gdb/dwarf2/macro.c | 16 +++++++++++++-
3 files changed, 36 insertions(+), 37 deletions(-)
--git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 13379851b9b6..33af77d3ecf3 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -48,47 +48,28 @@ line_header::add_file_name (const char *name,
unsigned int mod_time,
unsigned int length)
{
+ file_name_index index
+ = version >= 5 ? file_names_size (): file_names_size () + 1;
+
if (dwarf_line_debug >= 2)
- {
- size_t new_size;
- if (version >= 5)
- new_size = file_names_size ();
- else
- new_size = file_names_size () + 1;
- gdb_printf (gdb_stdlog, "Adding file %zu: %s\n",
- new_size, name);
- }
- m_file_names.emplace_back (name, d_index, mod_time, length);
+ gdb_printf (gdb_stdlog, "Adding file %d: %s\n", index, name);
+
+ m_file_names.emplace_back (name, index, d_index, mod_time, length);
}
std::string
-line_header::file_file_name (int file) const
+line_header::file_file_name (const file_entry &fe) const
{
- /* Is the file number a valid index into the line header's file name
- table? Remember that file numbers start with one, not zero. */
- if (is_valid_file_index (file))
- {
- const file_entry *fe = file_name_at (file);
+ gdb_assert (is_valid_file_index (fe.index));
- if (!IS_ABSOLUTE_PATH (fe->name))
- {
- const char *dir = fe->include_dir (this);
- if (dir != NULL)
- return path_join (dir, fe->name);
- }
+ if (IS_ABSOLUTE_PATH (fe.name))
+ return fe.name;
- return fe->name;
- }
- else
- {
- /* The compiler produced a bogus file number. We can at least
- record the macro definitions made in the file, even if we
- won't be able to find the file by name. */
- complaint (_("bad file number in macro information (%d)"),
- file);
+ const char *dir = fe.include_dir (this);
+ if (dir != nullptr)
+ return path_join (dir, fe.name);
- return string_printf ("<bad macro file number %d>", file);
- }
+ return fe.name;
}
static void
--git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h
index 53db3a1d5aa8..0fe539b0c427 100644
--- a/gdb/dwarf2/line-header.h
+++ b/gdb/dwarf2/line-header.h
@@ -36,9 +36,10 @@ struct file_entry
{
file_entry () = default;
- file_entry (const char *name_, dir_index d_index_,
+ file_entry (const char *name_, file_name_index index_, dir_index d_index_,
unsigned int mod_time_, unsigned int length_)
: name (name_),
+ index (index_),
d_index (d_index_),
mod_time (mod_time_),
length (length_)
@@ -52,6 +53,9 @@ struct file_entry
owned by debug_line_buffer. */
const char *name {};
+ /* The index of this file in the file table. */
+ file_name_index index {};
+
/* The directory index (1-based). */
dir_index d_index {};
@@ -168,8 +172,8 @@ struct line_header
const gdb_byte *statement_program_start {}, *statement_program_end {};
/* Return file name relative to the compilation directory of file
- number FILE in this object's file name table. */
- std::string file_file_name (int file) const;
+ FE in this object's file name table. */
+ std::string file_file_name (const file_entry &fe) const;
/* Return the compilation directory of the compilation unit in the context of
which this line header is read. Return nullptr if non applicable. */
diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
index 99c3653a2c3a..38c0fdfec738 100644
--- a/gdb/dwarf2/macro.c
+++ b/gdb/dwarf2/macro.c
@@ -52,7 +52,21 @@ macro_start_file (buildsym_compunit *builder,
const struct line_header *lh)
{
/* File name relative to the compilation directory of this source file. */
- std::string file_name = lh->file_file_name (file);
+ const file_entry *fe = lh->file_name_at (file);
+ std::string file_name;
+
+ if (fe != nullptr)
+ file_name = lh->file_file_name (*fe);
+ else
+ {
+ /* The compiler produced a bogus file number. We can at least
+ record the macro definitions made in the file, even if we
+ won't be able to find the file by name. */
+ complaint (_("bad file number in macro information (%d)"),
+ file);
+
+ file_name = string_printf ("<bad macro file number %d>", file);
+ }
if (! current_file)
{
--
2.35.2
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 4/7] gdb/dwarf: pass a file_entry to line_header::file_file_name
2022-04-28 3:35 ` [PATCH v3 4/7] gdb/dwarf: pass a file_entry to line_header::file_file_name Simon Marchi
@ 2022-05-03 20:12 ` Bruno Larsen
2022-07-28 16:26 ` Simon Marchi
0 siblings, 1 reply; 21+ messages in thread
From: Bruno Larsen @ 2022-05-03 20:12 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
Hi Simon!
On 4/28/22 00:35, Simon Marchi via Gdb-patches wrote:
> In the following patch, there will be some callers of file_file_name
> that will already have access to the file_entry object for which they
> want the file name. It would be inefficient to have them pass an index,
> only for line_header::file_file_name to re-lookup the same file_entry
> object. Change line_header::file_file_name to accept a file_entry
> object reference, instead of an index to look up.
>
> I think this change makes sense in any case. Callers that have an index
> can first obtain a file_entry using line_header::file_name_at or
> line_header::file_names.
>
> When passing a file_entry object, we can assume that the file_entry's
> index is valid, unlike when passing an index. So, push the special case
> about an invalid index to the sole current caller of file_file_name,
> macro_start_file. I think that error belongs there anyway, since it
> specifically talks about "bad file number in macro information".
>
> This requires recording the file index in the file_entry structure, so
> add that.
>
Thanks for looking at this! We definitely need some movement in the file side of things. The general direction looks good, but I have some questions
Related to this patch, do you feel like it could be worth trying to centralize files into a single class? Joining file_entry, file_and_directory, and the information contained in symtabs into a single class (maybe file_info?) that will always handle names and paths the same way?
> Change-Id: Ic6e44c407539d92b7863d7ba82405ade17f384ad
> ---
> gdb/dwarf2/line-header.c | 47 ++++++++++++----------------------------
> gdb/dwarf2/line-header.h | 10 ++++++---
> gdb/dwarf2/macro.c | 16 +++++++++++++-
> 3 files changed, 36 insertions(+), 37 deletions(-)
>
> diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
> index 13379851b9b6..33af77d3ecf3 100644
> --- a/gdb/dwarf2/line-header.c
> +++ b/gdb/dwarf2/line-header.c
> @@ -48,47 +48,28 @@ line_header::add_file_name (const char *name,
> unsigned int mod_time,
> unsigned int length)
> {
> + file_name_index index
> + = version >= 5 ? file_names_size (): file_names_size () + 1;
> +
> if (dwarf_line_debug >= 2)
> - {
> - size_t new_size;
> - if (version >= 5)
> - new_size = file_names_size ();
> - else
> - new_size = file_names_size () + 1;
> - gdb_printf (gdb_stdlog, "Adding file %zu: %s\n",
> - new_size, name);
> - }
> - m_file_names.emplace_back (name, d_index, mod_time, length);
> + gdb_printf (gdb_stdlog, "Adding file %d: %s\n", index, name);
> +
> + m_file_names.emplace_back (name, index, d_index, mod_time, length);
> }
>
> std::string
> -line_header::file_file_name (int file) const
> +line_header::file_file_name (const file_entry &fe) const
> {
> - /* Is the file number a valid index into the line header's file name
> - table? Remember that file numbers start with one, not zero. */
> - if (is_valid_file_index (file))
> - {
> - const file_entry *fe = file_name_at (file);
> + gdb_assert (is_valid_file_index (fe.index));
>
> - if (!IS_ABSOLUTE_PATH (fe->name))
> - {
> - const char *dir = fe->include_dir (this);
> - if (dir != NULL)
> - return path_join (dir, fe->name);
> - }
> + if (IS_ABSOLUTE_PATH (fe.name))
> + return fe.name;
>
> - return fe->name;
> - }
> - else
> - {
> - /* The compiler produced a bogus file number. We can at least
> - record the macro definitions made in the file, even if we
> - won't be able to find the file by name. */
> - complaint (_("bad file number in macro information (%d)"),
> - file);
> + const char *dir = fe.include_dir (this);
> + if (dir != nullptr)
> + return path_join (dir, fe.name);
>
> - return string_printf ("<bad macro file number %d>", file);
> - }
> + return fe.name;
I'm a bit confused about this last return. Why have a shortcut for absolute paths, then attempting to get the include dir, then returning only the name if that fails, instead of simplifying to something like:
if (!IS_ABSOLUTE_PATH (fe.name)
{
const char *dir = fe.include_dir (this);
if (dir != nullptr)
return path_join (dir, fe.name);
}
return fe.name;
> }
>
> static void
> diff --git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h
> index 53db3a1d5aa8..0fe539b0c427 100644
> --- a/gdb/dwarf2/line-header.h
> +++ b/gdb/dwarf2/line-header.h
> @@ -36,9 +36,10 @@ struct file_entry
> {
> file_entry () = default;
>
> - file_entry (const char *name_, dir_index d_index_,
> + file_entry (const char *name_, file_name_index index_, dir_index d_index_,
> unsigned int mod_time_, unsigned int length_)
> : name (name_),
> + index (index_),
> d_index (d_index_),
> mod_time (mod_time_),
> length (length_)
> @@ -52,6 +53,9 @@ struct file_entry
> owned by debug_line_buffer. */
> const char *name {};
>
> + /* The index of this file in the file table. */
> + file_name_index index {};
> +
> /* The directory index (1-based). */
> dir_index d_index {};
>
> @@ -168,8 +172,8 @@ struct line_header
> const gdb_byte *statement_program_start {}, *statement_program_end {};
>
> /* Return file name relative to the compilation directory of file
> - number FILE in this object's file name table. */
> - std::string file_file_name (int file) const;
> + FE in this object's file name table. */
> + std::string file_file_name (const file_entry &fe) const;
>
> /* Return the compilation directory of the compilation unit in the context of
> which this line header is read. Return nullptr if non applicable. */
> diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
> index 99c3653a2c3a..38c0fdfec738 100644
> --- a/gdb/dwarf2/macro.c
> +++ b/gdb/dwarf2/macro.c
> @@ -52,7 +52,21 @@ macro_start_file (buildsym_compunit *builder,
> const struct line_header *lh)
> {
> /* File name relative to the compilation directory of this source file. */
> - std::string file_name = lh->file_file_name (file);
> + const file_entry *fe = lh->file_name_at (file);
> + std::string file_name;
> +
> + if (fe != nullptr)
> + file_name = lh->file_file_name (*fe);
> + else
> + {
> + /* The compiler produced a bogus file number. We can at least
> + record the macro definitions made in the file, even if we
> + won't be able to find the file by name. */
> + complaint (_("bad file number in macro information (%d)"),
> + file);
> +
> + file_name = string_printf ("<bad macro file number %d>", file);
> + }
>
> if (! current_file)
> {
Cheers!
Bruno Larsen
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 4/7] gdb/dwarf: pass a file_entry to line_header::file_file_name
2022-05-03 20:12 ` Bruno Larsen
@ 2022-07-28 16:26 ` Simon Marchi
0 siblings, 0 replies; 21+ messages in thread
From: Simon Marchi @ 2022-07-28 16:26 UTC (permalink / raw)
To: Bruno Larsen, gdb-patches
On 5/3/22 16:12, Bruno Larsen wrote:
> Hi Simon!
> On 4/28/22 00:35, Simon Marchi via Gdb-patches wrote:
>> In the following patch, there will be some callers of file_file_name
>> that will already have access to the file_entry object for which they
>> want the file name. It would be inefficient to have them pass an index,
>> only for line_header::file_file_name to re-lookup the same file_entry
>> object. Change line_header::file_file_name to accept a file_entry
>> object reference, instead of an index to look up.
>>
>> I think this change makes sense in any case. Callers that have an index
>> can first obtain a file_entry using line_header::file_name_at or
>> line_header::file_names.
>>
>> When passing a file_entry object, we can assume that the file_entry's
>> index is valid, unlike when passing an index. So, push the special case
>> about an invalid index to the sole current caller of file_file_name,
>> macro_start_file. I think that error belongs there anyway, since it
>> specifically talks about "bad file number in macro information".
>>
>> This requires recording the file index in the file_entry structure, so
>> add that.
>>
>
> Thanks for looking at this! We definitely need some movement in the
> file side of things. The general direction looks good, but I have some
> questions
Thanks for the review. Sorry for the delay, I'm just coming back to
this patch series.
> Related to this patch, do you feel like it could be worth trying to
> centralize files into a single class? Joining file_entry,
> file_and_directory, and the information contained in symtabs into a
> single class (maybe file_info?) that will always handle names and
> paths the same way?
I don't know, I don't immediately see how that could be done, and it
depends what you mean by "handle names and paths".
>> diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
>> index 13379851b9b6..33af77d3ecf3 100644
>> --- a/gdb/dwarf2/line-header.c
>> +++ b/gdb/dwarf2/line-header.c
>> @@ -48,47 +48,28 @@ line_header::add_file_name (const char *name,
>> unsigned int mod_time,
>> unsigned int length)
>> {
>> + file_name_index index
>> + = version >= 5 ? file_names_size (): file_names_size () + 1;
>> +
>> if (dwarf_line_debug >= 2)
>> - {
>> - size_t new_size;
>> - if (version >= 5)
>> - new_size = file_names_size ();
>> - else
>> - new_size = file_names_size () + 1;
>> - gdb_printf (gdb_stdlog, "Adding file %zu: %s\n",
>> - new_size, name);
>> - }
>> - m_file_names.emplace_back (name, d_index, mod_time, length);
>> + gdb_printf (gdb_stdlog, "Adding file %d: %s\n", index, name);
>> +
>> + m_file_names.emplace_back (name, index, d_index, mod_time, length);
>> }
>> std::string
>> -line_header::file_file_name (int file) const
>> +line_header::file_file_name (const file_entry &fe) const
>> {
>> - /* Is the file number a valid index into the line header's file name
>> - table? Remember that file numbers start with one, not zero. */
>> - if (is_valid_file_index (file))
>> - {
>> - const file_entry *fe = file_name_at (file);
>> + gdb_assert (is_valid_file_index (fe.index));
>> - if (!IS_ABSOLUTE_PATH (fe->name))
>> - {
>> - const char *dir = fe->include_dir (this);
>> - if (dir != NULL)
>> - return path_join (dir, fe->name);
>> - }
>> + if (IS_ABSOLUTE_PATH (fe.name))
>> + return fe.name;
>> - return fe->name;
>> - }
>> - else
>> - {
>> - /* The compiler produced a bogus file number. We can at least
>> - record the macro definitions made in the file, even if we
>> - won't be able to find the file by name. */
>> - complaint (_("bad file number in macro information (%d)"),
>> - file);
>> + const char *dir = fe.include_dir (this);
>> + if (dir != nullptr)
>> + return path_join (dir, fe.name);
>> - return string_printf ("<bad macro file number %d>", file);
>> - }
>> + return fe.name;
>
> I'm a bit confused about this last return. Why have a shortcut for
> absolute paths, then attempting to get the include dir, then returning
> only the name if that fails, instead of simplifying to something like:
>
> if (!IS_ABSOLUTE_PATH (fe.name)
> {
> const char *dir = fe.include_dir (this);
> if (dir != nullptr)
> return path_join (dir, fe.name);
> }
> return fe.name;
Hmm, matter of style I guess. I'm a big fan of early returns, I see it
as filtering out cases, and I find it lightens the cognitive load a bit.
When I see:
if (IS_ABSOLUTE_PATH (fe.name))
return fe.name;
I understand immediately that if fe.name is absolute, we return it as
is, and the rest of the code only deals with relative paths. So my
brain can stop thinking about absolute paths for the rest of the
function.
Perhaps it would make more sense to swap the last two cases though, like
this:
if (IS_ABSOLUTE_PATH (fe.name))
return fe.name;
const char *dir = fe.include_dir (this);
if (dir == nullptr)
return fe.name;
return path_join (dir, fe.name);
Since the "dir == nullptr" condition filters out the unusual / error
case of not having a directory. I'll do this change.
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 5/7] gdb: add "id" fields to identify symtabs and subfiles
2022-04-28 3:35 [PATCH v3 0/7] Fix printing macros Simon Marchi
` (3 preceding siblings ...)
2022-04-28 3:35 ` [PATCH v3 4/7] gdb/dwarf: pass a file_entry to line_header::file_file_name Simon Marchi
@ 2022-04-28 3:35 ` Simon Marchi
2022-04-28 23:53 ` Lancelot SIX
2022-04-28 3:35 ` [PATCH v3 6/7] gdb: remove code to prepend comp dir in buildsym_compunit::start_subfile Simon Marchi
` (2 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2022-04-28 3:35 UTC (permalink / raw)
To: gdb-patches
Printing macros defined in the main source file doesn't work reliably
using various toolchains, especially when DWARF 5 is used. For example,
using the binaries produced by either of these commands:
$ gcc --version
gcc (GCC) 11.2.0
$ ld --version
GNU ld (GNU Binutils) 2.38
$ gcc test.c -g3 -gdwarf-5
$ clang --version
clang version 13.0.1
$ clang test.c -gdwarf-5 -fdebug-macro
I get:
$ ./gdb -nx -q --data-directory=data-directory a.out
(gdb) start
Temporary breakpoint 1 at 0x111d: file test.c, line 6.
Starting program: /home/simark/build/binutils-gdb-one-target/gdb/a.out
Temporary breakpoint 1, main () at test.c:6
6 return ZERO;
(gdb) p ZERO
No symbol "ZERO" in current context.
When starting to investigate this (taking the gcc-compiled binary as an
example), we see that GDB fails to look up the appropriate macro scope
when evaluating the expression. While stopped in
macro_lookup_inclusion:
(top-gdb) p name
$1 = 0x62100011a980 "test.c"
(top-gdb) p source.filename
$2 = 0x62100011a9a0 "/home/simark/build/binutils-gdb-one-target/gdb/test.c"
`source` is the macro_source_file that we would expect GDB to find.
`name` comes from the symtab::filename field of the symtab we are
stopped in. GDB doesn't find the appropriate macro_source_file because
the name of the macro_source_file doesn't match exactly the name of the
symtab.
The name of the main symtab comes from the compilation unit's
DW_AT_name, passed to the buildsym_compunit's constructor:
https://gitlab.com/gnutools/binutils-gdb/-/blob/4815d6125ec580cc02a1094d61b8c9d1cc83c0a1/gdb/dwarf2/read.c#L10627-10630
The contents of DW_AT_name, in this case, is "test.c". It is typically
(what I witnessed all compilers do) the same string that was passed to
the compiler on the command-line.
The name of the macro_source_file comes from the line number program
header's file table, from the call to the line_header::file_file_name
method:
https://gitlab.com/gnutools/binutils-gdb/-/blob/4815d6125ec580cc02a1094d61b8c9d1cc83c0a1/gdb/dwarf2/macro.c#L54-65
line_header::file_file_name prepends the directory path that the file
entry refers to, in the file table (if the file name is not already
absolute). In this case, the file name is "test.c", appended to the
directory "/home/simark/build/binutils-gdb-one-target/gdb".
Because the symtab's name is not created the same way as the
macro_source_file's name is created, we get this mismatch. GDB fails to
find the appropriate macro scope for the symtab, and we can't print
macros when stopped in that symtab.
To make this work, we must ensure that paths produced in these two ways
end up identical. This can be tricky because of the different ways a
path can be passed to the compiler.
Another this to consider is that while the main symtab's name (or
subfile, before it becomes a symtab) is created using DW_AT_name, the
main symtab is also referred to using its entry in the line table
header's file table, when processing the line table. We must therefore
ensure that the same name is produced in both cases, so that a call to
"start_subfile" for the main subfile will correctly find the
already-created subfile, created by buildsym_compunit's constructor. If
we fail to do that, things still often work, because of a fallback: the
watch_main_source_file_lossage method. This method determines that if
the main subfile has no symbols but there exists another subfile with
the same basename (e.g. "test.c") that does have symbols, it's probably
because there was some filename mismatch. So it replaces the main
subfile with that other subfile. I think that heuristic is useful as a
last effort to work around any bug or bad debug info, but I don't think
we should design things such as to rely on it. It's a heuristic, it can
get things wrong. So in my search for a fix, it is important that given
some good debug info, we don't end up relying on that for things to
work.
A first attempt at fixing this was to try to prepend the compilation
directory here or not prepend it there. In practice, because of all the
possible combinations of debug info the compilers produce, it was not
possible to get something that would produce reliable, consistent paths.
Another attempt at fixing this was to make both macro_source_file
objects and symtab objects use the most complete form of path possible.
That means to prepend directories at least until we get an absolute
path. In theory, we should end up with the same path in all cases.
This generally worked, but because it changed the symtab names, it
resulted in user-visible changes (for example, paths to source files in
Breakpoint hit messages becoming always absolute). I didn't find this
very good, first because there is a "set filename-display" setting that
lets the user control how they want the paths to be displayed, and that
would suddenly make this setting completely ineffective (although even
today, it is a bit dependent on the debug info). Second, it would
require a good amount of testsuite tweaks to make tests accept these
suddenly absolute paths.
This new patch is a slight variation of that: it adds a new field called
"filename_for_id" in struct symtab and struct subfile, next to the
existing filename field. The goal is to separate the internal ids used
for finding objects from the names used for presentation. This field is
used for identifying subfiles, symtabs and macro_source_files
internally. For DWARF symtabs, this new field is meant to contain the
"most complete possible" path, as discussed above. So for a given file,
it must always be in the same form, everywhere. The existing
symtab::filename field remains the one used for printing to the user, so
there shouldn't be any change in how paths are printed.
Changes in the core symtab files are:
- Add "name_for_id" and "filename_for_id" fields to "struct subfile"
and "struct symta"b, next to existing "name" and "filename" fields.
- Make buildsym_compunit::buildsym_compunit and
buildsym_compunit::start_subfile accept a "name_for_id" parameter
next to the existing "name" ones.
- Make buildsym_compunit::start_subfile used "name_for_id" for looking
up existing subfiles. This is the key thing for making calls
to start_subfile for the main source file look up the existing
subfile successfully, and avoid relying on
watch_main_source_file_lossage.
- Make sal_macro_scope pass "filename_for_id", rather than "filename",
to macro_lookup_inclusion. This is the key thing to making the
lookup work and macro printing work.
Changes in the DWARF files are:
- Make line_header::file_file_name return the "most complete possible"
name. The only pre-existing user of this method is the macro code,
to give the macro_source_file objects their name. And we now want
them to have this "most complete possible" name, which will match the
corresponding symtab's "filename_for_id".
- Make dwarf2_cu::start_compunit_symtab pass the "most complete
possible" name for the main symtab's "filename_for_id". In this
context, where the info comes from the compilation unit's DW_AT_name
/ DW_AT_comp_dir, it means prepending DW_AT_comp_dir to DW_AT_name if
DW_AT_name is not already absolute.
- Change dwarf2_start_subfile to build a name_for_id for the subfile
being started. The simplest way is to re-use
line_header::file_file_name, since the callers always have a
file_entry handy. This ensures that it will get the exact same path
representation as the macro code does, for the same file (since it
also uses line_header::file_file_name).
- Update calls to allocate_symtab to pass the "name_for_id" from the
subfile.
The rest of the changes are to update the other symtab users (jit,
ctfread, mdebugread, xcoffread). For those, the same value is passed
for the "id" as the for the filename, so they should keep the same
behavior they have today.
Tests exercising all this are added by the following patch.
Of all the cases I tried, the only one I found that ends up relying on
watch_main_source_file_lossage is the following one:
$ clang --version
clang version 13.0.1
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
$ clang ./test.c -g3 -O0 -gdwarf-4
$ ./gdb -nx --data-directory=data-directory -q -readnow -iex "set debug symtab-create 1" a.out
...
[symtab-create] start_subfile: name = test.c, name_for_id = /home/simark/build/binutils-gdb-one-target/gdb/test.c
[symtab-create] start_subfile: name = ./test.c, name_for_id = /home/simark/build/binutils-gdb-one-target/gdb/./test.c
[symtab-create] start_subfile: name = ./test.c, name_for_id = /home/simark/build/binutils-gdb-one-target/gdb/./test.c
[symtab-create] start_subfile: found existing symtab with name_for_id /home/simark/build/binutils-gdb-one-target/gdb/./test.c (/home/simark/build/binutils-gdb-one-target/gdb/./test.c)
[symtab-create] watch_main_source_file_lossage: using subfile ./test.c as the main subfile
As we can see, there are two forms used for "test.c", one with a "." and
one without. This comes from the fact that the compilation unit DIE
contains:
DW_AT_name ("test.c")
DW_AT_comp_dir ("/home/simark/build/binutils-gdb-one-target/gdb")
without a ".", and the line table for that file contains:
include_directories[ 1] = "."
file_names[ 1]:
name: "test.c"
dir_index: 1
When assembling the filename from that entry, we get a ".".
It is a bit unexpected that the main filename resulting from the line
table header does not match exactly the name in the compilation unit.
For instance, gcc uses "./test.c" for the DW_AT_name, which gives
identical paths in the compilation unit and in the line table header.
Similary, with DWARF 5:
$ clang ./test.c -g3 -O0 -gdwarf-5
clang create two entries that refer to the same file but are of in a different
form.
include_directories[ 0] = "/home/simark/build/binutils-gdb-one-target/gdb"
include_directories[ 1] = "."
file_names[ 0]:
name: "test.c"
dir_index: 0
file_names[ 1]:
name: "test.c"
dir_index: 1
The first file name produces a path without a "." while the second does.
This is not caught by watch_main_source_file_lossage, because of
dwarf_decode_lines that creates a symtab for each file entry in the line
table. It therefore appears as "non-empty" to
watch_main_source_file_lossage. This results in two symtabs:
(gdb) maintenance info symtabs
{ objfile /home/simark/build/binutils-gdb-one-target/gdb/a.out ((struct objfile *) 0x613000005d00)
{ ((struct compunit_symtab *) 0x62100011aca0)
debugformat DWARF 5
producer clang version 13.0.1
name test.c
dirname /home/simark/build/binutils-gdb-one-target/gdb
blockvector ((struct blockvector *) 0x621000129ec0)
user ((struct compunit_symtab *) (null))
{ symtab test.c ((struct symtab *) 0x62100011ad20)
fullname (null)
linetable ((struct linetable *) 0x0)
}
{ symtab ./test.c ((struct symtab *) 0x62100011ad60)
fullname (null)
linetable ((struct linetable *) 0x621000129ef0)
}
}
}
I am not sure what is the consequence of this, but this is also what
happens before my patch, so I think its acceptable to leave it as-is.
To handle these two cases nicely, I think we will need a function that
removes the unnecessary "." from path names, something that can be done
later.
Change-Id: I6fb4f10de040602779da07a0934b7078701a1856
---
gdb/buildsym-legacy.c | 4 +--
gdb/buildsym.c | 36 ++++++++++++++-------------
gdb/buildsym.h | 26 +++++++++++++++++---
gdb/ctfread.c | 2 +-
gdb/dwarf2/cu.c | 17 ++++++++++++-
| 13 +++++++---
| 6 +++--
gdb/dwarf2/read.c | 53 +++++++++++++++++++++-------------------
gdb/jit.c | 3 ++-
gdb/macroscope.c | 2 +-
gdb/mdebugread.c | 2 +-
gdb/symfile.c | 4 ++-
gdb/symfile.h | 3 ++-
gdb/symtab.h | 14 ++++++++++-
gdb/xcoffread.c | 1 +
15 files changed, 124 insertions(+), 62 deletions(-)
diff --git a/gdb/buildsym-legacy.c b/gdb/buildsym-legacy.c
index 490eef965d02..12d4c1e0ec15 100644
--- a/gdb/buildsym-legacy.c
+++ b/gdb/buildsym-legacy.c
@@ -135,7 +135,7 @@ void
start_subfile (const char *name)
{
gdb_assert (buildsym_compunit != nullptr);
- buildsym_compunit->start_subfile (name);
+ buildsym_compunit->start_subfile (name, name);
}
void
@@ -235,7 +235,7 @@ start_compunit_symtab (struct objfile *objfile, const char *name,
gdb_assert (buildsym_compunit == nullptr);
buildsym_compunit = new struct buildsym_compunit (objfile, name, comp_dir,
- language, start_addr);
+ name, language, start_addr);
return buildsym_compunit->get_compunit_symtab ();
}
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index feef5038773f..bb258b983ca4 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -53,6 +53,7 @@ struct pending_block
buildsym_compunit::buildsym_compunit (struct objfile *objfile_,
const char *name,
const char *comp_dir_,
+ const char *name_for_id,
enum language language_,
CORE_ADDR last_addr)
: m_objfile (objfile_),
@@ -71,7 +72,7 @@ buildsym_compunit::buildsym_compunit (struct objfile *objfile_,
It can happen that the debug info provides a different path to NAME than
DIRNAME,NAME. We cope with this in watch_main_source_file_lossage but
that only works if the main_subfile doesn't have a symtab yet. */
- start_subfile (name);
+ start_subfile (name, name_for_id);
/* Save this so that we don't have to go looking for it at the end
of the subfiles list. */
m_main_subfile = m_current_subfile;
@@ -486,40 +487,38 @@ buildsym_compunit::make_blockvector ()
return (blockvector);
}
-\f
-/* Start recording information about source code that came from an
- included (or otherwise merged-in) source file with a different
- name. NAME is the name of the file (cannot be NULL). */
+
+/* See buildsym.h. */
void
-buildsym_compunit::start_subfile (const char *name)
+buildsym_compunit::start_subfile (const char *name, const char *name_for_id)
{
/* See if this subfile is already registered. */
- symtab_create_debug_printf ("name = %s", name);
+ symtab_create_debug_printf ("name = %s, name_for_id = %s", name, name_for_id);
for (subfile *subfile = m_subfiles; subfile; subfile = subfile->next)
{
std::string subfile_name_holder;
- const char *subfile_name;
+ const char *subfile_name_for_id;
/* If NAME is an absolute path, and this subfile is not, then
attempt to create an absolute path to compare. */
- if (IS_ABSOLUTE_PATH (name)
- && !IS_ABSOLUTE_PATH (subfile->name)
+ if (IS_ABSOLUTE_PATH (name_for_id)
+ && !IS_ABSOLUTE_PATH (subfile->name_for_id)
&& !m_comp_dir.empty ())
{
subfile_name_holder = path_join (m_comp_dir.c_str (),
- subfile->name.c_str ());
- subfile_name = subfile_name_holder.c_str ();
+ subfile->name_for_id.c_str ());
+ subfile_name_for_id = subfile_name_holder.c_str ();
}
else
- subfile_name = subfile->name.c_str ();
+ subfile_name_for_id = subfile->name_for_id.c_str ();
- if (FILENAME_CMP (subfile_name, name) == 0)
+ if (FILENAME_CMP (subfile_name_for_id, name_for_id) == 0)
{
- symtab_create_debug_printf ("found existing symtab with name %s (%s)",
- subfile->name.c_str (), subfile_name);
+ symtab_create_debug_printf ("found existing symtab with name_for_id %s (%s)",
+ subfile->name_for_id.c_str (), subfile_name_for_id);
m_current_subfile = subfile;
return;
}
@@ -529,6 +528,7 @@ buildsym_compunit::start_subfile (const char *name)
subfile_up subfile (new struct subfile);
subfile->name = name;
+ subfile->name_for_id = name_for_id;
m_current_subfile = subfile.get ();
@@ -598,6 +598,7 @@ buildsym_compunit::patch_subfile_names (struct subfile *subfile,
{
m_comp_dir = std::move (subfile->name);
subfile->name = name;
+ subfile->name_for_id = name;
set_last_source_file (name);
/* Default the source language to whatever can be deduced from
@@ -935,7 +936,8 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
/* Allocate a symbol table if necessary. */
if (subfile->symtab == NULL)
- subfile->symtab = allocate_symtab (cu, subfile->name.c_str ());
+ subfile->symtab = allocate_symtab (cu, subfile->name.c_str (),
+ subfile->name_for_id.c_str ());
struct symtab *symtab = subfile->symtab;
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index ee75e6fd95d3..db04be03533a 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -54,6 +54,12 @@ struct subfile
struct subfile *next = nullptr;
std::string name;
+
+ /* This field is analoguous in function to symtab::filename_for_id.
+
+ It is used to look up existing subfiles in calls to start_subfile. */
+ std::string name_for_id;
+
std::vector<linetable_entry> line_vector_entries;
enum language language = language_unknown;
struct symtab *symtab = nullptr;
@@ -134,12 +140,16 @@ struct buildsym_compunit
{
/* Start recording information about a primary source file (IOW, not an
included source file).
+
COMP_DIR is the directory in which the compilation unit was compiled
- (or NULL if not known). */
+ (or NULL if not known).
+
+ NAME and NAME_FOR_ID have the same purpose as for the start_subfile
+ method. */
buildsym_compunit (struct objfile *objfile_, const char *name,
- const char *comp_dir_, enum language language_,
- CORE_ADDR last_addr);
+ const char *comp_dir_, const char *name_for_id,
+ enum language language_, CORE_ADDR last_addr);
/* Reopen an existing compunit_symtab so that additional symbols can
be added to it. Arguments are as for the main constructor. CUST
@@ -197,7 +207,15 @@ struct buildsym_compunit
void record_block_range (struct block *block,
CORE_ADDR start, CORE_ADDR end_inclusive);
- void start_subfile (const char *name);
+ /* Start recording information about source code that comes from a source
+ file. This sets the current subfile, creating it if necessary.
+
+ NAME is the user-visible name of the subfile.
+
+ NAME_FOR_ID is a name that must be stable between the different calls to
+ start_subfile referring to the same file (it is used for looking up
+ existing subfiles). It can be equal to NAME if NAME follows that rule. */
+ void start_subfile (const char *name, const char *name_for_id);
void patch_subfile_names (struct subfile *subfile, const char *name);
diff --git a/gdb/ctfread.c b/gdb/ctfread.c
index 828f300d29d5..04db548001d3 100644
--- a/gdb/ctfread.c
+++ b/gdb/ctfread.c
@@ -1253,7 +1253,7 @@ ctf_start_compunit_symtab (ctf_psymtab *pst,
ccp = &pst->context;
ccp->builder = new buildsym_compunit
- (of, pst->filename, nullptr,
+ (of, pst->filename, nullptr, pst->filename,
language_c, text_offset);
ccp->builder->record_debugformat ("ctf");
}
diff --git a/gdb/dwarf2/cu.c b/gdb/dwarf2/cu.c
index fcb0d77623bf..995a25e47e4e 100644
--- a/gdb/dwarf2/cu.c
+++ b/gdb/dwarf2/cu.c
@@ -21,6 +21,8 @@
#include "dwarf2/cu.h"
#include "dwarf2/read.h"
#include "objfiles.h"
+#include "filenames.h"
+#include "gdbsupport/pathstuff.h"
/* Initialize dwarf2_cu to read PER_CU, in the context of PER_OBJFILE. */
@@ -60,9 +62,22 @@ dwarf2_cu::start_compunit_symtab (const char *name, const char *comp_dir,
{
gdb_assert (m_builder == nullptr);
+ std::string name_for_id_holder;
+ const char *name_for_id = name;
+
+ /* Prepend the compilation directory to the filename if needed (if not
+ absolute already) to get the "name for id" for our main symtab. The name
+ for the main file coming from the line table header will be generated using
+ the same logic, so will hopefully match what we pass here. */
+ if (!IS_ABSOLUTE_PATH (name) && comp_dir != nullptr)
+ {
+ name_for_id_holder = path_join (comp_dir, name);
+ name_for_id = name_for_id_holder.c_str ();
+ }
+
m_builder.reset (new struct buildsym_compunit
(this->per_objfile->objfile,
- name, comp_dir, per_cu->lang, low_pc));
+ name, comp_dir, name_for_id, per_cu->lang, low_pc));
list_in_scope = get_builder ()->get_file_symbols ();
--git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 33af77d3ecf3..609b00edeb10 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -62,14 +62,19 @@ line_header::file_file_name (const file_entry &fe) const
{
gdb_assert (is_valid_file_index (fe.index));
- if (IS_ABSOLUTE_PATH (fe.name))
- return fe.name;
+ std::string ret = fe.name;
+
+ if (IS_ABSOLUTE_PATH (ret))
+ return ret;
const char *dir = fe.include_dir (this);
if (dir != nullptr)
- return path_join (dir, fe.name);
+ ret = path_join (dir, ret);
+
+ if (IS_ABSOLUTE_PATH (ret))
+ return ret;
- return fe.name;
+ return path_join (m_comp_dir, ret);
}
static void
--git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h
index 0fe539b0c427..44c0c59d4264 100644
--- a/gdb/dwarf2/line-header.h
+++ b/gdb/dwarf2/line-header.h
@@ -171,8 +171,10 @@ struct line_header
header. These point into dwarf2_per_objfile->line_buffer. */
const gdb_byte *statement_program_start {}, *statement_program_end {};
- /* Return file name relative to the compilation directory of file
- FE in this object's file name table. */
+ /* Return the most "complete" file name for FILE possible.
+
+ This means prepending the directory and compilation directory, as needed,
+ until we get an absolute path. */
std::string file_file_name (const file_entry &fe) const;
/* Return the compilation directory of the compilation unit in the context of
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 56f58a004af6..af43fcd81cd1 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -950,8 +950,8 @@ static void dwarf_decode_lines (struct line_header *,
struct dwarf2_cu *,
CORE_ADDR, int decode_mapping);
-static void dwarf2_start_subfile (struct dwarf2_cu *, const char *,
- const char *);
+static void dwarf2_start_subfile (dwarf2_cu *cu, const file_entry &fe,
+ const line_header &lh);
static struct symbol *new_symbol (struct die_info *, struct type *,
struct dwarf2_cu *, struct symbol * = NULL);
@@ -9665,18 +9665,20 @@ dwarf2_cu::setup_type_unit_groups (struct die_info *die)
for (i = 0; i < file_names.size (); ++i)
{
file_entry &fe = file_names[i];
- dwarf2_start_subfile (this, fe.name,
- fe.include_dir (line_header));
+ dwarf2_start_subfile (this, fe, *line_header);
buildsym_compunit *b = get_builder ();
- if (b->get_current_subfile ()->symtab == NULL)
+ subfile *sf = b->get_current_subfile ();
+
+ if (sf->symtab == nullptr)
{
/* NOTE: start_subfile will recognize when it's been
passed a file it has already seen. So we can't
assume there's a simple mapping from
cu->line_header->file_names to subfiles, plus
cu->line_header->file_names may contain dups. */
- const char *name = b->get_current_subfile ()->name.c_str ();
- b->get_current_subfile ()->symtab = allocate_symtab (cust, name);
+ const char *name = sf->name.c_str ();
+ const char *name_for_id = sf->name_for_id.c_str ();
+ sf->symtab = allocate_symtab (cust, name, name_for_id);
}
fe.symtab = b->get_current_subfile ()->symtab;
@@ -19946,11 +19948,9 @@ lnp_state_machine::handle_set_file (file_name_index file)
dwarf2_debug_line_missing_file_complaint ();
else
{
- const char *dir = fe->include_dir (m_line_header);
-
m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
m_line_has_non_zero_discriminator = m_discriminator != 0;
- dwarf2_start_subfile (m_cu, fe->name, dir);
+ dwarf2_start_subfile (m_cu, *fe, *m_line_header);
}
}
@@ -20233,7 +20233,7 @@ dwarf_decode_lines_1 (struct line_header *lh, struct dwarf2_cu *cu,
const file_entry *fe = state_machine.current_file ();
if (fe != NULL)
- dwarf2_start_subfile (cu, fe->name, fe->include_dir (lh));
+ dwarf2_start_subfile (cu, *fe, *lh);
/* Decode the table. */
while (line_ptr < line_end && !end_sequence)
@@ -20445,14 +20445,14 @@ dwarf_decode_lines (struct line_header *lh, struct dwarf2_cu *cu,
for (auto &fe : lh->file_names ())
{
- dwarf2_start_subfile (cu, fe.name, fe.include_dir (lh));
- if (builder->get_current_subfile ()->symtab == NULL)
- {
- builder->get_current_subfile ()->symtab
- = allocate_symtab (cust,
- builder->get_current_subfile ()->name.c_str ());
- }
- fe.symtab = builder->get_current_subfile ()->symtab;
+ dwarf2_start_subfile (cu, fe, *lh);
+ subfile *sf = builder->get_current_subfile ();
+
+ if (sf->symtab == nullptr)
+ sf->symtab = allocate_symtab (cust, sf->name.c_str (),
+ sf->name_for_id.c_str ());
+
+ fe.symtab = sf->symtab;
}
}
@@ -20480,10 +20480,12 @@ dwarf_decode_lines (struct line_header *lh, struct dwarf2_cu *cu,
subfile's name. */
static void
-dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
- const char *dirname)
+dwarf2_start_subfile (dwarf2_cu *cu, const file_entry &fe,
+ const line_header &lh)
{
- std::string copy;
+ std::string filename_holder;
+ const char *filename = fe.name;
+ const char *dirname = lh.include_dir_at (fe.d_index);
/* In order not to lose the line information directory,
we concatenate it to the filename when it makes sense.
@@ -20494,11 +20496,12 @@ dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL)
{
- copy = path_join (dirname, filename);
- filename = copy.c_str ();
+ filename_holder = path_join (dirname, filename);
+ filename = filename_holder.c_str ();
}
- cu->get_builder ()->start_subfile (filename);
+ std::string filename_for_id = lh.file_file_name (fe);
+ cu->get_builder ()->start_subfile (filename, filename_for_id.c_str ());
}
static void
diff --git a/gdb/jit.c b/gdb/jit.c
index b4a070bb8796..26e8a3fa0cd0 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -537,7 +537,8 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
});
cust = allocate_compunit_symtab (objfile, stab->file_name.c_str ());
- symtab *filetab = allocate_symtab (cust, stab->file_name.c_str ());
+ symtab *filetab = allocate_symtab (cust, stab->file_name.c_str (),
+ stab->file_name.c_str ());
add_compunit_symtab_to_objfile (cust);
/* JIT compilers compile in memory. */
diff --git a/gdb/macroscope.c b/gdb/macroscope.c
index 93f561acccd8..fe7f10e296ab 100644
--- a/gdb/macroscope.c
+++ b/gdb/macroscope.c
@@ -51,7 +51,7 @@ sal_macro_scope (struct symtab_and_line sal)
gdb::unique_xmalloc_ptr<struct macro_scope> ms (XNEW (struct macro_scope));
main_file = macro_main (cust->macro_table ());
- inclusion = macro_lookup_inclusion (main_file, sal.symtab->filename);
+ inclusion = macro_lookup_inclusion (main_file, sal.symtab->filename_for_id);
if (inclusion)
{
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index ca7c15ee63fd..32436d836f28 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -4620,7 +4620,7 @@ new_symtab (const char *name, int maxlines, struct objfile *objfile)
enum language lang;
add_compunit_symtab_to_objfile (cust);
- symtab = allocate_symtab (cust, name);
+ symtab = allocate_symtab (cust, name, name);
symtab->set_linetable (new_linetable (maxlines));
lang = compunit_language (cust);
diff --git a/gdb/symfile.c b/gdb/symfile.c
index b9ecef045377..80a26a7f7937 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2772,13 +2772,15 @@ deduce_language_from_filename (const char *filename)
CUST is from the result of allocate_compunit_symtab. */
struct symtab *
-allocate_symtab (struct compunit_symtab *cust, const char *filename)
+allocate_symtab (struct compunit_symtab *cust, const char *filename,
+ const char *filename_for_id)
{
struct objfile *objfile = cust->objfile ();
struct symtab *symtab
= OBSTACK_ZALLOC (&objfile->objfile_obstack, struct symtab);
symtab->filename = objfile->intern (filename);
+ symtab->filename_for_id = objfile->intern (filename_for_id);
symtab->fullname = NULL;
symtab->set_language (deduce_language_from_filename (filename));
diff --git a/gdb/symfile.h b/gdb/symfile.h
index f44dfa060a0c..243581b35273 100644
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -201,7 +201,8 @@ extern symfile_segment_data_up default_symfile_segments (bfd *abfd);
extern bfd_byte *default_symfile_relocate (struct objfile *objfile,
asection *sectp, bfd_byte *buf);
-extern struct symtab *allocate_symtab (struct compunit_symtab *, const char *)
+extern struct symtab *allocate_symtab
+ (struct compunit_symtab *cust, const char *filename, const char *id)
ATTRIBUTE_NONNULL (1);
extern struct compunit_symtab *allocate_compunit_symtab (struct objfile *,
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 854167dde452..45fa6ffa7369 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1643,10 +1643,22 @@ struct symtab
struct linetable *m_linetable;
- /* Name of this source file. This pointer is never NULL. */
+ /* Name of this source file, in a form appropriate to print to the user.
+
+ This pointer is never nullptr. */
const char *filename;
+ /* Filename for this source file, used as an identifier to link with
+ related objects such as associated macro_source_file objects. It must
+ therefore match the name of any macro_source_file object created for this
+ source file. The value can be the same as FILENAME if it is known to
+ follow that rule, or another form of the same file name, this is up to
+ the specific debug info reader.
+
+ This pointer is never nullptr.*/
+ const char *filename_for_id;
+
/* Language of this source file. */
enum language m_language;
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index d8735d29ede1..2e6833487b28 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -713,6 +713,7 @@ process_linenos (CORE_ADDR start, CORE_ADDR end)
}
struct subfile *current_subfile = get_current_subfile ();
current_subfile->name = inclTable[ii].name;
+ current_subfile->name_for_id = inclTable[ii].name;
#endif
start_subfile (pop_subfile ());
--
2.35.2
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/7] gdb: add "id" fields to identify symtabs and subfiles
2022-04-28 3:35 ` [PATCH v3 5/7] gdb: add "id" fields to identify symtabs and subfiles Simon Marchi
@ 2022-04-28 23:53 ` Lancelot SIX
2022-07-28 17:46 ` Simon Marchi
0 siblings, 1 reply; 21+ messages in thread
From: Lancelot SIX @ 2022-04-28 23:53 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
Hi,
Just some nits below.
When applying the patch I have:
Applying: gdb: add "id" fields to identify symtabs and subfiles
.git/rebase-apply/patch:153: trailing whitespace.
It is used to look up existing subfiles in calls to start_subfile. */
warning: 1 line adds whitespace errors.
On Wed, Apr 27, 2022 at 11:35:40PM -0400, Simon Marchi via Gdb-patches wrote:
> Printing macros defined in the main source file doesn't work reliably
> using various toolchains, especially when DWARF 5 is used. For example,
> using the binaries produced by either of these commands:
>
> $ gcc --version
> gcc (GCC) 11.2.0
> $ ld --version
> GNU ld (GNU Binutils) 2.38
> $ gcc test.c -g3 -gdwarf-5
>
> $ clang --version
> clang version 13.0.1
> $ clang test.c -gdwarf-5 -fdebug-macro
>
> I get:
>
> $ ./gdb -nx -q --data-directory=data-directory a.out
> (gdb) start
> Temporary breakpoint 1 at 0x111d: file test.c, line 6.
> Starting program: /home/simark/build/binutils-gdb-one-target/gdb/a.out
>
> Temporary breakpoint 1, main () at test.c:6
> 6 return ZERO;
> (gdb) p ZERO
> No symbol "ZERO" in current context.
>
> When starting to investigate this (taking the gcc-compiled binary as an
> example), we see that GDB fails to look up the appropriate macro scope
> when evaluating the expression. While stopped in
> macro_lookup_inclusion:
>
> (top-gdb) p name
> $1 = 0x62100011a980 "test.c"
> (top-gdb) p source.filename
> $2 = 0x62100011a9a0 "/home/simark/build/binutils-gdb-one-target/gdb/test.c"
>
> `source` is the macro_source_file that we would expect GDB to find.
> `name` comes from the symtab::filename field of the symtab we are
> stopped in. GDB doesn't find the appropriate macro_source_file because
> the name of the macro_source_file doesn't match exactly the name of the
> symtab.
>
> The name of the main symtab comes from the compilation unit's
> DW_AT_name, passed to the buildsym_compunit's constructor:
>
> https://gitlab.com/gnutools/binutils-gdb/-/blob/4815d6125ec580cc02a1094d61b8c9d1cc83c0a1/gdb/dwarf2/read.c#L10627-10630
>
> The contents of DW_AT_name, in this case, is "test.c". It is typically
> (what I witnessed all compilers do) the same string that was passed to
> the compiler on the command-line.
>
> The name of the macro_source_file comes from the line number program
> header's file table, from the call to the line_header::file_file_name
> method:
>
> https://gitlab.com/gnutools/binutils-gdb/-/blob/4815d6125ec580cc02a1094d61b8c9d1cc83c0a1/gdb/dwarf2/macro.c#L54-65
>
> line_header::file_file_name prepends the directory path that the file
> entry refers to, in the file table (if the file name is not already
> absolute). In this case, the file name is "test.c", appended to the
> directory "/home/simark/build/binutils-gdb-one-target/gdb".
>
> Because the symtab's name is not created the same way as the
> macro_source_file's name is created, we get this mismatch. GDB fails to
> find the appropriate macro scope for the symtab, and we can't print
> macros when stopped in that symtab.
>
> To make this work, we must ensure that paths produced in these two ways
> end up identical. This can be tricky because of the different ways a
> path can be passed to the compiler.
>
> Another this to consider is that while the main symtab's name (or
s/this/thing/ ?
> subfile, before it becomes a symtab) is created using DW_AT_name, the
> main symtab is also referred to using its entry in the line table
> header's file table, when processing the line table. We must therefore
> ensure that the same name is produced in both cases, so that a call to
> "start_subfile" for the main subfile will correctly find the
> already-created subfile, created by buildsym_compunit's constructor. If
> we fail to do that, things still often work, because of a fallback: the
> watch_main_source_file_lossage method. This method determines that if
> the main subfile has no symbols but there exists another subfile with
> the same basename (e.g. "test.c") that does have symbols, it's probably
> because there was some filename mismatch. So it replaces the main
> subfile with that other subfile. I think that heuristic is useful as a
> last effort to work around any bug or bad debug info, but I don't think
> we should design things such as to rely on it. It's a heuristic, it can
> get things wrong. So in my search for a fix, it is important that given
> some good debug info, we don't end up relying on that for things to
> work.
>
> A first attempt at fixing this was to try to prepend the compilation
> directory here or not prepend it there. In practice, because of all the
> possible combinations of debug info the compilers produce, it was not
> possible to get something that would produce reliable, consistent paths.
>
> Another attempt at fixing this was to make both macro_source_file
> objects and symtab objects use the most complete form of path possible.
> That means to prepend directories at least until we get an absolute
> path. In theory, we should end up with the same path in all cases.
> This generally worked, but because it changed the symtab names, it
> resulted in user-visible changes (for example, paths to source files in
> Breakpoint hit messages becoming always absolute). I didn't find this
> very good, first because there is a "set filename-display" setting that
> lets the user control how they want the paths to be displayed, and that
> would suddenly make this setting completely ineffective (although even
> today, it is a bit dependent on the debug info). Second, it would
> require a good amount of testsuite tweaks to make tests accept these
> suddenly absolute paths.
>
> This new patch is a slight variation of that: it adds a new field called
> "filename_for_id" in struct symtab and struct subfile, next to the
> existing filename field. The goal is to separate the internal ids used
> for finding objects from the names used for presentation. This field is
> used for identifying subfiles, symtabs and macro_source_files
> internally. For DWARF symtabs, this new field is meant to contain the
> "most complete possible" path, as discussed above. So for a given file,
> it must always be in the same form, everywhere. The existing
> symtab::filename field remains the one used for printing to the user, so
> there shouldn't be any change in how paths are printed.
>
> Changes in the core symtab files are:
>
> - Add "name_for_id" and "filename_for_id" fields to "struct subfile"
> and "struct symta"b, next to existing "name" and "filename" fields.
^
Two chars are swapped here.
> - Make buildsym_compunit::buildsym_compunit and
> buildsym_compunit::start_subfile accept a "name_for_id" parameter
> next to the existing "name" ones.
> - Make buildsym_compunit::start_subfile used "name_for_id" for looking
> up existing subfiles. This is the key thing for making calls
> to start_subfile for the main source file look up the existing
> subfile successfully, and avoid relying on
> watch_main_source_file_lossage.
> - Make sal_macro_scope pass "filename_for_id", rather than "filename",
> to macro_lookup_inclusion. This is the key thing to making the
> lookup work and macro printing work.
>
> Changes in the DWARF files are:
>
> - Make line_header::file_file_name return the "most complete possible"
> name. The only pre-existing user of this method is the macro code,
> to give the macro_source_file objects their name. And we now want
> them to have this "most complete possible" name, which will match the
> corresponding symtab's "filename_for_id".
> - Make dwarf2_cu::start_compunit_symtab pass the "most complete
> possible" name for the main symtab's "filename_for_id". In this
> context, where the info comes from the compilation unit's DW_AT_name
> / DW_AT_comp_dir, it means prepending DW_AT_comp_dir to DW_AT_name if
> DW_AT_name is not already absolute.
> - Change dwarf2_start_subfile to build a name_for_id for the subfile
> being started. The simplest way is to re-use
> line_header::file_file_name, since the callers always have a
> file_entry handy. This ensures that it will get the exact same path
> representation as the macro code does, for the same file (since it
> also uses line_header::file_file_name).
> - Update calls to allocate_symtab to pass the "name_for_id" from the
> subfile.
>
> The rest of the changes are to update the other symtab users (jit,
> ctfread, mdebugread, xcoffread). For those, the same value is passed
> for the "id" as the for the filename, so they should keep the same
> behavior they have today.
I guess this is just personal preference, but I would have chosen to
add overloads so those files do not have to change / bother about an
argument which is meaningless for them.
Something like:
// gdb/buildsym.h
struct buildsym_compunit
{
buildsym_compunit (struct objfile *objfile_, const char *name,
const char *comp_dir_, enum language language_,
CORE_ADDR last_addr)
: buildsym_compunit (objfile_, name, comp_dir_, name,
language_, last_addr);
buildsym_compunit (struct objfile *objfile_, const char *name,
const char *comp_dir_, const char *name_for_id,
language language_, CORE_ADDR last_addr);
...
void start_subfile (const char *name, const char *name_for_id);
void start_subfile (const char *name)
{ start_subfile (name, name); }
};
// gdb/symfile.h
extern struct symtab *allocate_symtab
(struct compunit_symtab *cust, const char *filename, const char *id))
ATTRIBUTE_NONNULL (1);
static inline struct symtab *
allocate_symtab (struct compunit_symtab *cust, const char *filename)
{
return allocate_sumtab (cust, filename, filename);
}
In the end it does not make much difference, your approach work also
perfectly fine so feel free to keep it the way it is.
Otherwise, FWIW I find the approach to keep one string representation of
the filename for display and one for some sort of canonicalization
perfectly appropriate.
>
> Tests exercising all this are added by the following patch.
>
> Of all the cases I tried, the only one I found that ends up relying on
> watch_main_source_file_lossage is the following one:
>
> $ clang --version
> clang version 13.0.1
> Target: x86_64-pc-linux-gnu
> Thread model: posix
> InstalledDir: /usr/bin
> $ clang ./test.c -g3 -O0 -gdwarf-4
> $ ./gdb -nx --data-directory=data-directory -q -readnow -iex "set debug symtab-create 1" a.out
> ...
> [symtab-create] start_subfile: name = test.c, name_for_id = /home/simark/build/binutils-gdb-one-target/gdb/test.c
> [symtab-create] start_subfile: name = ./test.c, name_for_id = /home/simark/build/binutils-gdb-one-target/gdb/./test.c
> [symtab-create] start_subfile: name = ./test.c, name_for_id = /home/simark/build/binutils-gdb-one-target/gdb/./test.c
> [symtab-create] start_subfile: found existing symtab with name_for_id /home/simark/build/binutils-gdb-one-target/gdb/./test.c (/home/simark/build/binutils-gdb-one-target/gdb/./test.c)
> [symtab-create] watch_main_source_file_lossage: using subfile ./test.c as the main subfile
>
> As we can see, there are two forms used for "test.c", one with a "." and
> one without. This comes from the fact that the compilation unit DIE
> contains:
>
> DW_AT_name ("test.c")
> DW_AT_comp_dir ("/home/simark/build/binutils-gdb-one-target/gdb")
>
> without a ".", and the line table for that file contains:
>
> include_directories[ 1] = "."
> file_names[ 1]:
> name: "test.c"
> dir_index: 1
>
> When assembling the filename from that entry, we get a ".".
>
> It is a bit unexpected that the main filename resulting from the line
> table header does not match exactly the name in the compilation unit.
> For instance, gcc uses "./test.c" for the DW_AT_name, which gives
> identical paths in the compilation unit and in the line table header.
>
> Similary, with DWARF 5:
s/Similary/Similarly/
Best,
Lancelot.
>
> $ clang ./test.c -g3 -O0 -gdwarf-5
>
> clang create two entries that refer to the same file but are of in a different
> form.
>
> include_directories[ 0] = "/home/simark/build/binutils-gdb-one-target/gdb"
> include_directories[ 1] = "."
> file_names[ 0]:
> name: "test.c"
> dir_index: 0
> file_names[ 1]:
> name: "test.c"
> dir_index: 1
>
> The first file name produces a path without a "." while the second does.
> This is not caught by watch_main_source_file_lossage, because of
> dwarf_decode_lines that creates a symtab for each file entry in the line
> table. It therefore appears as "non-empty" to
> watch_main_source_file_lossage. This results in two symtabs:
>
> (gdb) maintenance info symtabs
> { objfile /home/simark/build/binutils-gdb-one-target/gdb/a.out ((struct objfile *) 0x613000005d00)
> { ((struct compunit_symtab *) 0x62100011aca0)
> debugformat DWARF 5
> producer clang version 13.0.1
> name test.c
> dirname /home/simark/build/binutils-gdb-one-target/gdb
> blockvector ((struct blockvector *) 0x621000129ec0)
> user ((struct compunit_symtab *) (null))
> { symtab test.c ((struct symtab *) 0x62100011ad20)
> fullname (null)
> linetable ((struct linetable *) 0x0)
> }
> { symtab ./test.c ((struct symtab *) 0x62100011ad60)
> fullname (null)
> linetable ((struct linetable *) 0x621000129ef0)
> }
> }
> }
>
> I am not sure what is the consequence of this, but this is also what
> happens before my patch, so I think its acceptable to leave it as-is.
>
> To handle these two cases nicely, I think we will need a function that
> removes the unnecessary "." from path names, something that can be done
> later.
>
> Change-Id: I6fb4f10de040602779da07a0934b7078701a1856
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/7] gdb: add "id" fields to identify symtabs and subfiles
2022-04-28 23:53 ` Lancelot SIX
@ 2022-07-28 17:46 ` Simon Marchi
0 siblings, 0 replies; 21+ messages in thread
From: Simon Marchi @ 2022-07-28 17:46 UTC (permalink / raw)
To: Lancelot SIX; +Cc: gdb-patches
On 4/28/22 19:53, Lancelot SIX wrote:
> Hi,
>
> Just some nits below.
>
> When applying the patch I have:
>
> Applying: gdb: add "id" fields to identify symtabs and subfiles
> .git/rebase-apply/patch:153: trailing whitespace.
> It is used to look up existing subfiles in calls to start_subfile. */
> warning: 1 line adds whitespace errors.
Fixed.
>> Another this to consider is that while the main symtab's name (or
>
> s/this/thing/ ?
Fixed.
>> Changes in the core symtab files are:
>>
>> - Add "name_for_id" and "filename_for_id" fields to "struct subfile"
>> and "struct symta"b, next to existing "name" and "filename" fields.
> ^
> Two chars are swapped here.
Fixed.
>> - Make buildsym_compunit::buildsym_compunit and
>> buildsym_compunit::start_subfile accept a "name_for_id" parameter
>> next to the existing "name" ones.
>> - Make buildsym_compunit::start_subfile used "name_for_id" for looking
>> up existing subfiles. This is the key thing for making calls
>> to start_subfile for the main source file look up the existing
>> subfile successfully, and avoid relying on
>> watch_main_source_file_lossage.
>> - Make sal_macro_scope pass "filename_for_id", rather than "filename",
>> to macro_lookup_inclusion. This is the key thing to making the
>> lookup work and macro printing work.
>>
>> Changes in the DWARF files are:
>>
>> - Make line_header::file_file_name return the "most complete possible"
>> name. The only pre-existing user of this method is the macro code,
>> to give the macro_source_file objects their name. And we now want
>> them to have this "most complete possible" name, which will match the
>> corresponding symtab's "filename_for_id".
>> - Make dwarf2_cu::start_compunit_symtab pass the "most complete
>> possible" name for the main symtab's "filename_for_id". In this
>> context, where the info comes from the compilation unit's DW_AT_name
>> / DW_AT_comp_dir, it means prepending DW_AT_comp_dir to DW_AT_name if
>> DW_AT_name is not already absolute.
>> - Change dwarf2_start_subfile to build a name_for_id for the subfile
>> being started. The simplest way is to re-use
>> line_header::file_file_name, since the callers always have a
>> file_entry handy. This ensures that it will get the exact same path
>> representation as the macro code does, for the same file (since it
>> also uses line_header::file_file_name).
>> - Update calls to allocate_symtab to pass the "name_for_id" from the
>> subfile.
>>
>> The rest of the changes are to update the other symtab users (jit,
>> ctfread, mdebugread, xcoffread). For those, the same value is passed
>> for the "id" as the for the filename, so they should keep the same
>> behavior they have today.
>
> I guess this is just personal preference, but I would have chosen to
> add overloads so those files do not have to change / bother about an
> argument which is meaningless for them.
>
> Something like:
>
> // gdb/buildsym.h
> struct buildsym_compunit
> {
> buildsym_compunit (struct objfile *objfile_, const char *name,
> const char *comp_dir_, enum language language_,
> CORE_ADDR last_addr)
> : buildsym_compunit (objfile_, name, comp_dir_, name,
> language_, last_addr);
> buildsym_compunit (struct objfile *objfile_, const char *name,
> const char *comp_dir_, const char *name_for_id,
> language language_, CORE_ADDR last_addr);
>
> ...
>
> void start_subfile (const char *name, const char *name_for_id);
>
> void start_subfile (const char *name)
> { start_subfile (name, name); }
> };
>
> // gdb/symfile.h
> extern struct symtab *allocate_symtab
> (struct compunit_symtab *cust, const char *filename, const char *id))
> ATTRIBUTE_NONNULL (1);
>
> static inline struct symtab *
> allocate_symtab (struct compunit_symtab *cust, const char *filename)
> {
> return allocate_sumtab (cust, filename, filename);
> }
>
> In the end it does not make much difference, your approach work also
> perfectly fine so feel free to keep it the way it is.
I did that.
>
> Otherwise, FWIW I find the approach to keep one string representation of
> the filename for display and one for some sort of canonicalization
> perfectly appropriate.
Ok, thanks for the review.
>> Similary, with DWARF 5:
>
> s/Similary/Similarly/
Fixed.
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 6/7] gdb: remove code to prepend comp dir in buildsym_compunit::start_subfile
2022-04-28 3:35 [PATCH v3 0/7] Fix printing macros Simon Marchi
` (4 preceding siblings ...)
2022-04-28 3:35 ` [PATCH v3 5/7] gdb: add "id" fields to identify symtabs and subfiles Simon Marchi
@ 2022-04-28 3:35 ` Simon Marchi
2022-05-12 13:07 ` Bruno Larsen
2022-04-28 3:35 ` [PATCH v3 7/7] gdb/testsuite: add macros test for source files compiled in various ways Simon Marchi
2022-07-30 0:56 ` [PATCH v3 0/7] Fix printing macros Simon Marchi
7 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2022-04-28 3:35 UTC (permalink / raw)
To: gdb-patches
The bit of code removed by this patch was introduced to fix the same
kind of problem that the previous patch fixes. That is, to try to match
existing subfiles when different name forms are used to refer to a same
file.
The thread for the patch that introduced this code is:
https://pi.simark.ca/gdb-patches/45F8CBDF.9090501@hq.tensilica.com/
The important bits are that the compiler produced a compilation unit
with:
DW_AT_name : test.c
DW_AT_comp_dir : /home/maxim/W/BadgerPass/PR_14999
and DWARF v2 line table with:
The Directory Table:
/home/maxim/W/BadgerPass/PR_14999
The File Name Table:
Entry Dir Time Size Name
1 1 1173897037 152 test.c
Because the main symtab was created with only DW_AT_name, it was named
"test.c". And because the path built from the line header contained the
"directory" part, it was "/home/maxim/W/BadgerPass/PR_14999/test.c".
Because of this mismatch, thing didn't work, so they added this code to
prepend the compilation directory to the existing subfile names, so that
this specific case would work.
With the changes done earlier in this series, where subfiles are
identified using the "most complete path possible", this case would be
handled. The main subfile's would be
"/home/maxim/W/BadgerPass/PR_14999/test.c" from the start
(DW_AT_comp_dir + DW_AT_name). It's not so different from some DWARF 5
cases actually, which make the compilation directory explicit in the
line table header.
I therefore think that this code is no longer needed. It does feel like
a quick hack to make one specific case work, and we have a more general
solution now. Also, this code was introduced to work around a problem
in the DWARF debug info or the DWARF debug info reader. In general, I
think it's preferable for these hacks to be located in the specific
debug info reader code, rather than in the common code.
Even though this code was added to work around a DWARF reader problem,
it's possible that some other debug info reader has started taking
advantage of this code in the mean time. It's very difficult to
know or verify, but I think the likelyhood is quite small, so I'm
proposing to get rid of it to simplify things a little bit.
Change-Id: I710b8ec0d449d1b110d67ddf9fcbdb2b37108306
---
gdb/buildsym.c | 32 +++++++-------------------------
1 file changed, 7 insertions(+), 25 deletions(-)
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index bb258b983ca4..4672d937bd98 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -498,31 +498,13 @@ buildsym_compunit::start_subfile (const char *name, const char *name_for_id)
symtab_create_debug_printf ("name = %s, name_for_id = %s", name, name_for_id);
for (subfile *subfile = m_subfiles; subfile; subfile = subfile->next)
- {
- std::string subfile_name_holder;
- const char *subfile_name_for_id;
-
- /* If NAME is an absolute path, and this subfile is not, then
- attempt to create an absolute path to compare. */
- if (IS_ABSOLUTE_PATH (name_for_id)
- && !IS_ABSOLUTE_PATH (subfile->name_for_id)
- && !m_comp_dir.empty ())
- {
- subfile_name_holder = path_join (m_comp_dir.c_str (),
- subfile->name_for_id.c_str ());
- subfile_name_for_id = subfile_name_holder.c_str ();
- }
- else
- subfile_name_for_id = subfile->name_for_id.c_str ();
-
- if (FILENAME_CMP (subfile_name_for_id, name_for_id) == 0)
- {
- symtab_create_debug_printf ("found existing symtab with name_for_id %s (%s)",
- subfile->name_for_id.c_str (), subfile_name_for_id);
- m_current_subfile = subfile;
- return;
- }
- }
+ if (FILENAME_CMP (subfile->name_for_id.c_str (), name_for_id) == 0)
+ {
+ symtab_create_debug_printf ("found existing symtab with name_for_id %s",
+ subfile->name_for_id.c_str ());
+ m_current_subfile = subfile;
+ return;
+ }
/* This subfile is not known. Add an entry for it. */
--
2.35.2
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 6/7] gdb: remove code to prepend comp dir in buildsym_compunit::start_subfile
2022-04-28 3:35 ` [PATCH v3 6/7] gdb: remove code to prepend comp dir in buildsym_compunit::start_subfile Simon Marchi
@ 2022-05-12 13:07 ` Bruno Larsen
2022-07-28 17:47 ` Simon Marchi
0 siblings, 1 reply; 21+ messages in thread
From: Bruno Larsen @ 2022-05-12 13:07 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 4/28/22 00:35, Simon Marchi via Gdb-patches wrote:
> The bit of code removed by this patch was introduced to fix the same
> kind of problem that the previous patch fixes. That is, to try to match
> existing subfiles when different name forms are used to refer to a same
> file.
>
> The thread for the patch that introduced this code is:
>
> https://pi.simark.ca/gdb-patches/45F8CBDF.9090501@hq.tensilica.com/
>
> The important bits are that the compiler produced a compilation unit
> with:
>
> DW_AT_name : test.c
> DW_AT_comp_dir : /home/maxim/W/BadgerPass/PR_14999
>
> and DWARF v2 line table with:
>
> The Directory Table:
> /home/maxim/W/BadgerPass/PR_14999
>
> The File Name Table:
> Entry Dir Time Size Name
> 1 1 1173897037 152 test.c
>
> Because the main symtab was created with only DW_AT_name, it was named
> "test.c". And because the path built from the line header contained the
> "directory" part, it was "/home/maxim/W/BadgerPass/PR_14999/test.c".
> Because of this mismatch, thing didn't work, so they added this code to
> prepend the compilation directory to the existing subfile names, so that
> this specific case would work.
>
> With the changes done earlier in this series, where subfiles are
> identified using the "most complete path possible", this case would be
> handled. The main subfile's would be
> "/home/maxim/W/BadgerPass/PR_14999/test.c" from the start
> (DW_AT_comp_dir + DW_AT_name). It's not so different from some DWARF 5
> cases actually, which make the compilation directory explicit in the
> line table header.
>
> I therefore think that this code is no longer needed. It does feel like
> a quick hack to make one specific case work, and we have a more general
> solution now. Also, this code was introduced to work around a problem
> in the DWARF debug info or the DWARF debug info reader. In general, I
> think it's preferable for these hacks to be located in the specific
> debug info reader code, rather than in the common code.
>
> Even though this code was added to work around a DWARF reader problem,
> it's possible that some other debug info reader has started taking
> advantage of this code in the mean time. It's very difficult to
> know or verify, but I think the likelyhood is quite small, so I'm
> proposing to get rid of it to simplify things a little bit.
>
> Change-Id: I710b8ec0d449d1b110d67ddf9fcbdb2b37108306
Hello Simon,
This change seems reasonable, and local testing found no regressions. In fact, because this prepending code was not always applied, it caused some problems when testing using clang, and removing it has fixed failures in gdb.base/maint.exp and gdb.base/jit-elf.exp!
FWIW, I think you can approve your patch.
Cheers!
Bruno Larsen
> ---
> gdb/buildsym.c | 32 +++++++-------------------------
> 1 file changed, 7 insertions(+), 25 deletions(-)
>
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index bb258b983ca4..4672d937bd98 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -498,31 +498,13 @@ buildsym_compunit::start_subfile (const char *name, const char *name_for_id)
> symtab_create_debug_printf ("name = %s, name_for_id = %s", name, name_for_id);
>
> for (subfile *subfile = m_subfiles; subfile; subfile = subfile->next)
> - {
> - std::string subfile_name_holder;
> - const char *subfile_name_for_id;
> -
> - /* If NAME is an absolute path, and this subfile is not, then
> - attempt to create an absolute path to compare. */
> - if (IS_ABSOLUTE_PATH (name_for_id)
> - && !IS_ABSOLUTE_PATH (subfile->name_for_id)
> - && !m_comp_dir.empty ())
> - {
> - subfile_name_holder = path_join (m_comp_dir.c_str (),
> - subfile->name_for_id.c_str ());
> - subfile_name_for_id = subfile_name_holder.c_str ();
> - }
> - else
> - subfile_name_for_id = subfile->name_for_id.c_str ();
> -
> - if (FILENAME_CMP (subfile_name_for_id, name_for_id) == 0)
> - {
> - symtab_create_debug_printf ("found existing symtab with name_for_id %s (%s)",
> - subfile->name_for_id.c_str (), subfile_name_for_id);
> - m_current_subfile = subfile;
> - return;
> - }
> - }
> + if (FILENAME_CMP (subfile->name_for_id.c_str (), name_for_id) == 0)
> + {
> + symtab_create_debug_printf ("found existing symtab with name_for_id %s",
> + subfile->name_for_id.c_str ());
> + m_current_subfile = subfile;
> + return;
> + }
>
> /* This subfile is not known. Add an entry for it. */
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 6/7] gdb: remove code to prepend comp dir in buildsym_compunit::start_subfile
2022-05-12 13:07 ` Bruno Larsen
@ 2022-07-28 17:47 ` Simon Marchi
0 siblings, 0 replies; 21+ messages in thread
From: Simon Marchi @ 2022-07-28 17:47 UTC (permalink / raw)
To: Bruno Larsen, gdb-patches
> Hello Simon,
>
> This change seems reasonable, and local testing found no regressions.
> In fact, because this prepending code was not always applied, it
> caused some problems when testing using clang, and removing it has
> fixed failures in gdb.base/maint.exp and gdb.base/jit-elf.exp!
Well, that's good news, glad to hear that!
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 7/7] gdb/testsuite: add macros test for source files compiled in various ways
2022-04-28 3:35 [PATCH v3 0/7] Fix printing macros Simon Marchi
` (5 preceding siblings ...)
2022-04-28 3:35 ` [PATCH v3 6/7] gdb: remove code to prepend comp dir in buildsym_compunit::start_subfile Simon Marchi
@ 2022-04-28 3:35 ` Simon Marchi
2022-05-12 13:17 ` Bruno Larsen
2022-07-30 0:56 ` [PATCH v3 0/7] Fix printing macros Simon Marchi
7 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2022-04-28 3:35 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@efficios.com>
Using different ways of passing source file paths to compilers results n
different file and directory paths in the line header. For example:
- gcc foo.c
- gcc ./foo.c
- gcc ../cwd/foo.c
- gcc $PWD/foo.c
Because of this, GDB sometimes failed to look up macros. The previous
patch fixed that as much as possible. This patch adds the corresponding
tests.
Add both a DWARF assembler-based test and a regular test. The DWARF
assembled-based one tests some hard-coded debug info based on what I
have observed some specific versions of gcc and clang generate. We want
to make sure that GDB keeps handling all these cases correctly, even if
it's not always clear whether they are really valid DWARF. Also, they
will be tested no matter what the current target compiler is for a given
test run.
The regular test is compiled using the target compiler, so it may help
find bugs when testing against some other toolchains than what was used
to generate the DWARF assembler-based test.
For the DWARF assembler-based test, add to testsuite/lib/dwarf.exp the
necessary code to generate a DWARF5 .debug_macro section. The design of
the new procs is based on what was done for rnglists and loclists.
To test against a specific compiler one can use this command, for
example:
$ make check TESTS="gdb.base/macro-source-path.exp" RUNTESTFLAGS="CC_FOR_TARGET=clang --target_board unix/gdb:debug_flags=-gdwarf-5"
Change-Id: Iab8da498e57d10cc2a3d09ea136685d9278cfcf6
---
gdb/testsuite/gdb.base/macro-source-path.c | 22 +
gdb/testsuite/gdb.base/macro-source-path.exp | 87 ++++
gdb/testsuite/gdb.dwarf2/macro-source-path.c | 20 +
.../gdb.dwarf2/macro-source-path.exp | 393 ++++++++++++++++++
gdb/testsuite/lib/dwarf.exp | 92 ++++
5 files changed, 614 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/macro-source-path.c
create mode 100644 gdb/testsuite/gdb.base/macro-source-path.exp
create mode 100644 gdb/testsuite/gdb.dwarf2/macro-source-path.c
create mode 100644 gdb/testsuite/gdb.dwarf2/macro-source-path.exp
diff --git a/gdb/testsuite/gdb.base/macro-source-path.c b/gdb/testsuite/gdb.base/macro-source-path.c
new file mode 100644
index 000000000000..f4ede117a2a0
--- /dev/null
+++ b/gdb/testsuite/gdb.base/macro-source-path.c
@@ -0,0 +1,22 @@
+/* Copyright 2022 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#define TWO 2
+
+int
+main (void)
+{
+ return ONE + TWO;
+}
diff --git a/gdb/testsuite/gdb.base/macro-source-path.exp b/gdb/testsuite/gdb.base/macro-source-path.exp
new file mode 100644
index 000000000000..edbb4aee8e98
--- /dev/null
+++ b/gdb/testsuite/gdb.base/macro-source-path.exp
@@ -0,0 +1,87 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Compile a source file using different ways of passing the path to the
+# compiler. Then, verify that we can print a macro defined in that file.
+
+standard_testfile
+
+# If the host is remote, source files are uploaded to the host and compiled
+# there, but without the directory structure we expect, making the test
+# pointless. Skip the test in that case.
+if { [is_remote host] } {
+ return
+}
+
+# Copy the source file at these locations in the output directory ($out):
+#
+# $out/cwd/macro-source-path.c
+# $out/other/macro-source-path.c
+#
+# Set the current working directory to $out/cwd, so that we can test compiling
+# using relative paths.
+
+set out_dir [standard_output_file ""]
+file mkdir $out_dir/cwd
+file mkdir $out_dir/other
+file copy -force $srcdir/$subdir/$srcfile $out_dir/cwd
+file copy -force $srcdir/$subdir/$srcfile $out_dir/other
+
+# Run one test.
+#
+# SRC is the path to the source file, to be passed to the compiler as-is.
+# NAME is the name of the test.
+
+proc test { src name } {
+ with_test_prefix $name {
+ set binfile $::out_dir/$name
+
+ if { [gdb_compile $src $binfile executable {debug macros additional_flags=-DONE=1}] != "" } {
+ fail "could not compile"
+ return
+ }
+
+ clean_restart $binfile
+
+ if { ![runto_main] } {
+ return
+ }
+
+ # Print the macro that is defined on the command-line.
+ if { [test_compiler_info "clang-*"] } {
+ # This is really a clang bug, it puts the macros defined on the command
+ # line after the main source file, in the macro table.
+ setup_kfail "gdb/29034" "*-*-*"
+ }
+ gdb_test "print ONE" " = 1"
+
+ # Print the macro that is defined in the main file.
+ gdb_test "print TWO" " = 2"
+ }
+}
+
+# When adding a test here, please consider adding an equivalent case to the test
+# of the same name in gdb.dwarf2.
+
+with_cwd "$out_dir/cwd" {
+ test $testfile.c filename
+ test ./$testfile.c dot-filename
+ test ../cwd/$testfile.c dot-dot-filename
+ test [file normalize $testfile.c] absolute-cwd
+ test ../other/$testfile.c dot-dot-other
+ test [file normalize ../other/$testfile.c] absolute-other
+}
diff --git a/gdb/testsuite/gdb.dwarf2/macro-source-path.c b/gdb/testsuite/gdb.dwarf2/macro-source-path.c
new file mode 100644
index 000000000000..749bcc1580ee
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/macro-source-path.c
@@ -0,0 +1,20 @@
+/* Copyright 2022 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+int
+main (void)
+{
+ asm ("main_label: .globl main_label");
+}
diff --git a/gdb/testsuite/gdb.dwarf2/macro-source-path.exp b/gdb/testsuite/gdb.dwarf2/macro-source-path.exp
new file mode 100644
index 000000000000..284bb4d813b6
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/macro-source-path.exp
@@ -0,0 +1,393 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Generate binaries imitating different ways source file paths can be passed to
+# compilers. Test printing macros from those binaries.
+
+load_lib dwarf.exp
+
+if {![dwarf2_support]} {
+ return 0
+}
+
+standard_testfile .c
+
+lassign [function_range main $srcdir/$subdir/$srcfile] \
+ main_start main_len
+
+# Run one test.
+#
+# - TEST_NAME is the name of the test, used to differentiate the binaries.
+# - LINES_VERSION is the version of the version of the .debug_line section to
+# generate.
+# - DW_AT_NAME is the string to put in the compilation unit's DW_AT_name
+# attribute.
+# - MAIN_FILE_IDX is the file index the .debug_line and .debug_macro sections
+# will use to refer to the main file.
+# - DIRECTORIES is a list of directories to put in the .debug_line section
+# header
+# - FILE_NAMES is a list of {name, directory index} pairs describing the files
+# names to put in the .debug_line section header.
+
+proc do_test { test_name lines_version DW_AT_name main_file_idx directories
+ file_names } {
+ with_test_prefix "test_name=$test_name" {
+ foreach_with_prefix is_64 {true false} {
+ # So we can access them in Dwarf::assemble...
+ set ::lines_version $lines_version
+ set ::DW_AT_name $DW_AT_name
+ set ::main_file_idx $main_file_idx
+ set ::directories $directories
+ set ::file_names $file_names
+ set ::is_64 $is_64
+ set 32_or_64 [expr $is_64 ? 64 : 32]
+
+ set asm_file [standard_output_file ${::testfile}-${test_name}-${32_or_64}.S]
+ Dwarf::assemble $asm_file {
+ declare_labels Llines cu_macros
+
+ # DW_AT_comp_dir is always the current working directory
+ # from which the compiler was invoked. We pretend the compiler was
+ # always launched from /tmp/cwd.
+ set comp_dir "/tmp/cwd"
+
+ cu {} {
+ DW_TAG_compile_unit {
+ {DW_AT_producer "My C Compiler"}
+ {DW_AT_language @DW_LANG_C11}
+ {DW_AT_name $::DW_AT_name}
+ {DW_AT_comp_dir $comp_dir}
+ {DW_AT_stmt_list $Llines DW_FORM_sec_offset}
+ {DW_AT_macros $cu_macros DW_FORM_sec_offset}
+ } {
+ declare_labels int_type
+
+ int_type: DW_TAG_base_type {
+ {DW_AT_byte_size 4 DW_FORM_sdata}
+ {DW_AT_encoding @DW_ATE_signed}
+ {DW_AT_name int}
+ }
+
+ DW_TAG_subprogram {
+ {MACRO_AT_func {main}}
+ {type :$int_type}
+ }
+ }
+ }
+
+ # Define the .debug_line section.
+ lines [list version $::lines_version] "Llines" {
+ foreach directory $::directories {
+ include_dir $directory
+ }
+
+ foreach file_name $::file_names {
+ lassign $file_name name dir_index
+ file_name $name $dir_index
+ }
+
+ # A line number program just good enough so that GDB can
+ # figure out we are stopped in main.
+ program {
+ DW_LNS_set_file $::main_file_idx
+ DW_LNE_set_address $::main_start
+ line 10
+ DW_LNS_copy
+
+ DW_LNE_set_address "$::main_start + $::main_len"
+ DW_LNE_end_sequence
+ }
+ }
+
+ # Define the .debug_macro section.
+ macro {
+ cu_macros: unit {
+ "debug-line-offset-label" $Llines
+ "is-64" $::is_64
+ } {
+ # A macro defined outside the main file, as if it was defined
+ # on the command line with -D.
+ #
+ # Clang has this bug where it puts the macros defined on
+ # the command-line after the main file portion (see
+ # PR 29034). We're not trying to replicate that here,
+ # this is not in the scope of this test.
+ define 0 "ONE 1"
+ start_file 0 $::main_file_idx
+ # A macro defined at line 1 of the main file.
+ define 1 "TWO 2"
+ end_file
+ }
+ }
+ }
+
+ if { [prepare_for_testing "failed to prepare" ${::testfile}-${test_name}-${32_or_64} \
+ [list $::srcfile $asm_file] {nodebug}] } {
+ return
+ }
+
+ if ![runto_main] {
+ return
+ }
+
+ gdb_test "print ONE" " = 1"
+ gdb_test "print TWO" " = 2"
+ }
+ }
+}
+
+# When adding a test here, please consider adding an equivalent case to the test
+# of the same name in gdb.base.
+
+# Based on `gcc -gdwarf-5 -g3 <file>`, gcc 11 paired with gas from binutils 2.38.
+
+## test.c
+do_test gcc11-ld238-dw5-filename 5 "test.c" 1 {
+ "/tmp/cwd"
+} {
+ {"test.c" 0}
+ {"test.c" 0}
+}
+
+## ./test.c
+do_test gcc11-ld238-dw5-dot-filename 5 "./test.c" 1 {
+ "/tmp/cwd"
+ "."
+} {
+ {"test.c" 1}
+ {"test.c" 1}
+}
+
+## ../cwd/test.c
+do_test gcc11-ld238-dw5-dot-dot-cwd 5 "../cwd/test.c" 1 {
+ "/tmp/cwd"
+ "../cwd"
+} {
+ {"test.c" 1}
+ {"test.c" 1}
+}
+
+## /tmp/cwd/test.c
+do_test gcc11-ld238-dw5-absolute-cwd 5 "/tmp/cwd/test.c" 1 {
+ "/tmp/cwd"
+ "/tmp/cwd"
+} {
+ {"test.c" 1}
+ {"test.c" 0}
+}
+
+## ../other/test.c
+do_test gcc11-ld238-dw5-dot-dot-other 5 "../other/test.c" 1 {
+ "/tmp/cwd"
+ "../other"
+} {
+ {"test.c" 1}
+ {"test.c" 1}
+}
+
+## /tmp/other/test.c
+do_test gcc11-ld238-dw5-absolute-other 5 "/tmp/other/test.c" 1 {
+ "/tmp/cwd"
+ "/tmp/other"
+} {
+ {"test.c" 1}
+ {"test.c" 1}
+}
+
+# Based on `gcc -gdwarf-4 -g3 <file>`, gcc 11 paired with gas from binutils
+# 2.38. With -gdwarf-4, gcc generates a v4 (pre-standard) .debug_macro section.
+
+## test.c
+do_test gcc11-ld238-dw4-filename 4 "test.c" 1 {
+} {
+ {"test.c" 0}
+}
+
+## ./test.c
+do_test gcc11-ld238-dw4-dot-filename 4 "./test.c" 1 {
+ "."
+} {
+ {"test.c" 1}
+}
+
+## ../cwd/test.c
+do_test gcc11-ld238-dw4-dot-dot-cwd 4 "../cwd/test.c" 1 {
+ "../cwd"
+} {
+ {"test.c" 1}
+}
+
+## /tmp/cwd/test.c
+do_test gcc11-ld238-dw4-absolute-cwd 4 "/tmp/cwd/test.c" 1 {
+ "/tmp/cwd"
+} {
+ {"test.c" 1}
+}
+
+## ../other/test.c
+do_test gcc11-ld238-dw4-dot-dot-other 4 "../other/test.c" 1 {
+ "../other"
+} {
+ {"test.c" 1}
+}
+
+## /tmp/other/test.c
+do_test gcc11-ld238-dw4-absolute-other 4 "/tmp/other/test.c" 1 {
+ "/tmp/other"
+} {
+ {"test.c" 1}
+}
+
+# Based on `clang-14 -gdwarf-5 -fdebug-macro -g3 <file>` (using its
+# built-in assembler)
+
+## test.c
+do_test clang14-dw5-filename 5 "test.c" 0 {
+ "/tmp/cwd"
+} {
+ {"test.c" 0}
+}
+
+## ./test.c
+do_test clang14-dw5-dot-filename 5 "test.c" 1 {
+ "/tmp/cwd"
+ "."
+} {
+ {"test.c" 0}
+ {"test.c" 1}
+}
+
+## ../cwd/test.c
+do_test clang14-dw5-dot-dot-cwd 5 "../cwd/test.c" 0 {
+ "/tmp/cwd"
+} {
+ {"../cwd/test.c" 0}
+}
+
+## /tmp/cwd/test.c
+do_test clang14-dw5-absolute-cwd 5 "/tmp/cwd/test.c" 1 {
+ "/tmp/cwd"
+} {
+ {"/tmp/cwd/test.c" 0}
+ {"test.c" 0}
+}
+
+## ../other/test.c
+do_test clang14-dw5-dot-dot-other 5 "../other/test.c" 0 {
+ "/tmp/cwd"
+} {
+ {"../other/test.c" 0}
+}
+
+## /tmp/other/test.c
+do_test clang14-dw5-absolute-other 5 "/tmp/other/test.c" 1 {
+ "/tmp/cwd"
+ "/tmp"
+} {
+ {"/tmp/other/test.c" 0}
+ {"other/test.c" 1}
+}
+
+# Based on `clang-14 -gdwarf-4 -fdebug-macro -g3 <file>` (using its built-in
+# assembler). With -gdwarf-4, clang produces a .debug_macinfo section, not a
+# .debug_macro section. But this test still creates a .debug_macro section,
+# that's good enough for what we want to test.
+
+## test.c
+do_test clang14-dw4-filename 4 "test.c" 1 {
+} {
+ {"test.c" 0}
+}
+
+## ./test.c
+do_test clang14-dw4-dot-filename 4 "test.c" 1 {
+ "."
+} {
+ {"test.c" 1}
+}
+
+## ../cwd/test.c
+do_test clang14-dw4-dot-dot-cwd 4 "../cwd/test.c" 1 {
+ "../cwd"
+} {
+ {"test.c" 1}
+}
+
+## /tmp/cwd/test.c
+do_test clang14-dw4-absolute-cwd 4 "/tmp/cwd/test.c" 1 {
+} {
+ {"test.c" 0}
+}
+
+## ../other/test.c
+do_test clang14-dw4-dot-dot-other 4 "../other/test.c" 1 {
+ "../other"
+} {
+ {"test.c" 1}
+}
+
+## /tmp/other/test.c
+do_test clang14-dw4-absolute-other 4 "/tmp/other/test.c" 1 {
+ "/tmp"
+} {
+ {"other/test.c" 1}
+}
+
+# Based on `gcc -gdwarf-5 -g3 <file>`, gcc 11 paired with gas from binutils
+# 2.34 (Ubuntu 20.04). It generates a v5 .debug_macro section, but a v3
+# .debug_line section.
+
+## test.c
+do_test gcc11-ld234-dw5-filename 3 "test.c" 1 {
+} {
+ {"test.c" 0}
+}
+
+## ./test.c
+do_test gcc11-ld234-dw5-dot-filename 3 "./test.c" 1 {
+ "."
+} {
+ {"test.c" 1}
+}
+
+## ../cwd/test.c
+do_test gcc11-ld234-dw5-dot-dot-cwd 3 "../cwd/test.c" 1 {
+ "../cwd"
+} {
+ {"test.c" 1}
+}
+
+## /tmp/cwd/test.c
+do_test gcc11-ld234-dw5-absolute-cwd 3 "/tmp/cwd/test.c" 1 {
+ "/tmp/cwd"
+} {
+ {"test.c" 1}
+}
+
+## ../other/test.c
+do_test gcc11-ld234-dw5-dot-dot-other 3 "../other/test.c" 1 {
+ "../other"
+} {
+ {"test.c" 1}
+}
+
+## /tmp/other/test.c
+do_test gcc11-ld234-dw5-absolute-other 3 "/tmp/other/test.c" 1 {
+ "/tmp/other"
+} {
+ {"test.c" 1}
+}
diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index 55e97c33a6e1..fb62e3ca038d 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -2142,6 +2142,98 @@ namespace eval Dwarf {
incr _debug_loclists_locdesc_count
}
+ # Emit a DWARF .debug_macro section.
+ #
+ # BODY must be Tcl code that emits the content of the section. It is
+ # evaluated in the caller's context. The body can use the `unit` proc
+ # (see `_macro_unit`) to generate macro units.
+
+ proc macro { body } {
+ _section ".debug_macro"
+
+ with_override Dwarf::unit Dwarf::_macro_unit {
+ uplevel $body
+ }
+ }
+
+ # Generate one macro unit.
+ #
+ # This proc is meant to be used within proc macro's body. It is made
+ # available as `unit` while inside proc macro's body.
+ #
+ # BODY must be Tcl code that emits the content of the unit. It may call
+ # procedures defined below, prefixed with `_macro_unit_`, to generate the
+ # unit's content. It is evaluated in the caller's context.
+ #
+ # The `is-64 true|false` options tells whether to use 64-bit DWARF instead
+ # of 32-bit DWARF. The default is 32-bit.
+ #
+ # If specified, the `debug-line-offset-label` option is the name of a label
+ # to use for the unit header's `debug_line_offset` field value. If
+ # omitted, the unit header will not contain the `debug_line_offset` field.
+
+ proc _macro_unit { options body } {
+ parse_options {
+ {"is-64" "false"}
+ {"debug-line-offset-label" ""}
+ }
+
+ _op .2byte 5 "version"
+
+ # Flags:
+ #
+ # offset_size_flag = set if is-64 is true
+ # debug_line_offset_flag = set if debug-line-offset-label is set
+ # opcode_operands_table_flag = 0
+ set flags 0
+
+ if { ${is-64} } {
+ set flags [expr $flags | 0x1]
+ }
+
+ if { ${debug-line-offset-label} != "" } {
+ set flags [expr $flags | 0x2]
+ }
+
+ _op .byte $flags "flags"
+
+ if { ${debug-line-offset-label} != "" } {
+ if { ${is-64} } {
+ _op .8byte ${debug-line-offset-label} "debug_line offset"
+ } else {
+ _op .4byte ${debug-line-offset-label} "debug_line offset"
+ }
+ }
+
+ with_override Dwarf::define Dwarf::_macro_unit_define {
+ with_override Dwarf::start_file Dwarf::_macro_unit_start_file {
+ with_override Dwarf::end_file Dwarf::_macro_unit_end_file {
+ uplevel $body
+ }}}
+ }
+
+ # Emit a DW_MACRO_define entry.
+
+ proc _macro_unit_define { lineno text } {
+ _op .byte 0x1 "DW_MACRO_define"
+ _op .uleb128 $lineno "Line number"
+ _op .asciz "\"$text\"" "Macro definition"
+ }
+
+ # Emit a DW_MACRO_start_file entry.
+
+ proc _macro_unit_start_file { lineno file_idx } {
+ _op .byte 0x3 "DW_MACRO_start_file"
+ _op .uleb128 $lineno
+ _op .uleb128 $file_idx
+ }
+
+ # Emit a DW_MACRO_end_file entry.
+
+ proc _macro_unit_end_file {} {
+ _op .byte 0x4 "DW_MACRO_end_file"
+ }
+
# Emit a DWARF .debug_line unit.
# OPTIONS is a list with an even number of elements containing
# option-name and option-value pairs.
--
2.35.2
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 7/7] gdb/testsuite: add macros test for source files compiled in various ways
2022-04-28 3:35 ` [PATCH v3 7/7] gdb/testsuite: add macros test for source files compiled in various ways Simon Marchi
@ 2022-05-12 13:17 ` Bruno Larsen
2022-07-28 17:51 ` Simon Marchi
0 siblings, 1 reply; 21+ messages in thread
From: Bruno Larsen @ 2022-05-12 13:17 UTC (permalink / raw)
To: Simon Marchi, gdb-patches; +Cc: Simon Marchi
On 4/28/22 00:35, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> Using different ways of passing source file paths to compilers results n
> different file and directory paths in the line header. For example:
>
> - gcc foo.c
> - gcc ./foo.c
> - gcc ../cwd/foo.c
> - gcc $PWD/foo.c
>
> Because of this, GDB sometimes failed to look up macros. The previous
> patch fixed that as much as possible. This patch adds the corresponding
> tests.
>
> Add both a DWARF assembler-based test and a regular test. The DWARF
> assembled-based one tests some hard-coded debug info based on what I
> have observed some specific versions of gcc and clang generate. We want
> to make sure that GDB keeps handling all these cases correctly, even if
> it's not always clear whether they are really valid DWARF. Also, they
> will be tested no matter what the current target compiler is for a given
> test run.
>
> The regular test is compiled using the target compiler, so it may help
> find bugs when testing against some other toolchains than what was used
> to generate the DWARF assembler-based test.
>
> For the DWARF assembler-based test, add to testsuite/lib/dwarf.exp the
> necessary code to generate a DWARF5 .debug_macro section. The design of
> the new procs is based on what was done for rnglists and loclists.
>
> To test against a specific compiler one can use this command, for
> example:
>
> $ make check TESTS="gdb.base/macro-source-path.exp" RUNTESTFLAGS="CC_FOR_TARGET=clang --target_board unix/gdb:debug_flags=-gdwarf-5"
>
> Change-Id: Iab8da498e57d10cc2a3d09ea136685d9278cfcf6
Hello Simon!
Thanks for these tests! I am not a TCL expert, so I am limited in what I can comment on this patch, but I do have a comment below. Other than that, the patch looks alright to me, FWIW.
> ---
> gdb/testsuite/gdb.base/macro-source-path.c | 22 +
> gdb/testsuite/gdb.base/macro-source-path.exp | 87 ++++
> gdb/testsuite/gdb.dwarf2/macro-source-path.c | 20 +
> .../gdb.dwarf2/macro-source-path.exp | 393 ++++++++++++++++++
> gdb/testsuite/lib/dwarf.exp | 92 ++++
> 5 files changed, 614 insertions(+)
> create mode 100644 gdb/testsuite/gdb.base/macro-source-path.c
> create mode 100644 gdb/testsuite/gdb.base/macro-source-path.exp
> create mode 100644 gdb/testsuite/gdb.dwarf2/macro-source-path.c
> create mode 100644 gdb/testsuite/gdb.dwarf2/macro-source-path.exp
>
<snip>
> diff --git a/gdb/testsuite/gdb.dwarf2/macro-source-path.exp b/gdb/testsuite/gdb.dwarf2/macro-source-path.exp
> new file mode 100644
> index 000000000000..284bb4d813b6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/macro-source-path.exp
> @@ -0,0 +1,393 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +# Generate binaries imitating different ways source file paths can be passed to
> +# compilers. Test printing macros from those binaries.
> +
> +load_lib dwarf.exp
> +
> +if {![dwarf2_support]} {
> + return 0
> +}
> +
> +standard_testfile .c
> +
> +lassign [function_range main $srcdir/$subdir/$srcfile] \
> + main_start main_len
> +
> +# Run one test.
> +#
> +# - TEST_NAME is the name of the test, used to differentiate the binaries.
> +# - LINES_VERSION is the version of the version of the .debug_line section to
> +# generate.
> +# - DW_AT_NAME is the string to put in the compilation unit's DW_AT_name
> +# attribute.
> +# - MAIN_FILE_IDX is the file index the .debug_line and .debug_macro sections
> +# will use to refer to the main file.
> +# - DIRECTORIES is a list of directories to put in the .debug_line section
> +# header
> +# - FILE_NAMES is a list of {name, directory index} pairs describing the files
> +# names to put in the .debug_line section header.
> +
> +proc do_test { test_name lines_version DW_AT_name main_file_idx directories
> + file_names } {
> + with_test_prefix "test_name=$test_name" {
> + foreach_with_prefix is_64 {true false} {
> + # So we can access them in Dwarf::assemble...
> + set ::lines_version $lines_version
> + set ::DW_AT_name $DW_AT_name
> + set ::main_file_idx $main_file_idx
> + set ::directories $directories
> + set ::file_names $file_names
> + set ::is_64 $is_64
> + set 32_or_64 [expr $is_64 ? 64 : 32]
> +
> + set asm_file [standard_output_file ${::testfile}-${test_name}-${32_or_64}.S]
> + Dwarf::assemble $asm_file {
> + declare_labels Llines cu_macros
> +
> + # DW_AT_comp_dir is always the current working directory
> + # from which the compiler was invoked. We pretend the compiler was
> + # always launched from /tmp/cwd.
> + set comp_dir "/tmp/cwd"
> +
> + cu {} {
> + DW_TAG_compile_unit {
> + {DW_AT_producer "My C Compiler"}
> + {DW_AT_language @DW_LANG_C11}
> + {DW_AT_name $::DW_AT_name}
> + {DW_AT_comp_dir $comp_dir}
> + {DW_AT_stmt_list $Llines DW_FORM_sec_offset}
> + {DW_AT_macros $cu_macros DW_FORM_sec_offset}
> + } {
> + declare_labels int_type
> +
> + int_type: DW_TAG_base_type {
> + {DW_AT_byte_size 4 DW_FORM_sdata}
> + {DW_AT_encoding @DW_ATE_signed}
> + {DW_AT_name int}
> + }
> +
> + DW_TAG_subprogram {
> + {MACRO_AT_func {main}}
> + {type :$int_type}
> + }
> + }
> + }
> +
> + # Define the .debug_line section.
> + lines [list version $::lines_version] "Llines" {
> + foreach directory $::directories {
> + include_dir $directory
> + }
> +
> + foreach file_name $::file_names {
> + lassign $file_name name dir_index
> + file_name $name $dir_index
> + }
> +
> + # A line number program just good enough so that GDB can
> + # figure out we are stopped in main.
> + program {
> + DW_LNS_set_file $::main_file_idx
> + DW_LNE_set_address $::main_start
> + line 10
> + DW_LNS_copy
> +
> + DW_LNE_set_address "$::main_start + $::main_len"
> + DW_LNE_end_sequence
> + }
> + }
> +
> + # Define the .debug_macro section.
> + macro {
> + cu_macros: unit {
> + "debug-line-offset-label" $Llines
> + "is-64" $::is_64
> + } {
> + # A macro defined outside the main file, as if it was defined
> + # on the command line with -D.
> + #
> + # Clang has this bug where it puts the macros defined on
> + # the command-line after the main file portion (see
> + # PR 29034). We're not trying to replicate that here,
> + # this is not in the scope of this test.
> + define 0 "ONE 1"
> + start_file 0 $::main_file_idx
> + # A macro defined at line 1 of the main file.
> + define 1 "TWO 2"
> + end_file
> + }
> + }
> + }
> +
> + if { [prepare_for_testing "failed to prepare" ${::testfile}-${test_name}-${32_or_64} \
> + [list $::srcfile $asm_file] {nodebug}] } {
> + return
> + }
> +
> + if ![runto_main] {
> + return
> + }
> +
> + gdb_test "print ONE" " = 1"
> + gdb_test "print TWO" " = 2"
> + }
> + }
> +}
> +
> +# When adding a test here, please consider adding an equivalent case to the test
> +# of the same name in gdb.base.
> +
> +# Based on `gcc -gdwarf-5 -g3 <file>`, gcc 11 paired with gas from binutils 2.38.
This comment wasn't really clear to me. Maybe something like "The following calls are based on ..." or something to make clear that this comment refers to a group of do_test calls below, would avoid possible confusion.
Cheers!
Bruno Larsen
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 7/7] gdb/testsuite: add macros test for source files compiled in various ways
2022-05-12 13:17 ` Bruno Larsen
@ 2022-07-28 17:51 ` Simon Marchi
0 siblings, 0 replies; 21+ messages in thread
From: Simon Marchi @ 2022-07-28 17:51 UTC (permalink / raw)
To: Bruno Larsen, gdb-patches; +Cc: Simon Marchi
>> +# Based on `gcc -gdwarf-5 -g3 <file>`, gcc 11 paired with gas from binutils 2.38.
>
> This comment wasn't really clear to me. Maybe something like "The
> following calls are based on ..." or something to make clear that this
> comment refers to a group of do_test calls below, would avoid possible
> confusion.
Done. I changed them to "The following tests are based on the output
of...".
Thanks,
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/7] Fix printing macros
2022-04-28 3:35 [PATCH v3 0/7] Fix printing macros Simon Marchi
` (6 preceding siblings ...)
2022-04-28 3:35 ` [PATCH v3 7/7] gdb/testsuite: add macros test for source files compiled in various ways Simon Marchi
@ 2022-07-30 0:56 ` Simon Marchi
7 siblings, 0 replies; 21+ messages in thread
From: Simon Marchi @ 2022-07-30 0:56 UTC (permalink / raw)
To: gdb-patches
On 2022-04-27 23:35, Simon Marchi wrote:
> Hi,
>
> This is v3 of
>
> https://sourceware.org/pipermail/gdb-patches/2022-April/188230.html
>
> This new attempt uses an approach similar to v2 (use complete paths to
> identify symtabs/subfiles and macro_source_files). But instead
> modifying the existing filename/name fields of symtab and subfile, and
> therefore causing important UI changes, this new version adds the
> information in a new field.
>
> Patches 3 and 4 are preparatory. Patch 6 removes some code that I
> believe is made obsolete by patch 5.
I pushed this series, with a slightly modified patch 5. I'll reply
to that one with more details.
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread