[PATCH v2 0/6] Fix printing macros

[PATCH v2 0/6] Fix printing macros

[Simon Marchi]

This is the second version of this series:

  https://sourceware.org/pipermail/gdb-patches/2022-April/187550.html

The first 4 patches of that series were merged, but 4 new preparatory
patches appeared :).

The main difference in the main patch (patch 5) is that rather than
doing a change in buildsym_compunit, which could unexpectedly affect
other debug readers, the changes are now localized in the DWARF reader.

Another reason for this change is strategy is that the main subfile name
(that comes from the CU's DW_AT_comp_dir + DW_AT_name) became different
from the name the DWARF reader passed to start_subfile when referring to
the main file (which came from the line header) when parsing the line
table.

Things still worked, but only because it was caught by the heuristic in
watch_main_source_file_lossage.  With some of the debug output added by
the current series, it would give this:

    $ ./gdb -q --data-directory=data-directory -nx -readnow -iex "set debug symtab-create 1" a.out
    Reading symbols from a.out...
    [symtab-create] elf_read_minimal_symbols: reading minimal symbols of objfile /home/smarchi/build/binutils-gdb/gdb/a.out
    [symtab-create] install: installing 32 minimal symbols of objfile /home/smarchi/build/binutils-gdb/gdb/a.out
    [symtab-create] elf_read_minimal_symbols: done reading minimal symbols
    Expanding full symbols from a.out...
    [symtab-create] start_subfile: name = /home/smarchi/build/binutils-gdb/gdb/test.c <--- comes from CU's DW_AT_name + DW_AT_comp_dir
    [symtab-create] start_subfile: name = test.c <--- comes from line table
    [symtab-create] start_subfile: name = test.c
    [symtab-create] start_subfile: found existing symtab with name test.c (test.c)
    [symtab-create] start_subfile: name = <built-in>
    [symtab-create] start_subfile: name = /usr/include/stdc-predef.h
    [symtab-create] watch_main_source_file_lossage: using subfile test.c as the main subfile

watch_main_source_file_lossage detects that the main subfile is empty
(has no symbols, no line entries), and sees that there is another
subfile that has the same basename as the main subfile.  So it assumes
that this is really the main subfile, just under another name, and
makes that one the main subfile.  That's perhaps an ok last resort
heuristic, but I'd rather not voluntarily rely on that.  Who knows when
it will get it wrong.

With the new approach implemented in patch 5, we get:

    $ ./gdb -q --data-directory=data-directory -nx -readnow -iex "set debug symtab-create 1" a.out
    Reading symbols from a.out...
    [symtab-create] elf_read_minimal_symbols: reading minimal symbols of objfile /home/smarchi/build/binutils-gdb/gdb/a.out
    [symtab-create] install: installing 32 minimal symbols of objfile /home/smarchi/build/binutils-gdb/gdb/a.out
    [symtab-create] elf_read_minimal_symbols: done reading minimal symbols
    Expanding full symbols from a.out...
    [symtab-create] start_subfile: name = /home/smarchi/build/binutils-gdb/gdb/test.c
    [symtab-create] start_subfile: name = /home/smarchi/build/binutils-gdb/gdb/test.c
    [symtab-create] start_subfile: found existing symtab with name /home/smarchi/build/binutils-gdb/gdb/test.c (/home/smarchi/build/binutils-gdb/gdb/test.c)
    [symtab-create] start_subfile: name = /home/smarchi/build/binutils-gdb/gdb/test.c
    [symtab-create] start_subfile: found existing symtab with name /home/smarchi/build/binutils-gdb/gdb/test.c (/home/smarchi/build/binutils-gdb/gdb/test.c)
    [symtab-create] start_subfile: name = /home/smarchi/build/binutils-gdb/gdb/<built-in>
    [symtab-create] start_subfile: name = /usr/include/stdc-predef.h

All calls to start_subfile for the main file are passed the same path.

Simon Marchi (6):
  gdb: introduce symtab_create_debug_printf
  gdb: add debug prints in buildsym.c
  gdb/dwarf: add constructor to line_header
  gdb/dwarf: pass compilation directory to line header, initialize
    dir[0] for DWARF 4
  gdb/dwarf: use complete paths for symtabs and macro_source_files
  gdb/testsuite: add macros test for source files compiled in various
    ways

 gdb/buildsym.c                                |   7 +
 gdb/dwarf2/cu.c                               |  11 +
 gdb/dwarf2/line-header.c                      | 133 ++++----
 gdb/dwarf2/line-header.h                      |  76 +++--
 gdb/dwarf2/read.c                             |  85 ++---
 gdb/elfread.c                                 |  15 +-
 gdb/minsyms.c                                 |  16 +-
 gdb/psymtab.c                                 |  14 +-
 gdb/symfile.c                                 |  22 +-
 gdb/symtab.h                                  |  11 +
 gdb/testsuite/gdb.base/macro-source-path.c    |  22 ++
 gdb/testsuite/gdb.base/macro-source-path.exp  |  87 +++++
 .../gdb.dwarf2/dw2-objfile-overlap.exp        |   2 +-
 .../gdb.dwarf2/imported-unit-bp.exp.tcl       |   2 +-
 gdb/testsuite/gdb.dwarf2/macro-source-path.c  |  20 ++
 .../gdb.dwarf2/macro-source-path.exp          | 299 ++++++++++++++++++
 gdb/testsuite/lib/dwarf.exp                   |  92 ++++++
 17 files changed, 731 insertions(+), 183 deletions(-)
 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

-- 
2.35.1

[PATCH v2 1/6] gdb: introduce symtab_create_debug_printf

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

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 b136c605b1a..b6dea7ece2b 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 cbd0ad22392..f4cb89590ff 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 5d9949bca1d..0670aced87d 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 6f546f5b059..b9ecef04537 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 40283350cbe..87cb87e252a 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2595,6 +2595,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.1

[PATCH v2 2/6] gdb: add debug prints in buildsym.c

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

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 586c0920468..98dd674346b 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -497,6 +497,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;
@@ -517,6 +519,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;
 	}
