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

[dankm]

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

[dankm]

Restored original test case file names.

[dankm]

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]

Still fine by me, thanks!

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

[joerg]

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]

Enforce path mapping. This requires LLVM review D56769.

[Lekensteyn]

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]

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]

Update style.

[Lekensteyn]

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]

Undo accidental change.

[dankm]

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

[dankm]

Move trailing path separator stripping back to Clang.

[raj.khem]

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]

lib/CodeGen/CGDebugInfo.cpp

476 ↗

(On Diff #182121)

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

[Lekensteyn]

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

[manojgupta]

@dankm are you still working on this patch?

[dankm]

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.

[dankm]

In D49466#1586140, @manojgupta wrote:

@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.

[Lekensteyn]

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

174

"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".

186

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

[phosek]

@dankm do you still plan to work on this? We would really like to see this landed and we could help if needed.

[MaskRay]

It'd be nice to see this land.

[phosek]

@dankm is it OK if we take over this change to push it forward?

[dankm]

llvm/include/llvm/Support/Path.h

174

Thanks. I like your wording, and I've used it in the incoming patch.

186

Works for me. I did that to make my call sites somewhat more readable. It'll be changed too.

[dankm]

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

8

The tests will need more thinking, you're probably right, but I don't have much time to work on this at the moment. How do you feel about addressing this in the future?

[dankm]

• Address feedback from @Lekensteyn

[dankm]

Whoops. There are extra changes here I didn't mean to submit :/

[dankm]

• Address feedback from @Lekensteyn

[dankm]

In D49466#1681534, @phosek wrote:

@dankm is it OK if we take over this change to push it forward?

At this point sure. Unless it's accepted as-is now, then I don't have a commit bit to finish it off.

[dankm]

Ping?

[MaskRay]

In D49466#1760765, @dankm wrote:

Ping?

The tests need fixing... I can commit it. Now that we've migrated to the llvm monorepo, the git commit message can retain the author info properly...

[MaskRay]

Add back remapDIPath that was unintentionally deleted by D69213, caught by a test.

Small adjustment of the code

[MaskRay]

Minimize diff in Options.td
Properly rebase remapDIPath on top of D69213

[dankm]

In D49466#1760860, @MaskRay wrote:

Add back remapDIPath that was unintentionally deleted by D69213, caught by a test.

Small adjustment of the code

Right. Can't believe I missed that. Thanks.

[thakis]

There's still one failing test on Windows after the fix attempt: http://45.33.8.238/win/3052/step_6.txt

Please take a look and revert if it's not an easy fix. (And please watch bots after committing stuff.)

[MaskRay]

In D49466#1761044, @thakis wrote:

There's still one failing test on Windows after the fix attempt: http://45.33.8.238/win/3052/step_6.txt

Please take a look and revert if it's not an easy fix. (And please watch bots after committing stuff.)

XFAIL'ed clang/test/Preprocessor/file_test.c. I didn't receive an email from http://45.33.8.238/win/3052/step_6.txt not sure if that was because the author email was @dankm's, not mine. If there is a console. please tell me :)

[MaskRay]

Does this work on Windows?

--- i/clang/test/Preprocessor/file_test.c
+++ w/clang/test/Preprocessor/file_test.c
@@ -1,8 +1,7 @@
-// XFAIL: system-windows
 // RUN: %clang -E -ffile-prefix-map=%p=/UNLIKELY_PATH/empty -c -o - %s | FileCheck %s
 // RUN: %clang -E -fmacro-prefix-map=%p=/UNLIKELY_PATH/empty -c -o - %s | FileCheck %s
 // RUN: %clang -E -fmacro-prefix-map=%p=/UNLIKELY_PATH=empty -c -o - %s | FileCheck %s -check-prefix CHECK-EVIL
-// RUN: %clang -E -fmacro-prefix-map=%p/= -c -o - %s | FileCheck %s --check-prefix CHECK-REMOVE
+// RUN: %clang -E -fmacro-prefix-map=%/p/= -c -o - %/s | FileCheck %s --check-prefix CHECK-REMOVE

startswith is not ideal because /t will match /tmp. However, gcc appears to use something similar to startswith, see:

% gcc -c -g -fdebug-prefix-map=/t=x /tmp/c/a.c -o /tmp/c/a.o
% llvm-dwarfdump /tmp/c/a.o | grep -m 1 DW_AT_name
              DW_AT_name        ("xmp/c/a.c")

[MaskRay]

The ugly path separator pattern {{(/|\\\\)}} appears in 60+ tests. Can we teach clang and other tools to

1. accept both / and \ input
2. but only output /

on Windows? We can probably remove llvm::sys::path::Style::{windows,posix,native} from include/llvm/Support/Path.h and only keep the posix form.

[probinson]

In D49466#1761156, @MaskRay wrote:

The ugly path separator pattern {{(/|\\\\)}} appears in 60+ tests. Can we teach clang and other tools to

1. accept both / and \ input

In general they do, AFAIK, although it's not feasible in cases where / is the character that introduces an option, which is common on standard Windows utilities.

2. but only output /

on Windows?

This is often actually incorrect for Windows.

We can probably remove llvm::sys::path::Style::{windows,posix,native} from include/llvm/Support/Path.h and only keep the posix form.

It's highly unlikely that will be correct for all cases, and certainly will not match users' expectations. Debug info, for example, will want normal Windows syntax file paths with \.