michael-jabbour-sonarsource wrote:
Hello @compnerd, and thank you for sharing the information and for the prompt
response.
> Without this change, a file on a substituted drive will have two separate
> paths.
I am not sure I understand this part. For me, a file on a substituted drive (or
under a symlink) may always have multiple different valid paths to reference
it. What is important, is that it always has a single canonical path. This is
the reason I don’t fully see “root-preserving canonicalization” as a form of a
“canonical path” anymore. To summarize my concerns:
1. The patch seems to work slightly differently from what is described in the
patch description:
1. It doesn't only avoid resolving the mapped drive as claimed, but it also
skips the symlink resolution on mapped drives (it only removes dots in that
case).
2. The impact above is not only limited to substituted/mapped drives, but
to any case where the roots are not identical (for example, also in cross-drive
links).
2. It affects clang’s usability as a library. I feel that such fallback should
be applied on a higher-level API, and ideally should impact only lit tests (as
they are the motivation stated in the patch). For example, by looking at usages
in clang, I could find that many of them still assume that
`FileManager::getCanonicalName` resolves symlinks (at least by looking at the
comments):
1. clangd’s `SourceCode::getCanonicalPath` which claims to resolve symlinks
in its docs. IIUC, it no longer does so if the code is checked out in a
substituted drive (see
[here](https://github.com/llvm/llvm-project/blob/47c892a49136c68425e7ade08553598e63ef4e70/clang-tools-extra/clangd/SourceCode.cpp#L533)).
2. Inside clang itself, there is code that handles directory resolution in
module map and framework header search (for example
[here](https://github.com/llvm/llvm-project/blob/47c892a49136c68425e7ade08553598e63ef4e70/clang/lib/Lex/HeaderSearch.cpp#L573)
and
[here](https://github.com/SonarSource/llvm-project/blob/d84668d2b2ec28a5d6f81e6eb22a17b014a000cf/clang/lib/Lex/ModuleMap.cpp#L449)).
This may not impact Windows users, but I am not entirely sure.
3. Skipping symlink resolution makes it possible for `getCanonicalName` to
return different paths for the same file (breaking the canonical property,
which can impact users of clang as a library, depending on how they use it).
Basically, we have concerns about the correctness of all existing usages of
`getCanonicalName` (inside and outside clang). It also impacted us as users of
clang as a library. If the motivation is only to fix lit tests, I can see a few
alternatives:
1. Try to apply such a fix on a higher-level API where needed, to avoid
unexpected impact? I didn't try to look for such an API myself yet. Do you know
if this been explored while working on the previous patch?
2. Switch to a solution that adapts the test as needed without altering
functionality (e.g. prior to what [was explored after this
comment](https://reviews.llvm.org/D154130#4515403))
3. Revert the patch, and switch to a user-specific workaround which does not
require changing LLVM (as suggested in this PR). Note that the patch, not only
alters the behavior of clang, but also breaks the lit setup for some other
developers on Windows (see [here](https://reviews.llvm.org/D154130#4647550)).
Any thoughts about this are appreciated.
Best regards,
Michael
https://github.com/llvm/llvm-project/pull/139739
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