@@ -745,6 +749,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.1

[PATCH v2 3/6] gdb/dwarf: add constructor to line_header

[Simon Marchi]

The following patch conditionally initializes the m_include_dir vector
in the constructor, based on the line header version number.  This
requires to know the version number in the constructor.

While at it, add a constructor that receives as parameters all the
information that comes before the list of directories and files, rather
than just passing the version.

Add a second constructor specifically for the case of looking up a
line_header in an htab_t.

Change-Id: I4fbdf4616b3a143b7154f1d1c2ee552e46cdc09a
---
 gdb/dwarf2/line-header.c | 64 ++++++++++++++++++++++------------------
 gdb/dwarf2/line-header.h | 35 ++++++++++++++++++----
 gdb/dwarf2/read.c        |  4 +--
 3 files changed, 67 insertions(+), 36 deletions(-)

diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 63230847568..4c214da8d28 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -266,7 +266,6 @@ dwarf_decode_line_header  (sect_offset sect_off, bool is_dwz,
 			   struct dwarf2_section_info *section,
 			   const struct comp_unit_head *cu_header)
 {
-  const gdb_byte *line_ptr;
   unsigned int bytes_read, offset_size;
   int i;
   const char *cur_dir, *cur_file;
@@ -281,12 +280,7 @@ dwarf_decode_line_header  (sect_offset sect_off, bool is_dwz,
       return 0;
     }
 
-  line_header_up lh (new line_header ());
-
-  lh->sect_off = sect_off;
-  lh->offset_in_dwz = is_dwz;
-
-  line_ptr = section->buffer + to_underlying (sect_off);
+  const gdb_byte *line_ptr = section->buffer + to_underlying (sect_off);
 
   /* Read in the header.  */
   LONGEST unit_length
@@ -294,24 +288,26 @@ dwarf_decode_line_header  (sect_offset sect_off, bool is_dwz,
 					      &bytes_read, &offset_size);
   line_ptr += bytes_read;
 
-  const gdb_byte *start_here = line_ptr;
+  const gdb_byte *statement_program_end = line_ptr + unit_length;
 
   if (line_ptr + unit_length > (section->buffer + section->size))
     {
       dwarf2_statement_list_fits_in_line_number_section_complaint ();
       return 0;
     }
-  lh->statement_program_end = start_here + unit_length;
-  lh->version = read_2_bytes (abfd, line_ptr);
+
+  short version = read_2_bytes (abfd, line_ptr);
   line_ptr += 2;
-  if (lh->version > 5)
+
+  if (version > 5)
     {
       /* This is a version we don't understand.  The format could have
 	 changed in ways we don't handle properly so just punt.  */
       complaint (_("unsupported version in .debug_line section"));
       return NULL;
     }
-  if (lh->version >= 5)
+
+  if (version >= 5)
     {
       gdb_byte segment_selector_size;
 
@@ -332,43 +328,55 @@ dwarf_decode_line_header  (sect_offset sect_off, bool is_dwz,
 
   LONGEST header_length = read_offset (abfd, line_ptr, offset_size);
   line_ptr += offset_size;
-  lh->statement_program_start = line_ptr + header_length;
-  lh->minimum_instruction_length = read_1_byte (abfd, line_ptr);
+  const gdb_byte *statement_program_start = line_ptr + header_length;
+  unsigned char minimum_instruction_length = read_1_byte (abfd, line_ptr);
   line_ptr += 1;
 
-  if (lh->version >= 4)
+  unsigned char maximum_ops_per_instruction;
+  if (version >= 4)
     {
-      lh->maximum_ops_per_instruction = read_1_byte (abfd, line_ptr);
+      maximum_ops_per_instruction = read_1_byte (abfd, line_ptr);
       line_ptr += 1;
     }
   else
-    lh->maximum_ops_per_instruction = 1;
+    maximum_ops_per_instruction = 1;
 
-  if (lh->maximum_ops_per_instruction == 0)
+  if (maximum_ops_per_instruction == 0)
     {
-      lh->maximum_ops_per_instruction = 1;
+      maximum_ops_per_instruction = 1;
       complaint (_("invalid maximum_ops_per_instruction "
 		   "in `.debug_line' section"));
     }
 
-  lh->default_is_stmt = read_1_byte (abfd, line_ptr);
+  bool default_is_stmt = read_1_byte (abfd, line_ptr);
   line_ptr += 1;
-  lh->line_base = read_1_signed_byte (abfd, line_ptr);
+  signed char line_base = read_1_signed_byte (abfd, line_ptr);
   line_ptr += 1;
-  lh->line_range = read_1_byte (abfd, line_ptr);
+  unsigned char line_range = read_1_byte (abfd, line_ptr);
   line_ptr += 1;
-  lh->opcode_base = read_1_byte (abfd, line_ptr);
+  unsigned char opcode_base = read_1_byte (abfd, line_ptr);
   line_ptr += 1;
-  lh->standard_opcode_lengths.reset (new unsigned char[lh->opcode_base]);
+  std::unique_ptr<unsigned char[]> standard_opcode_lengths
+    (new unsigned char[opcode_base]);
 
-  lh->standard_opcode_lengths[0] = 1;  /* This should never be used anyway.  */
-  for (i = 1; i < lh->opcode_base; ++i)
+  standard_opcode_lengths[0] = 1;  /* This should never be used anyway.  */
+  for (i = 1; i < opcode_base; ++i)
     {
-      lh->standard_opcode_lengths[i] = read_1_byte (abfd, line_ptr);
+      standard_opcode_lengths[i] = read_1_byte (abfd, line_ptr);
       line_ptr += 1;
     }
 
-  if (lh->version >= 5)
+  line_header_up lh (new line_header (sect_off, is_dwz, version,
+				      minimum_instruction_length,
+				      maximum_ops_per_instruction,
+				      default_is_stmt,
+				      line_base, line_range,
+				      opcode_base,
+				      std::move (standard_opcode_lengths),
+				      statement_program_start,
+				      statement_program_end));
+
+  if (version >= 5)
     {
       /* Read directory table.  */
       read_formatted_entries (per_objfile, abfd, &line_ptr, lh.get (),
diff --git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h
index 59b317d0d72..f7c18784649 100644
--- a/gdb/dwarf2/line-header.h
+++ b/gdb/dwarf2/line-header.h
@@ -68,8 +68,33 @@ struct file_entry
    which contains the following information.  */
 struct line_header
 {
-  line_header ()
-    : offset_in_dwz {}
+  line_header (sect_offset sect_off, bool offset_in_dwz, short version,
+	       unsigned char minimum_instruction_length,
+	       unsigned char maximum_ops_per_instruction,
+	       bool default_is_stmt, signed char line_base,
+	       unsigned char line_range, unsigned char opcode_base,
+	       std::unique_ptr<unsigned char[]> standard_opcode_lengths,
+	       const gdb_byte *statement_program_start,
+	       const gdb_byte *statement_program_end)
+    : sect_off (sect_off),
+      offset_in_dwz (offset_in_dwz),
+      version (version),
+      minimum_instruction_length (minimum_instruction_length),
+      maximum_ops_per_instruction (maximum_ops_per_instruction),
+      default_is_stmt (default_is_stmt),
+      line_base (line_base),
+      line_range (line_range),
+      opcode_base (opcode_base),
+      standard_opcode_lengths (std::move (standard_opcode_lengths)),
+      statement_program_start (statement_program_start),
+      statement_program_end (statement_program_end)
+  {}
+
+  /* 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.  */
@@ -131,16 +156,16 @@ struct line_header
   { return m_file_names; }
 
   /* Offset of line number information in .debug_line section.  */
-  sect_offset sect_off {};
+  sect_offset sect_off;
 
   /* OFFSET is for struct dwz_file associated with dwarf2_per_objfile.  */
-  unsigned offset_in_dwz : 1; /* Can't initialize bitfields in-class.  */
+  unsigned offset_in_dwz : 1;
 
   unsigned short version {};
   unsigned char minimum_instruction_length {};
   unsigned char maximum_ops_per_instruction {};
   unsigned char default_is_stmt {};
-  int line_base {};
+  signed char line_base {};
   unsigned char line_range {};
   unsigned char opcode_base {};
 
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d6fccf83dd2..14636e9bf0b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -9420,7 +9420,6 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 {
   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 +9448,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)
     {
-- 
2.35.1

[PATCH v2 4/6] gdb/dwarf: pass compilation directory to line header, initialize dir[0] for DWARF 4

[Simon Marchi]

I noticed that when building:

  $ gcc --version
  gcc (Ubuntu 9.4.0-1ubuntu1~20.04) 9.4.0
  $ ld --version
  GNU ld (GNU Binutils for Ubuntu) 2.34
  $ gcc -gdwarf-4 -g3 test.c

I get the following files and directories in the line table header:

    include_directories[  1] = "/usr/include"
    file_names[  1]:
               name: "test.c"
          dir_index: 0
           mod_time: 0x00000000
             length: 0x00000000
    file_names[  2]:
               name: "stdc-predef.h"
          dir_index: 1
           mod_time: 0x00000000
             length: 0x00000000

file_names[1] refers to directory index 0, which in DWARF 4 means the
compilation directory, not explicitly listed in the directory table.
The compilation directory can be found in the DW_AT_comp_dir attribute
of the compilation unit.  DWARF 5 explicitly lists it in the directory
table.  The reason for this is to make it possible to use the line table
without reading (or having access to) the DIE tree.

DWARF 4, section 6.2.4 (The Line Number Program Header), item 12, says:

    The directory index represents an entry in the include_directories
    section. The index is 0 if the file was found in the current
    directory of the compilation, 1 if it was found in the first
    directory in the include_directories section, and so on. The
    directory index is ignored for file names that represent full path
    names.

Currently, our implementation of line_header::include_dir_at is such
that for DWARF 4, if the directory index 0 is requested, we return
nullptr, as if it didn't exist.  If the same file was using a DWARF 5
line table, the directory at index 0 would exist, so we would return it.
I think it would be more consistent to return the compilation directory
in that case, for DWARF 4.  It reduces a bit the differences between how
things end up for DWARF 4 and 5.

My use case for this is that along with the next patch, it will ensure
that a path created by doing "DW_AT_comp_dir + DW_AT_name" (the main
subfile/symtab of a CU) ends up the same as a path representing the same
main file but created from the line header.

To achieve this, pass the compilation directory down when creating a
line_header object.  If the line table is version <= 4, pre-fill index 0
of the include dirs vector with the compilation directory.  In some
contexts (like type units), there is no compilation directory, so it is
allowed to pass nullptr as the compilation directory.  In this case, if
the include dir 0 is ever requested, we will return nullptr, just like
today.

I am not sure if this causes user-visible changes (I haven't seen any).
But if it does, it will probably be some source file path somewhere that
used to not contain the compilation directory, and will now contain it.

Change-Id: Iba3ba0293e4e2d13a64b257cf9a3094684d54330
---
 gdb/dwarf2/line-header.c | 18 +++++++-----------
 gdb/dwarf2/line-header.h | 36 ++++++++++++++++++++++++------------
 gdb/dwarf2/read.c        | 23 ++++++++++++++---------
 3 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 4c214da8d28..37f4aacdc12 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -30,15 +30,9 @@ void
 line_header::add_include_dir (const char *include_dir)
 {
   if (dwarf_line_debug >= 2)
-    {
-      size_t new_size;
-      if (version >= 5)
-	new_size = m_include_dirs.size ();
-      else
-	new_size = m_include_dirs.size () + 1;
-      gdb_printf (gdb_stdlog, "Adding dir %zu: %s\n",
-		  new_size, include_dir);
-    }
+    gdb_printf (gdb_stdlog, "Adding dir %zu: %s\n",
+		m_include_dirs.size (), include_dir);
+
   m_include_dirs.push_back (include_dir);
 }
 
@@ -264,7 +258,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 *dw_at_comp_dir)
 {
   unsigned int bytes_read, offset_size;
   int i;
@@ -374,7 +369,8 @@ dwarf_decode_line_header  (sect_offset sect_off, bool is_dwz,
 				      opcode_base,
 				      std::move (standard_opcode_lengths),
 				      statement_program_start,
-				      statement_program_end));
+				      statement_program_end,
+				      dw_at_comp_dir));
 
   if (version >= 5)
     {
diff --git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h
index f7c18784649..873d6d1e60f 100644
--- a/gdb/dwarf2/line-header.h
+++ b/gdb/dwarf2/line-header.h
@@ -68,6 +68,9 @@ struct file_entry
    which contains the following information.  */
 struct line_header
 {
+  /* DW_AT_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 (sect_offset sect_off, bool offset_in_dwz, short version,
 	       unsigned char minimum_instruction_length,
 	       unsigned char maximum_ops_per_instruction,
@@ -75,7 +78,8 @@ struct line_header
 	       unsigned char line_range, unsigned char opcode_base,
 	       std::unique_ptr<unsigned char[]> standard_opcode_lengths,
 	       const gdb_byte *statement_program_start,
-	       const gdb_byte *statement_program_end)
+	       const gdb_byte *statement_program_end,
+	       const char *dw_at_comp_dir)
     : sect_off (sect_off),
       offset_in_dwz (offset_in_dwz),
       version (version),
@@ -87,8 +91,15 @@ struct line_header
       opcode_base (opcode_base),
       standard_opcode_lengths (std::move (standard_opcode_lengths)),
       statement_program_start (statement_program_start),
-      statement_program_end (statement_program_end)
-  {}
+      statement_program_end (statement_program_end),
+      m_dw_at_comp_dir (dw_at_comp_dir != nullptr ? dw_at_comp_dir : "")
+  {
+    /* For DWARF 4, directory 0 is implicit and means the compilation
+       directory.  For DWARF 5, it is explicitly listed in the directory table
+       and will be added with a call to add_include_dir.  */
+    if (version < 5)
+      m_include_dirs.push_back (dw_at_comp_dir);
+  }
 
   /* This constructor should only be used to create line_header intances to do
      hash table lookups.  */
@@ -108,14 +119,10 @@ struct line_header
      Returns NULL if INDEX is out of bounds.  */
   const char *include_dir_at (dir_index index) const
   {
-    int vec_index;
-    if (version >= 5)
-      vec_index = index;
-    else
-      vec_index = index - 1;
-    if (vec_index < 0 || vec_index >= m_include_dirs.size ())
-      return NULL;
-    return m_include_dirs[vec_index];
+    if (index < 0 || index >= m_include_dirs.size ())
+      return nullptr;
+
+    return m_include_dirs[index];
   }
 
   bool is_valid_file_index (int file_index) const
@@ -196,6 +203,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;
+
+  /* DW_AT_comp_dir attribute of the compilation unit in the context of which we
+     are reading this line header.  Empty if not applicable.  */
+  std::string m_dw_at_comp_dir;
 };
 
 typedef std::unique_ptr<line_header> line_header_up;
@@ -216,6 +227,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 *dw_at_comp_dir);
 
 #endif /* DWARF2_LINE_HEADER_H */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 14636e9bf0b..7c06f45fde6 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 *dw_at_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,6 +9417,7 @@ 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;
@@ -9469,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;
 
@@ -9547,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)
@@ -9621,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)
     {
@@ -19659,7 +19662,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 *dw_at_comp_dir)
 {
   struct dwarf2_section_info *section;
   dwarf2_per_objfile *per_objfile = cu->per_objfile;
@@ -19676,7 +19680,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,
+				   dw_at_comp_dir);
 }
 
 /* Subroutine of dwarf_decode_lines to simplify it.
-- 
2.35.1

[PATCH v2 5/6] gdb/dwarf: use complete paths for symtabs and macro_source_files

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

Printing macros defined in the main source file doesn't work reliably using
various toolchains, especially when DWARF 5 is used.  For example, using either
of these binaries:

    $ 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.  But
it doesn't find it because the filename it is looking for isn't exactly
like the filename as written in the macro_source_file object.

The `name` parameter comes from the symtab::filename field of the symtab
we are stopped at.  The symtab's filename 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 to create the main subfile of the compunit,
which eventually becomes the compilation unit's main symtab.  In this
case it is just the filename, "test.c".

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 the file is
relative to, if the file name is not 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.

This patch fixes things by making the DWARF reader "render" file names
always the same way, as complete as possible.  For the main symtab's
filename (what is passed to the builder constructor), that means: if
DW_AT_name is not absolute, prepend DW_AT_comp_dir.  This is done in
dwarf2_cu::start_compunit_symtab.

For paths coming from the line header:

 1) if the filename alone is already absolute, return it
 2) otherwise, prepend the directory referenced by the file entry, if
    that is now absolute, return that
 3) otherwise, prepend the compilation unit's compilation directory and
    return that

With this, paths built from DW_AT_name/DW_AT_comp_dir should always come
out in the same form (at least, if we can trust the compiler did things
correctly).

Since this changes some symtab names, there is some user-visible
changes when those names are output, as can be seen from the few tests I
needed to update.  The difference is that some symtab names that
previously didn't include a directory portion will now include one.

I don't know if that will bother some people.  I am personally fine with
this change.

Finally, change the comment above the line_header::file_file_name
declaration.  The part about it returning a name relative the
compilation directory is just not true, I found it very misleading when
working on this.  We can't guarantee that the result will be relative to
the compilation directory, if the compiler has put some absolute file
and directory paths in the line header.

Change-Id: I0372906dafc01d6b3774b2ab72f9d28d7069b6cc
---
 gdb/dwarf2/cu.c                               | 11 ++++
 gdb/dwarf2/line-header.c                      | 53 +++++++++--------
 gdb/dwarf2/line-header.h                      | 11 ++--
 gdb/dwarf2/read.c                             | 58 ++++---------------
 .../gdb.dwarf2/dw2-objfile-overlap.exp        |  2 +-
 .../gdb.dwarf2/imported-unit-bp.exp.tcl       |  2 +-
 6 files changed, 60 insertions(+), 77 deletions(-)

diff --git a/gdb/dwarf2/cu.c b/gdb/dwarf2/cu.c
index fcb0d77623b..d8c74b14969 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,6 +62,15 @@ dwarf2_cu::start_compunit_symtab (const char *name, const char *comp_dir,
 {
   gdb_assert (m_builder == nullptr);
 
+  /* Join NAME and COMP_DIR together.  This is done to match what line_header
+     does, assembling paths as much as possible up to the comp dir.  */
+  std::string name_copy;
+  if (!IS_ABSOLUTE_PATH (name) && comp_dir != nullptr && comp_dir[0] != '\0')
+    {
+      name_copy = path_join (comp_dir, name);
+      name = name_copy.c_str ();
+    }
+
   m_builder.reset (new struct buildsym_compunit
 		   (this->per_objfile->objfile,
 		    name, comp_dir, per_cu->lang, low_pc));
diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 37f4aacdc12..e67c30d0963 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -42,17 +42,13 @@ line_header::add_file_name (const char *name,
 			    unsigned int mod_time,
 			    unsigned int length)
 {
+  file_name_index index =
+    (this->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
@@ -60,20 +56,7 @@ line_header::file_file_name (int file) 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);
-
-      if (!IS_ABSOLUTE_PATH (fe->name))
-	{
-	  const char *dir = fe->include_dir (this);
-	  if (dir != NULL)
-	    return path_join (dir, fe->name);
-	}
-
-      return fe->name;
-    }
-  else
+  if (!is_valid_file_index (file))
     {
       /* The compiler produced a bogus file number.  We can at least
 	 record the macro definitions made in the file, even if we
@@ -83,6 +66,28 @@ line_header::file_file_name (int file) const
 
       return string_printf ("<bad macro file number %d>", file);
     }
+
+  /* Make the name as "complete" as possible.  Prepend the include directory and
+     compilation directory if needed.  */
+
+  /* If the file name is already absolute, no need to prepend anything.  */
+  const file_entry *fe = file_name_at (file);
+  if (IS_ABSOLUTE_PATH (fe->name))
+    return fe->name;
+
+  /* The file name is relative, prepend the directory name (if we have one)
+     associated to the file entry.  */
+  const char *dir = fe->include_dir (this);
+  if (dir == nullptr)
+    return fe->name;
+
+  /* If the directory is absolute or we don't have a comp dir, prepend only
+     the directory.  */
+  if (IS_ABSOLUTE_PATH (dir) || m_dw_at_comp_dir.empty ())
+    return path_join (dir, fe->name);
+
+  /* Otherwise, concatenate the comp dir, the directory and the file name.  */
+  return path_join (m_dw_at_comp_dir.c_str (), dir, fe->name);
 }
 
 static void
diff --git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h
index 873d6d1e60f..5e191d023e7 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,7 +53,10 @@ struct file_entry
      owned by debug_line_buffer.  */
   const char *name {};
 
-  /* The directory index (1-based).  */
+  /* Index of this file in the line header file table.  */
+  file_name_index index {};
+
+  /* The directory index.  */
   dir_index d_index {};
 
   unsigned int mod_time {};
@@ -189,8 +193,7 @@ 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
-     number FILE in this object's file name table.  */
+  /* Return file name of file number FILE in this object's file name table.  */
   std::string file_file_name (int file) const;
 
  private:
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 7c06f45fde6..31bd4a56ac7 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -950,8 +950,7 @@ 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 (struct dwarf2_cu *cu, const char *filename);
 
 static struct symbol *new_symbol (struct die_info *, struct type *,
 				  struct dwarf2_cu *, struct symbol * = NULL);
@@ -9665,8 +9664,9 @@ 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, line_header->file_file_name (fe.index).c_str ());
+
 	  buildsym_compunit *b = get_builder ();
 	  if (b->get_current_subfile ()->symtab == NULL)
 	    {
@@ -19943,11 +19943,10 @@ 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);
+      std::string name = m_line_header->file_file_name (fe->index);
+      dwarf2_start_subfile (m_cu, name.c_str ());
     }
 }
 
