[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Dan McGregor via Phabricator via cfe-commits]

dankm added inline comments.


================
Comment at: lib/Lex/PPMacroExpansion.cpp:1460
+  for (const auto &Entry : MacroPrefixMap)
+    if (Path.startswith(Entry.first))
+      return (Twine(Entry.second) + Path.substr(Entry.first.size())).str();
----------------
Lekensteyn wrote:
> joerg wrote:
> > Lekensteyn wrote:
> > > dankm wrote:
> > > > dankm wrote:
> > > > > joerg wrote:
> > > > > > This doesn't handle directory vs string prefix prefix correctly, does it? At the very least, it should have a test case :)
> > > > > Good catch. I expect not. But on the other hand, it's exactly what debug-prefix-map does :)
> > > > > 
> > > > > I'll add test cases in a future review. My first goal was getting something sort-of working.
> > > > There should be a test, but apparently the debug prefix map code also does this.
> > > > 
> > > > What do you think the correct behaviour should be? a string prefix, or a directory prefix?
> > > It should be a string prefix (like GCC)
> > I disagree. I consider it a bug in GCC that it is a string prefix. It's quite inconsistent as well.
> I agree with you, it should have been a directory prefix but GCC implements it as a string prefix although the GCC documents it as:
> "-fdebug-prefix-map=old=new When compiling files residing in **directory old**, record debugging information describing them as if the files resided in **directory new** instead."
> 
> If you decide to fix `-fmacro-prefix-map` to use a directory prefix match, then the `-fdebug-prefix-map` should also be fixed for consistency. What about implementing the (buggy) GCC-compatible behavior first and then fixing both cases in a future patch? (I don't mind when the buggy behavior is fixed, I just want to see this functionality moving forward.)
> 
> Another edge case that I ran into is when using the option to drop directories. When using `-ffile-prefix-map=/src=`, the command `cd /src && cc /src/foo.c` would have `__FILE__` equal to `/foo.c`. As a native "fix", one would try `-ffile-prefix-map=/src/=` which indeed produces `__FILE__` equal to `foo.c`.
> 
> Matching with a trailing slash however fails to correctly remap some debug information, namely `DW_AT_comp_dir`. This contains the working directory (`/src`) which is not matched by `/src/`. By using a proper directory prefix match, this would be nicely fixed.
Yes, I'm going to submit my code with tests, and hoist the prefix remapping (for debug-prefix-map and macro-prefix-map) into a common location. Most probably part of Path.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49466/new/

https://reviews.llvm.org/D49466