Page MenuHomePhabricator

Initial implementation of -fmacro-prefix-map and -ffile-prefix-map
Needs RevisionPublic

Authored by dankm on Jul 17 2018, 8:51 PM.

Details

Summary

GCC 8 implements -fmacro-prefix-map. Like -fdebug-prefix-map, it replaces a string prefix for the FILE macro.
-ffile-prefix-map is the union of -fdebug-prefix-map and -fmacro-prefix-map

Diff Detail

Event Timeline

dankm created this revision.Jul 17 2018, 8:51 PM
dankm added a reviewer: rsmith.
erichkeane added inline comments.Jul 18 2018, 10:32 AM
include/clang/Lex/Preprocessor.h
155 ↗(On Diff #156003)

Scrolling back up, put this implementation as close to the debug implementation as you can, they are so ridiculously related that having them far enough apart that future changes could cause them to divert is troublesome to me.

include/clang/Lex/PreprocessorOptions.h
171

It seems this can be StringRefs as well.

lib/Driver/ToolChains/Clang.cpp
609–610

filtered can take multiple options, you should be able to not add anything here except adding OPT_ffile_prefix_map_EQ to the filtered line, plus a ternary in the Diag line.

636

See advice above.

Additionally/alternatively, I wonder if these two functions could be trivially combined.

1243

Is there a good reason for this to not live with the call to addDebugPrefixMapArg?

lib/Frontend/CompilerInvocation.cpp
2850

Again, this is so much like the debug-prefix otpion, it should probably just be right next to it.

Additionally, we don't use curley brackets on single-line bodies.

lib/Lex/PPMacroExpansion.cpp
1458

make MacroPrefixMap const.

1530

This change shouldn't be necessary, SmallString is still likely the right answer here.

lib/Lex/Preprocessor.cpp
160 ↗(On Diff #156003)

I'm unconvinced that this is necessary. ExpandBuiltinMacro is in Preprocessor, so it has access to PPOpts directly.

joerg added inline comments.Jul 18 2018, 1:44 PM
lib/Driver/ToolChains/Clang.cpp
609

I find it confusing that -ffile_prefix_map implies -fdebug-prefix-map. I'm not sure that is desirable in every case. It seems better to have a combined option that explicitly does both.

612

I'd prefer the bailout style here, i.e. if (...) { D.diag(...); continue }

636

Or at least the for loop could be refactored into a small helper function that takes the option name, output option and error as argument.

lib/Lex/PPMacroExpansion.cpp
1461

This doesn't handle directory vs string prefix prefix correctly, does it? At the very least, it should have a test case :)

dankm added inline comments.Jul 18 2018, 2:51 PM
lib/Driver/ToolChains/Clang.cpp
609

-ffile-prefix-map is the combined option. -fmacro-prefix-map is the preprocessor option, and -fdebug-prefix-map is the codegen option.

636

Good ideas. I'll look into them.

1243

Mostly because this is the function that adds preprocessor specific options. There's no other reason why it couldn't be done alongside addDebugPrefixMapArg in this file.

lib/Frontend/CompilerInvocation.cpp
2850

This is here because it's a preprocessor option. This function handles those. The DebugPrefixMap handling is a codegen option.

lib/Lex/PPMacroExpansion.cpp
1458

I shall.

1461

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.

1530

I tried that long ago. It didn't work, I don't remember exactly why. But I agree that SmallString should be enough. I'll dig more.

lib/Lex/Preprocessor.cpp
160 ↗(On Diff #156003)

It has access to PPOpts, but the implementation file doesn't have a full definition of PreprocessorOptions. I could add that to this file, then this becomes redundant.

dankm updated this revision to Diff 156164.Jul 18 2018, 3:11 PM
dankm retitled this revision from Initial implementation of -fmacro-prefix-map and -ffile-prefix-map to Initial implementation of -fmacro-prefix-mapand -ffile-prefix-map.

Address some of the comments by erichkeane and joerg.

erichkeane added inline comments.Jul 19 2018, 6:17 AM
include/clang/Basic/DiagnosticDriverKinds.td
118

Since these are otherwise identical, perhaps a %select{...|...} for the flag name?

include/clang/Lex/PreprocessorOptions.h
171

Did you miss this one? Or is there a good reason these cannot be stringrefs?

lib/Driver/ToolChains/Clang.cpp
611–612

With the continue above, 'else' is unnecessary/against coding standard.

644

See above.

lib/Lex/PPMacroExpansion.cpp
1457

Did clang-format do this formatting? It looks REALLY weird...

1530

Just noting to handle this before approval.

1533

First, comments end in a period. Second, isn't that what the next line does?

dankm added inline comments.Jul 19 2018, 8:41 AM
include/clang/Lex/PreprocessorOptions.h
171

I didn't miss it. StringRefs here don't survive. The function that adds them to the map creates temporary strings, that go away once that function ends causing StringRefs to dangle. std::string keeps copies.

lib/Driver/ToolChains/Clang.cpp
611–612

Next diff will have that.

lib/Lex/PPMacroExpansion.cpp
1457

No, that's my text editor. I'll fix it.

1461

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?

1530

Yup, with some changes to remapMacroPath SmallString works fine.

1533

Yes, old comment is old ;)

erichkeane added inline comments.Jul 19 2018, 8:56 AM
include/clang/Lex/PreprocessorOptions.h
171

Oh! I hadn't realized that getAllArgValues gives a vector<string>. That is actually pretty odd for our codebase. Looking into it, there is no reason that function cannot return a vector of StringRef...

Alright, at one point someone should likely fix that, but that person should change this type.

dankm updated this revision to Diff 156295.Jul 19 2018, 9:40 AM
dankm retitled this revision from Initial implementation of -fmacro-prefix-mapand -ffile-prefix-map to Initial implementation of -fmacro-prefix-map and -ffile-prefix-map.
dankm edited the summary of this revision. (Show Details)
alxu added a subscriber: alxu.Jul 23 2018, 2:08 PM

FYI, gcc uses the stupid and bad string prefix matching approach. if clang supports the same option, it should have the same behavior. you could decide that it's a bad idea, but then the option should be called something else, otherwise people will have to go back to testing compiler name and versions again which is :(.

rnk accepted this revision.Jul 27 2018, 4:43 PM

lgtm

This revision is now accepted and ready to land.Jul 27 2018, 4:43 PM
emaste added a subscriber: emaste.Sep 13 2018, 12:43 PM
Godin added a subscriber: Godin.Sep 13 2018, 4:05 PM
Lekensteyn requested changes to this revision.Oct 1 2018, 9:23 AM
Lekensteyn added a subscriber: Lekensteyn.

The functionality looks correct to me, but could you include some tests in test/Driver/ and test/Preprocessor/ just to be sure?
test/Driver/debug-prefix-map.c and test/CodeGen/debug-prefix-map.c could serve as inspiration.

The documentation should probable be updated too: docs/ClangCommandLineReference.rst

(It would be nice to have this feature for Reproducible Builds)

lib/Lex/PPMacroExpansion.cpp
1461

It should be a string prefix (like GCC)

This revision now requires changes to proceed.Oct 1 2018, 9:23 AM
joerg added inline comments.Oct 1 2018, 11:38 AM
lib/Lex/PPMacroExpansion.cpp
1461

I disagree. I consider it a bug in GCC that it is a string prefix. It's quite inconsistent as well.

Lekensteyn added inline comments.Oct 1 2018, 1:13 PM
lib/Lex/PPMacroExpansion.cpp
1461

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.

PostgreSQL 11 is now using LLVM to do JITing of SQL expressions. We'd need this feature to strip the build directory off the .bc bitcode files so the .deb packages build reproducibly.
@dankm: Are you still working on this? What can we do to help getting this move forward?