diff mbox

PR77895: DWARF: Emit DW_AT_comp_dir in all cases, even if source is an absolute path

Message ID 85c55bf6-98ed-0277-f035-beaf469bb5bc@pwned.gg
State New
Headers show

Commit Message

Ximin Luo Oct. 17, 2016, 6 p.m. UTC 
(Please keep me on CC, I am not subscribed)

In working on another patch for DW_AT_comp_dir behaviour, I tried to write a
test for that other patch. This test did not work as expected - DW_AT_comp_dir
was not even generated in the resulting output! But this had nothing to do with
my patch so I dug a bit deeper.

It turns out that GCC does not emit DW_AT_comp_dir if the source path given is
an absolute path, and the file being compiled contains a typedef to a compiler
builtin (sorry, I don't know the terminology here very well). This typedef
exists in the vast majority of cases in the real world via standard library
includes, so from the outside, the behaviour of GCC "looks like" it emits
DW_AT_comp_dir in all/most cases.

From looking at the source code dwarf2out.c, Richard Biener determined that the
current code is written to "intend to" not emit DW_AT_comp_dir if to path is
absolute, regardless of the typedef. It is possible to "fix" this bug by a
1-line change, to conform to the way currently "intended" by GCC code. However,
I think a much better fix is a 22-line deletion of code from dwarf2out.c, to
instead emit DW_AT_comp_dir in all cases.

The original aim of not emitting DW_AT_comp_dir seems to be to avoid emitting
redundant information. However, this information is *not* redundant! The DWARF2
spec says:

"A DW_AT_comp_dir attribute whose value is a null-terminated string containing
the current working directory of the compilation command that produced this
compilation unit in whatever form makes sense for the host system."

Conceptually, the working directory is independent from an input file, so it
would *not* be redundant to emit it in the general case. In future versions of
DWARF, other information might be added (such as relative -I flags, etc) that
are dependent on knowing the working directory. The logic of "don't emit
DW_AT_comp_dir if DW_AT_name is absolute" would then break. In other words, the
choice to avoid emitting DW_AT_comp_dir is a brittle and premature optimisation
that loses information in the general case. Therefore, it is better to emit it
in all circumstances, in case the reader needs to know what the working
directory was at compile-time.

*Sometimes*, *parts* of it might be redundant yes - and rewriting DW_AT_name to
be relative to this, would remove the redundancy. Doing this is compatible with
the above points, and I could amend the patch to do this - although I suggest
it's not worth the effort, unless there is a function in GCC code that already
does this.

Some more minor advantages are:

- We delete 21 lines, this reduces complexity in the already-complex code.
  We can also get rid of the Darwin-specific workaround described here:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53453 (Not currently part of my
  patch, but I can add this in.)

- The GCC test suite compiles tests via their absolute paths, and many many
  other buildsystems do this too. This was in fact my original problem - I was
  trying to test something involving DW_AT_comp_dir, but GCC does not emit this
  in the testsuite! Rather than saying "this is a bug, everyone else should
  spend effort fixing this", it is better to fix it in one place, i.e. the
  source of where it is (not) generated.

Thanks for your time,

Ximin

-- 
GPG: ed25519/56034877E1F87C35
GPG: rsa4096/1318EFAC5FBBDBCE
https://github.com/infinity0/pubkeys.git

Comments

Mike Stump Oct. 17, 2016, 9:26 p.m. UTC | #1 
On Oct 17, 2016, at 11:00 AM, Ximin Luo <infinity0@pwned.gg> wrote:
> Therefore, it is better to emit it in all circumstances, in case the reader needs to know what the working

> directory was at compile-time.


I can't help but wonder if this would break ccache some?
Ximin Luo Oct. 17, 2016, 9:38 p.m. UTC | #2 
Mike Stump:
> On Oct 17, 2016, at 11:00 AM, Ximin Luo <infinity0@pwned.gg> wrote:

>> Therefore, it is better to emit it in all circumstances, in case the reader needs to know what the working

>> directory was at compile-time.

> 

> I can't help but wonder if this would break ccache some?

> 


Could you explain this in some more detail? At the moment, GCC will already emit DW_AT_name with an absolute path (in the scenario that this patch is relevant to). How does ccache work around this at the moment? (Does it use debug-prefix-map? In which case, this also affects DW_AT_comp_dir, so my patch should be fine.)

X

-- 
GPG: ed25519/56034877E1F87C35
GPG: rsa4096/1318EFAC5FBBDBCE
https://github.com/infinity0/pubkeys.git
Mike Stump Oct. 17, 2016, 9:44 p.m. UTC | #3 
On Oct 17, 2016, at 2:38 PM, Ximin Luo <infinity0@pwned.gg> wrote:
> 

> Mike Stump:

>> On Oct 17, 2016, at 11:00 AM, Ximin Luo <infinity0@pwned.gg> wrote:

>>> Therefore, it is better to emit it in all circumstances, in case the reader needs to know what the working

>>> directory was at compile-time.

>> 

>> I can't help but wonder if this would break ccache some?

>> 

> 

> Could you explain this in some more detail? At the moment, GCC will already emit DW_AT_name with an absolute path (in the scenario that this patch is relevant to). How does ccache work around this at the moment? (Does it use debug-prefix-map? In which case, this also affects DW_AT_comp_dir, so my patch should be fine.)


If you compile the same file, but in a different directory, I wonder if cwd will cause the cache entry to not be reused.

I expect one of the ccache people that are around would just know if it will care at all.
Richard Biener Oct. 18, 2016, 8:21 a.m. UTC | #4 
On Mon, Oct 17, 2016 at 11:44 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Oct 17, 2016, at 2:38 PM, Ximin Luo <infinity0@pwned.gg> wrote:

>>

>> Mike Stump:

>>> On Oct 17, 2016, at 11:00 AM, Ximin Luo <infinity0@pwned.gg> wrote:

>>>> Therefore, it is better to emit it in all circumstances, in case the reader needs to know what the working

>>>> directory was at compile-time.

>>>

>>> I can't help but wonder if this would break ccache some?

>>>

>>

>> Could you explain this in some more detail? At the moment, GCC will already emit DW_AT_name with an absolute path (in the scenario that this patch is relevant to). How does ccache work around this at the moment? (Does it use debug-prefix-map? In which case, this also affects DW_AT_comp_dir, so my patch should be fine.)

>

> If you compile the same file, but in a different directory, I wonder if cwd will cause the cache entry to not be reused.

>

> I expect one of the ccache people that are around would just know if it will care at all.


I believe ccache compares preprocessed source, definitely _not_ DWARF
output, so this shouldn't break anything there.
It might result in different object file output but as the reporter
figured due to a bug in dwarf2out.c we end up generating
DW_AT_comp_dir in almost all cases already.

I think the patch is ok but it misses a ChangeLog entry.  How did you
test the patch? (bootstrapped and tested on ...?)

Thanks,
Richard.
Ximin Luo Oct. 18, 2016, 12:35 p.m. UTC | #5 
Richard Biener:
> On Mon, Oct 17, 2016 at 11:44 PM, Mike Stump <mikestump@comcast.net> wrote:

>> On Oct 17, 2016, at 2:38 PM, Ximin Luo <infinity0@pwned.gg> wrote:

>>>

>>> Mike Stump:

>>>> On Oct 17, 2016, at 11:00 AM, Ximin Luo <infinity0@pwned.gg> wrote:

>>>>> Therefore, it is better to emit it in all circumstances, in case the reader needs to know what the working

>>>>> directory was at compile-time.

>>>>

>>>> I can't help but wonder if this would break ccache some?

>>>>

>>>

>>> Could you explain this in some more detail? At the moment, GCC will already emit DW_AT_name with an absolute path (in the scenario that this patch is relevant to). How does ccache work around this at the moment? (Does it use debug-prefix-map? In which case, this also affects DW_AT_comp_dir, so my patch should be fine.)

>>

>> If you compile the same file, but in a different directory, I wonder if cwd will cause the cache entry to not be reused.

>>

>> I expect one of the ccache people that are around would just know if it will care at all.

> 

> I believe ccache compares preprocessed source, definitely _not_ DWARF

> output, so this shouldn't break anything there.

> It might result in different object file output but as the reporter

> figured due to a bug in dwarf2out.c we end up generating

> DW_AT_comp_dir in almost all cases already.

> 

> I think the patch is ok but it misses a ChangeLog entry.  How did you

> test the patch? (bootstrapped and tested on ...?)

> 


Thanks, I'll add the Changelog entry. My computer isn't very powerful, so I didn't bootstrap it yet, I only tested it on a stage1 compiler, on Debian testing/unstable. I'll find some time to bootstrap it and test it fully over the next few days.

Shall I also get rid of the Darwin force_at_comp_dir stuff? Looking into it a bit more, my patch basically obsoletes the need for this so I can delete that as well.

X

-- 
GPG: ed25519/56034877E1F87C35
GPG: rsa4096/1318EFAC5FBBDBCE
https://github.com/infinity0/pubkeys.git
Richard Biener Oct. 18, 2016, 1:19 p.m. UTC | #6 
On Tue, Oct 18, 2016 at 2:35 PM, Ximin Luo <infinity0@pwned.gg> wrote:
> Richard Biener:

>> On Mon, Oct 17, 2016 at 11:44 PM, Mike Stump <mikestump@comcast.net> wrote:

>>> On Oct 17, 2016, at 2:38 PM, Ximin Luo <infinity0@pwned.gg> wrote:

>>>>

>>>> Mike Stump:

>>>>> On Oct 17, 2016, at 11:00 AM, Ximin Luo <infinity0@pwned.gg> wrote:

>>>>>> Therefore, it is better to emit it in all circumstances, in case the reader needs to know what the working

>>>>>> directory was at compile-time.

>>>>>

>>>>> I can't help but wonder if this would break ccache some?

>>>>>

>>>>

>>>> Could you explain this in some more detail? At the moment, GCC will already emit DW_AT_name with an absolute path (in the scenario that this patch is relevant to). How does ccache work around this at the moment? (Does it use debug-prefix-map? In which case, this also affects DW_AT_comp_dir, so my patch should be fine.)

>>>

>>> If you compile the same file, but in a different directory, I wonder if cwd will cause the cache entry to not be reused.

>>>

>>> I expect one of the ccache people that are around would just know if it will care at all.

>>

>> I believe ccache compares preprocessed source, definitely _not_ DWARF

>> output, so this shouldn't break anything there.

>> It might result in different object file output but as the reporter

>> figured due to a bug in dwarf2out.c we end up generating

>> DW_AT_comp_dir in almost all cases already.

>>

>> I think the patch is ok but it misses a ChangeLog entry.  How did you

>> test the patch? (bootstrapped and tested on ...?)

>>

>

> Thanks, I'll add the Changelog entry. My computer isn't very powerful, so I didn't bootstrap it yet, I only tested it on a stage1 compiler, on Debian testing/unstable. I'll find some time to bootstrap it and test it fully over the next few days.

>

> Shall I also get rid of the Darwin force_at_comp_dir stuff? Looking into it a bit more, my patch basically obsoletes the need for this so I can delete that as well.


That would be nice.

Richard.

> X

>

> --

> GPG: ed25519/56034877E1F87C35

> GPG: rsa4096/1318EFAC5FBBDBCE

> https://github.com/infinity0/pubkeys.git
diff mbox

Patch

Index: gcc-7-20161009/gcc/dwarf2out.c
===================================================================
--- gcc-7-20161009.orig/gcc/dwarf2out.c
+++ gcc-7-20161009/gcc/dwarf2out.c
@@ -21994,7 +21994,7 @@  gen_compile_unit_die (const char *filena
     {
       add_name_attribute (die, filename);
       /* Don't add cwd for <built-in>.  */
-      if (!IS_ABSOLUTE_PATH (filename) && filename[0] != '<')
+      if (filename[0] != '<')
 	add_comp_dir_attribute (die);
     }
 
@@ -26332,20 +26332,6 @@  prune_unused_types (void)
     prune_unmark_dies (ctnode->root_die);
 }
 
-/* Set the parameter to true if there are any relative pathnames in
-   the file table.  */
-int
-file_table_relative_p (dwarf_file_data **slot, bool *p)
-{
-  struct dwarf_file_data *d = *slot;
-  if (!IS_ABSOLUTE_PATH (d->filename))
-    {
-      *p = true;
-      return 0;
-    }
-  return 1;
-}
-
 /* Helpers to manipulate hash table of comdat type units.  */
 
 struct comdat_type_hasher : nofree_ptr_hash <comdat_type_node>
@@ -28152,15 +28138,7 @@  dwarf2out_early_finish (const char *file
   /* Add the name for the main input file now.  We delayed this from
      dwarf2out_init to avoid complications with PCH.  */
   add_name_attribute (comp_unit_die (), remap_debug_filename (filename));
-  if (!IS_ABSOLUTE_PATH (filename) || targetm.force_at_comp_dir)
-    add_comp_dir_attribute (comp_unit_die ());
-  else if (get_AT (comp_unit_die (), DW_AT_comp_dir) == NULL)
-    {
-      bool p = false;
-      file_table->traverse<bool *, file_table_relative_p> (&p);
-      if (p)
-	add_comp_dir_attribute (comp_unit_die ());
-    }
+  add_comp_dir_attribute (comp_unit_die ());
 
   /* With LTO early dwarf was really finished at compile-time, so make
      sure to adjust the phase after annotating the LTRANS CU DIE.  */
Index: gcc-7-20161009/gcc/testsuite/gcc.dg/debug/dwarf2/pr77985.c
===================================================================
--- /dev/null
+++ gcc-7-20161009/gcc/testsuite/gcc.dg/debug/dwarf2/pr77985.c
@@ -0,0 +1,9 @@ 
+/* DW_AT_comp_dir is emitted even if the file is compiled via an absolute path,
+   as is the case in the gcc testsuite. */
+/* { dg-do compile } */
+/* { dg-options "-g -dA" } */
+/* { dg-final { scan-assembler-dem "DW_AT_comp_dir:" } } */
+
+void func (void)
+{
+}