[PATCH] D102669: [analyzer][ctu] Fix wrong 'multiple definitions' errors caused by space characters in lookup names when parsing the ctu index file

[Balázs Benics via Phabricator via cfe-commits]

steakhal reopened this revision.
steakhal added a comment.
This revision is now accepted and ready to land.

In D102669#3205889 <https://reviews.llvm.org/D102669#3205889>, @OikawaKirie wrote:

> In D102669#3199270 <https://reviews.llvm.org/D102669#3199270>, @steakhal wrote:
>
>> We shouldn't skip mac targets. I CC ASTImporter folks, they probably have an M1 <https://reviews.llvm.org/M1>.
>
> I am not intended to ignore this problem triggered on M1 <https://reviews.llvm.org/M1>. However, I think it is not this patch that leads to this problem, it just triggers it.
> I mean we can just disable the test case temporarily on M1 <https://reviews.llvm.org/M1>, and fix this problem as well as enable this patch and the one of on-demand-parsing in another patch.
> I think they trigger the same problem for the same reason on M1 <https://reviews.llvm.org/M1>.
>
> Besides, it seems to be the problem of `ASTUnit::LoadFromCommandLine`, rather than the ASTImporter.

Prior to this patch, it worked on M1 <https://reviews.llvm.org/M1>; after landing it broke something, so we clearly shouldn't land this.
We should add a test-case demonstrating the problem with M1 <https://reviews.llvm.org/M1> with a given configuration.
Then we need to track down and fix the underlying issue causing it. That should be done probably in a separate patch and add it as a parent patch to this one.

If all of these are done, we can probably land both of them.



================
Comment at: clang/test/Analysis/ctu-lookup-name-with-space.cpp:13
+// RUN:   -verify %s
+
+void importee();
----------------
OikawaKirie wrote:
> arichardson wrote:
> > OikawaKirie wrote:
> > > Adding this line here.
> > Disabling the test on non- Linux is not a good idea IMO since it means we lose coverage on other platforms. My guess is that you just need to specify an explicit triple in the clang invocations.
> AFAIK, we cannot do that. If this test case is executed on different platforms, we cannot determine the triple ahead of time and specify it in the invocation list.
If we were to pin the triple, then each platform would emit the correct AST dumps according to that platform - ~~ cross-compilation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102669