Page MenuHomePhabricator

Initial implementation of -fmacro-prefix-map and -ffile-prefix-map
AcceptedPublic

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
1460 ↗(On Diff #156003)

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
1460 ↗(On Diff #156003)

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
1460 ↗(On Diff #156003)

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?

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?

I am. I'm about to push a new review. Sorry I missed this earlier.

dankm added inline comments.Jan 10 2019, 11:36 AM
lib/Lex/PPMacroExpansion.cpp
1460 ↗(On Diff #156003)

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.

dankm updated this revision to Diff 181151.Jan 10 2019, 2:05 PM

Added unit tests for the prefix remapping.

Switched the sorting on the prefix map, so that <somepath>/sub gets remapped before <somepath> if both are specified.

I intend to do a more invasive change after this review to unify path prefix remapping.

alxu added a comment.Jan 10 2019, 2:41 PM

FYI, according to my comment on D49652, assuming I checked it correctly, gcc applies the maps in reverse order of command line specification, not sorted order. It seems unlikely that anyone is actually depending on the order though.

dankm added a comment.Jan 11 2019, 6:33 AM

FYI, according to my comment on D49652, assuming I checked it correctly, gcc applies the maps in reverse order of command line specification, not sorted order. It seems unlikely that anyone is actually depending on the order though.

Yeah, I noticed that, but it appears to be undefined by GCC's documentation. I agree with review D49652, but I also want to get this in before 8.0 branches, even if it's not ideal.

Right now we apply them in strict alphabetical order, switching to reverse order lets one map /objdir/<sysroot> to / while remapping /objdir/ to /some/other/dir, which is my use case.

joerg added a comment.Jan 11 2019, 6:55 AM

That's the other reason why I find the GCC specification as string prefix confusing. I still say we should just go with mapping of path names and then the order question mostly goes away.

It would be nice to have this for Clang 8.0, the branch date is within 5 days :)

lib/Driver/ToolChains/Clang.cpp
617 ↗(On Diff #181151)

For clang -ffile-prefix-map=foo, wouldn't this report invalid argument 'foo' to -fdebug-prefix-map? If so, perhaps some method of A or A->getOption() can be used?

630 ↗(On Diff #181151)

Same concern here about -ffile-prefix-map=foo showing an error message about -fmacro-prefix-map.

test/Preprocessor/file_test.c
5 ↗(On Diff #181151)

Any reason to keep this comment?

dankm marked 2 inline comments as done.Jan 11 2019, 8:38 AM

It would be nice to have this for Clang 8.0, the branch date is within 5 days :)

Yup, that's why I'm ignoring a new baby for this :)

lib/Driver/ToolChains/Clang.cpp
617 ↗(On Diff #181151)

Yes, it would seem so. It looks like A->getOption().getName() can be used.

test/Preprocessor/file_test.c
5 ↗(On Diff #181151)

Ha. No. That's from when I started writing this test. It can go away.

dankm updated this revision to Diff 181293.Jan 11 2019, 9:03 AM

Made diagnostics for file-prefix-map display the actual option name.

Could you add more tests to check the error message for bad options (missing =):

-fdebug-prefix-map=bad
-fmacro-prefix-map=bad
-ffile-prefix-map=bad

FWIW, GCC emits two errors for -ffile-prefix-map=bad.

Another edge case is -ffile-prefix-map==foo/, GCC currently uses this to prepend foo/ to every path. Not sure if that is intentional, but that is the current behavior (one which is also replicated by this patch I believe).

Could you also mark review comments that are completed as "done"? It should make the diff easier to read (I hope) :)

include/clang/Basic/DiagnosticDriverKinds.td
118 ↗(On Diff #181293)

Maybe rename _to_prefix_map to _to_option? (And maybe swap the order of parameters so %0 comes before %1?)

dankm updated this revision to Diff 181363.Jan 11 2019, 1:28 PM

renamed err_drv_invalid_argument_to_prefix_map to err_drv_invalid_argument_to_option
added more frontend tests for macro-prefix-map and file-prefix-map.

dankm marked 4 inline comments as done.Jan 11 2019, 1:33 PM

Could you add more tests to check the error message for bad options (missing =):

-fdebug-prefix-map=bad
-fmacro-prefix-map=bad
-ffile-prefix-map=bad

Some more got added with the latest diff

FWIW, GCC emits two errors for -ffile-prefix-map=bad.

Yes, this does too. It looked odd to me, but it's not a huge deal.

Another edge case is -ffile-prefix-map==foo/, GCC currently uses this to prepend foo/ to every path. Not sure if that is intentional, but that is the current behavior (one which is also replicated by this patch I believe).

Yes, with this patch it does that for file-prefix-map and macro-prefix-map. It already did that (sort-of) for debug-prefix-map, but seems to add it twice for some debugging information, but I'll fix that later since it's done that since at least version 5.0.

Could you also mark review comments that are completed as "done"? It should make the diff easier to read (I hope) :)

Yes, I tried to do that with this comment. I'm new to phabricator.

Except one thing, it looks reasonable to me. I'll try to run some tests and report back tomorrow.

(Not very familiar with Phabricator either. I still see some comments, hopefully the "Collapse" function does something useful here.)

test/Driver/prefix-map.S
7 ↗(On Diff #181363)

Maybe restore the old file name (debug-prefix-map.S) since this still tests the debug prefix functionality? And otherwise this comment needs to be updated.

Lekensteyn accepted this revision.Jan 12 2019, 4:52 PM

Tests pass here, using it on a large CMake project with a CMAKE_BUILD_TYPE=Debug and c/cxxflags -ffile-prefix-map=$builddir= -ffile-prefix-map=$srcdir/= -fuse-ld=lld successfully strips all traces of $builddir and $srcdir.

If you could take care of the previous comment (undo the rename or rename debug-prefix.map.c), then I've no further comments.

If @joerg or someone else could give the final review/pass, that would be great :)

lib/Driver/ToolChains/Clang.cpp
612 ↗(On Diff #156003)

Wouldn't using if (...) { D.diag(...); continue; } also skip the A->claim() call? Presumably that could result in spurious errors as well about unused arguments?

This revision is now accepted and ready to land.Jan 12 2019, 4:52 PM
dankm updated this revision to Diff 181562.Jan 14 2019, 8:07 AM

Restored original test case file names.

dankm marked 3 inline comments as done.Jan 14 2019, 8:09 AM
dankm added inline comments.
lib/Driver/ToolChains/Clang.cpp
612 ↗(On Diff #156003)

It would, and did. I had @joerg's suggestion in an earlier patch on this review.

Lekensteyn accepted this revision.Jan 14 2019, 8:25 AM

Still fine by me, thanks!

As for the commit message, perhaps reference:
https://bugs.llvm.org/show_bug.cgi?id=38135

joerg added a comment.Jan 15 2019, 4:41 AM

As discussed with dankm on IRC, I still would like to see the correct behavior going into 8.0, i.e. not change it later. Since this also matters for potential faster implementations later, it seems like a good idea to do it now. The changes are well-localized.

(1) Do path prefix matching and not string prefix matching. The difference is that the file name must be longer than the prefix and the prefix must be followed by a path separator.
(2) The longest prefix match wins. Substituation is applied only once per file name, independent of the rules. This gives more predictable output and allows switching to a tree-lookup later.

dankm updated this revision to Diff 181964.Jan 15 2019, 7:23 PM
dankm marked an inline comment as done.

Enforce path mapping. This requires LLVM review D56769.

Changes still look reasonable, but the preceding path (https://reviews.llvm.org/D56769) needs some work.

lib/CodeGen/CGDebugInfo.cpp
607 ↗(On Diff #181964)

Any reason for dropping remapDIPath here? Wouldn't this result in the full path being included even when using:

clang -fdebug-prefix-map=/full/path/= /full/path/source.c
lib/Lex/PPMacroExpansion.cpp
1466 ↗(On Diff #181964)

Style: space between if and (

dankm marked an inline comment as done.Jan 16 2019, 6:49 AM
I'll update the style nit, and spend some non-tired time on the string remapping. Thanks
lib/CodeGen/CGDebugInfo.cpp
607 ↗(On Diff #181964)

Whoops. That probably shouldn't have been included this round. This is a bugfix. MainFileName is already remapped from earlier in this function, this keeps it from remapping twice if you have an empty old prefix.

dankm updated this revision to Diff 182037.Jan 16 2019, 6:51 AM

Update style.

Lekensteyn added inline comments.Jan 16 2019, 7:30 AM
lib/CodeGen/CGDebugInfo.cpp
607 ↗(On Diff #181964)

The remapping was done here:

c
  std::string MainFileDir;
  if (const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID())) {
    MainFileDir = remapDIPath(MainFile->getDir()->getName());

(Observation: the declaration could probably be moved inside the if block since it is not used outside.)

What about the second case though? For example, assume /tmp/testdir/mytest.ii:

# 1 "/tmp/mytest.c"
# 1 "<built-in>"
# 1 "<command-line>"
# 31 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 32 "<command-line>" 2
# 1 "/tmp/mytest.c"
int main(int argc, const char *argv[])
{
    return 0;
}

What happens if you now compile with clang -fdebug-prefix-map=/tmp/=/bla/ /tmp/testdir/mytest.ii from /tmp/testdir?

Unless this affects the current patch, consider moving it to a separate change.

dankm updated this revision to Diff 182047.Jan 16 2019, 7:44 AM

Undo accidental change.

dankm marked an inline comment as done.Jan 16 2019, 7:45 AM

Sure, I'll (eventually) make a separate review.

dankm updated this revision to Diff 182121.Jan 16 2019, 12:32 PM

Move trailing path separator stripping back to Clang.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 10:00 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
raj.khem added inline comments.Feb 15 2019, 11:27 AM
lib/CodeGen/CGDebugInfo.cpp
476 ↗(On Diff #182121)

looking at llvm/lib/Support/Path.cpp replace_path_prefix() returns void but here inside if() it will expect a bool return value

raj.khem added inline comments.Feb 15 2019, 11:34 AM
lib/CodeGen/CGDebugInfo.cpp
476 ↗(On Diff #182121)

nm I guess I needed to look into https://reviews.llvm.org/D56769 as well.

Hi @dankm, any progress on this feature? The proposed branch off date for Clang 9.0.0 is 18 July 2019: https://lists.llvm.org/pipermail/cfe-dev/2019-June/062628.html

E5ten added a subscriber: E5ten.Jul 3 2019, 9:57 PM

@dankm are you still working on this patch?

dankm updated this revision to Diff 212723.Wed, Jul 31, 9:36 PM

Latest changes. I've been sitting on these for months, so I don't remember all that changed. The path remapping contract changed somewhat, and it's now based on the git monorepo.

Herald added a project: Restricted Project. · View Herald TranscriptWed, Jul 31, 9:36 PM
dankm added a comment.Wed, Jul 31, 9:37 PM

@dankm are you still working on this patch?

Yes, I've been afk for a bit due to family circumstances, but I just uploaded more.

Thanks for picking this up again. I've left some nitpicks below in a quick review.

The "strict" parameter is not precisely defined, if that is fixed I think this would be ready for merge.

clang/test/Driver/debug-prefix-map.c
8

What about combining these two tests? The command is the same, maybe you could have a new -check-prefix to reduce the number of invocations? Likewise for the cases below.

llvm/include/llvm/Support/Path.h
172

"strict checking" is ambiguous on its own. What about something like:

If strict is true, a directory separator following \a OldPrefix will also be stripped. Otherwise, directory separators will only be matched and stripped when present in \a OldPrefix.

Or whatever semantics you would like to assign to "strict mode".

181

Why have a variant with the parameters swapped, is it common in LLVM to have such convenience wrappers?

Why not require callers to pass Style::native whenever they want to modify "strict"?

llvm/lib/Support/Path.cpp
512

this condition is duplicated above