[PATCH] D87928: Provide -fsource-dir flag in Clang

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

In D87928#2394913 <https://reviews.llvm.org/D87928#2394913>, @dexonsmith wrote:

> I missed this before, and it's an interesting problem.
>
> I have a few questions:
>
> - I'm not clear on the expected interaction between the current working directory of the compiler and `-fsource-dir`. What happens if they're different?

They're completely independent, `-fsource-dir` is only used for coverage mapping and macros.

> - I'm curious if you've considered other solutions that tie the source directory more closely to the current working directory, such as a flag `-frelative-to-working-dir`, which makes things relative to the compiler's working directory. Is that too restrictive?

I think it depends on the use cases and user expectations. For example, in our case we have layout like this:

  /working/dir/
               sources/
                       lib/a/a.c
                       include/a/a.h
               out/
                       default/

Here `out/default` is the working directory so if we used `-frelative-to-working-dir`, we would end up with paths like `../../sources/lib/a/a.c` in the coverage mapping or macro expansions like `__FILE__`. While this may be acceptable for macro expansions, it's definitely undesirable in coverage mapping.

I can think of some potential solutions. We use could use `-fprofile-prefix-map=../../=` to strip the `../../` prefix. Alternatively we could introduce a new flags in tools like `llvm-cov` and `llvm-profdata` to strip the prefix during post-processing.

> - Have you looked at the similar "MakeRelative" logic in `ASTWriter.cpp`? Can this logic be shared somehow?

I wasn't aware of that bit but once we decide on the approach, I'll try to deduplicate the logic.

> - What's the expected behaviour when building modules implicitly with `-fmodules`? What value (if any) should they get for `-fsource-dir`?

Right now this flag doesn't apply to modules at all so there's no effect.

> - How should this affect the dependency output (e.g., `clang-scan-deps` and `.d` files), if at all?

The same as for modules. We could consider extending it to support modules and dependency files as well although I'd prefer to do it separately.

> - Have you considered having `-fsource-dir` add entries to the prefix maps, rather than duplicating logic?

That might be an option if we land D83154 <https://reviews.llvm.org/D83154> first. It wasn't clear if we're going to have both but it might make sense to introduce `-fprofile-prefix-map` and then build `-fsource-dir` on top.

> I am a bit concerned about mixing the concepts of "relative base path" with "source directory", since the latter sounds to me like where pristine sources live and that's not always the right relative base path.
>
> For example, consider a layout like this:
>
>   /working/dir/
>                sources/
>                        lib/a/a.c
>                        include/a/a.h
>                        include/a/a.td
>                build/
>                      include/lib/a/a.inc
>                      lib/a/a.o
>                toolchain/usr/
>                              bin/clang
>                              lib/clang/11.0/include/stdint.h
>                sdk/usr/include/
>                                stdint.h
>                                c++/v1/
>                                       vector
>                                       stdint.h
>
> It seems cleaner to have everything relative to `/working/dir`, not `/working/dir/sources`, since you can name all compiler inputs and outputs with the former. But the latter is conceptually the source directory.

Yes, I agree. Ultimately it's up to the user to decide what output they expect. I guess this is one advantage of using the `-frelative-to-working-dir` approach where it's unambiguous and user can use other flags to do additional rewriting if needed.

> WDYT?

Thanks for the detailed feedback!



================
Comment at: clang/lib/Lex/PPMacroExpansion.cpp:1545-1547
+      StringRef SourceDir = PPOpts->SourceDir;
+      if (!SourceDir.empty() && FN.startswith(SourceDir))
+        llvm::sys::path::make_relative(FN, SourceDir, FN);
----------------
dexonsmith wrote:
> What does this do with `-fsource-dir /a/b` for the file `/a/b.c`? (It doesn't seem quite right for it to turn it into `.c` or `../b.c`...)
That's definitely undesirable. We could introduce a new function like `llvm::sys::path::has_prefix` to compare path components instead pure string comparison to avoid this issue.


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

https://reviews.llvm.org/D87928