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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