@@ -20230,7 +20229,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, lh->file_file_name (fe->index).c_str ());
 
       /* Decode the table.  */
       while (line_ptr < line_end && !end_sequence)
@@ -20442,7 +20441,7 @@ 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));
+      dwarf2_start_subfile (cu, lh->file_file_name (fe.index).c_str ());
       if (builder->get_current_subfile ()->symtab == NULL)
 	{
 	  builder->get_current_subfile ()->symtab
@@ -20453,48 +20452,13 @@ dwarf_decode_lines (struct line_header *lh, struct dwarf2_cu *cu,
     }
 }
 
-/* Start a subfile for DWARF.  FILENAME is the name of the file and
-   DIRNAME the name of the source directory which contains FILENAME
-   or NULL if not known.
-   This routine tries to keep line numbers from identical absolute and
-   relative file names in a common subfile.
-
-   Using the `list' example from the GDB testsuite, which resides in
-   /srcdir and compiling it with Irix6.2 cc in /compdir using a filename
-   of /srcdir/list0.c yields the following debugging information for list0.c:
+/* Start a subfile for DWARF.
 
-   DW_AT_name:          /srcdir/list0.c
-   DW_AT_comp_dir:      /compdir
-   files.files[0].name: list0.h
-   files.files[0].dir:  /srcdir
-   files.files[1].name: list0.c
-   files.files[1].dir:  /srcdir
-
-   The line number information for list0.c has to end up in a single
-   subfile, so that `break /srcdir/list0.c:1' works as expected.
-   start_subfile will ensure that this happens provided that we pass the
-   concatenation of files.files[1].dir and files.files[1].name as the
-   subfile's name.  */
+   FILENAME is the name of the subfile.  */
 
 static void
-dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
-		      const char *dirname)
+dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename)
 {
-  std::string copy;
-
-  /* In order not to lose the line information directory,
-     we concatenate it to the filename when it makes sense.
-     Note that the Dwarf3 standard says (speaking of filenames in line
-     information): ``The directory index is ignored for file names
-     that represent full path names''.  Thus ignoring dirname in the
-     `else' branch below isn't an issue.  */
-
-  if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL)
-    {
-      copy = path_join (dirname, filename);
-      filename = copy.c_str ();
-    }
-
   cu->get_builder ()->start_subfile (filename);
 }
 
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-objfile-overlap.exp b/gdb/testsuite/gdb.dwarf2/dw2-objfile-overlap.exp
index 2257dd22819..6ea68e1f997 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-objfile-overlap.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-objfile-overlap.exp
@@ -45,4 +45,4 @@ gdb_breakpoint "*outer_before"
 
 # FAIL was:
 # No line number information available for address 0x4 <outer_inner>
-gdb_test "info line inner" {Line 2 of "inner\.c" starts at address .*}
+gdb_test "info line inner" {Line 2 of "/tmp/inner\.c" starts at address .*}
diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit-bp.exp.tcl b/gdb/testsuite/gdb.dwarf2/imported-unit-bp.exp.tcl
index fe92c530888..d2a74c5031b 100644
--- a/gdb/testsuite/gdb.dwarf2/imported-unit-bp.exp.tcl
+++ b/gdb/testsuite/gdb.dwarf2/imported-unit-bp.exp.tcl
@@ -126,4 +126,4 @@ if { [prepare_for_testing "failed to prepare" ${testfile} \
 gdb_reinitialize_dir /tmp
 
 # Using an absolute path is important to see the bug.
-gdb_test "break /tmp/${srcfile}:19" "Breakpoint .* file $srcfile, line .*"
+gdb_test "break /tmp/${srcfile}:19" "Breakpoint .* file /tmp/$srcfile, line .*"
-- 
2.35.1

[PATCH v2 6/6] gdb/testsuite: add macros test for source files compiled in various ways

[Simon Marchi]

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          | 299 ++++++++++++++++++
 gdb/testsuite/lib/dwarf.exp                   |  92 ++++++
 5 files changed, 520 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 00000000000..f4ede117a2a
--- /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 00000000000..edbb4aee8e9
--- /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 00000000000..749bcc1580e
--- /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 00000000000..0b2b6e0277d
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/macro-source-path.exp
@@ -0,0 +1,299 @@
+# 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.
+			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 as from binutils 2.38.
+
+## test.c
+do_test gcc11-ld238-filename 5 "test.c" 1 {
+    "/tmp/cwd"
+} {
+    {"test.c" 0}
+    {"test.c" 0}
+}
+
+## ./test.c
+do_test gcc11-ld238-dot-filename 5 "./test.c" 1 {
+    "/tmp/cwd"
+    "."
+} {
+    {"test.c" 1}
+    {"test.c" 1}
+}
+
+## ../cwd/test.c
+do_test gcc11-ld238-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-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-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-absolute-other 5 "/tmp/other/test.c" 1 {
+    "/tmp/cwd"
+    "/tmp/other"
+} {
+    {"test.c" 1}
+    {"test.c" 1}
+}
+
+# Based on `clang-14 -gdwarf-5 -fdebug-macro -g3 <file>` (using its
+# built-in assembler)
+
+## test.c
+do_test clang14-filename 5 "test.c" 0 {
+    "/tmp/cwd"
+} {
+    {"test.c" 0}
+}
+
+## ./test.c
+do_test clang14-dot-filename 5 "test.c" 1 {
+    "/tmp/cwd"
+    "."
+} {
+    {"test.c" 0}
+    {"test.c" 1}
+}
+
+## ../cwd/test.c
+do_test clang14-dot-dot-cwd 5 "../cwd/test.c" 0 {
+    "/tmp/cwd"
+} {
+    {"../cwd/test.c" 0}
+}
+
+## /tmp/cwd/test.c
+do_test clang14-absolute-cwd  5 "/tmp/cwd/test.c" 1 {
+    "/tmp/cwd"
+} {
+    {"/tmp/cwd/test.c" 0}
+    {"test.c" 0}
+}
+
+## ../other/test.c
+do_test clang14-dot-dot-other 5 "../other/test.c" 0 {
+    "/tmp/cwd"
+} {
+    {"../other/test.c" 0}
+}
+
+## /tmp/other/test.c
+do_test clang14-absolute-other 5 "/tmp/other/test.c" 1 {
+    "/tmp/cwd"
+    "/tmp"
+} {
+    {"/tmp/other/test.c" 0}
+    {"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-filename 3 "test.c" 1 {
+} {
+    {"test.c" 0}
+}
+
+## ./test.c
+do_test gcc11-ld234-dot-filename 3 "./test.c" 1 {
+    "."
+} {
+    {"test.c" 1}
+}
+
+## ../cwd/test.c
+do_test gcc11-ld234-dot-dot-cwd 3 "../cwd/test.c" 1 {
+    "../cwd"
+} {
+    {"test.c" 1}
+}
+
+## /tmp/cwd/test.c
+do_test gcc11-ld234-absolute-cwd 3 "/tmp/cwd/test.c" 1 {
+    "/tmp/cwd"
+} {
+    {"test.c" 1}
+}
+
+## ../other/test.c
+do_test gcc11-ld234-dot-dot-other 3 "../other/test.c" 1 {
+    "../other"
+} {
+    {"test.c" 1}
+}
+
+## /tmp/other/test.c
+do_test gcc11-ld234-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 55e97c33a6e..fb62e3ca038 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.1

Re: [PATCH v2 5/6] gdb/dwarf: use complete paths for symtabs and macro_source_files

[Simon Marchi]

On 2022-04-21 16:21, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> Printing macros defined in the main source file doesn't work reliably using
> various toolchains, especially when DWARF 5 is used.  For example, using either
> of these binaries:
> 
>     $ 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.  But
> it doesn't find it because the filename it is looking for isn't exactly
> like the filename as written in the macro_source_file object.
> 
> The `name` parameter comes from the symtab::filename field of the symtab
> we are stopped at.  The symtab's filename 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 to create the main subfile of the compunit,
> which eventually becomes the compilation unit's main symtab.  In this
> case it is just the filename, "test.c".
> 
> 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 the file is
> relative to, if the file name is not 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.
> 
> This patch fixes things by making the DWARF reader "render" file names
> always the same way, as complete as possible.  For the main symtab's
> filename (what is passed to the builder constructor), that means: if
> DW_AT_name is not absolute, prepend DW_AT_comp_dir.  This is done in
> dwarf2_cu::start_compunit_symtab.
> 
> For paths coming from the line header:
> 
>  1) if the filename alone is already absolute, return it
>  2) otherwise, prepend the directory referenced by the file entry, if
>     that is now absolute, return that
>  3) otherwise, prepend the compilation unit's compilation directory and
>     return that
> 
> With this, paths built from DW_AT_name/DW_AT_comp_dir should always come
> out in the same form (at least, if we can trust the compiler did things
> correctly).
> 
> Since this changes some symtab names, there is some user-visible
> changes when those names are output, as can be seen from the few tests I
> needed to update.  The difference is that some symtab names that
> previously didn't include a directory portion will now include one.
> 
> I don't know if that will bother some people.  I am personally fine with
> this change.

I sent this a bit too fast, it seems like I didn't test this patch
properly.  There is a handful of tests that are affected, where expected
paths need to be updated.

There's one in particular, gdb.dwarf2/dw2-dir-file-name.exp, that made
me learn about the "set filename-display relative" setting (which is
actually the default value for "set filename-display").  This says that
GDB should print source files as relative to the compilation directory.
With my patch, we always print source file paths with the compilation
directory in it, so that renders this setting a bit moot.

[Note that even today, if the user passes an absolute path to the
compiler, they will get an absolute path in GDB, even though the setting
is "relative".]

This tells me that it's perhaps not a good idea to go this route.  If
a setting exists to control whether the user wants the path formatted as
relative to the compilation directory (if possible) or absolute, we
should probably keep honoring that.

An alternative I can explore is to try to add the complete path to
symtabs as a separate, new field, but keep symtab::filename as is.  The
new field would just be used as a "stable" identifier for the symtab.
But presentation-wise, things would stay the same.  This would avoid (in
theory) having to update a bunch of tests, and it wouldn't change the
user experience.

Simon