-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Contents of the GMF from clangd's preamble will still have ODR checks run #2094
Comments
(Also apologies if this should have gone on |
This is slightly odd to me since I've skipped the ODR check in my branch: ChuanqiXu9/clangd-for-modules@13b00ff |
I started testing this a few days ago and might not have that change in if it's actually 2 days old 😅 That being said off the top of my head I think this issue would still persist? Because the problem isn't that skipping ODR checks in the GMF is off, it's that the stuff in the prelude isn't getting detected as being part of the GMF in the first place. But I also wrote this after staring at the source code for like, 6 hours straight, so I could be a bit off base. |
Maybe we'd best to handle this in clang. See the comments of llvm/llvm-project@c184b94 for details. |
So I tested on the latest
|
Yeah, that is somewhat a fundamental problem. Let me see if there is any workarounds... |
The compatibility of named modules and PCH and header like modules are always in a dark corner case. We feel it is not compatible. But due to (seems) no one do that in practice. We don't know any problems yet. Until we tried to support modules in clangd, where the PCHs are automatically used. This looks fundamental or we have to disable PCH in module units. But it is a long term topic. I tried to work around it in llvm/llvm-project@2f0910d. I think you can try again. |
I filed llvm/llvm-project#99612. You can look at it if you're interested. |
That did fix the first
If I modify if (MergedDCIt != Reader.MergedDeclContexts.end() &&
!shouldSkipCheckingODR(D) &&
!shouldSkipCheckingODR(cast<Decl>(MergedDCIt->second)) && // <-- this is new
MergedDCIt->second == D->getDeclContext())
Reader.PendingOdrMergeChecks.push_back(D); though I'm not entirely sure if this is correct. Trying to check why this was a violation (why is it saying an obviously present member is missing?) in the first place was a bit of a rabbit hole, but it led me to [ const Decl *DCanon = D->getCanonicalDecl();
// [...]
for (auto *CanonMember : CanonDef->decls()) {
if (CanonMember->getCanonicalDecl() == DCanon) {
// This can happen if the declaration is merely mergeable and not
// actually redeclarable (we looked for redeclarations earlier).
//
// FIXME: We should be able to detect this more efficiently, without
// pulling in all of the members of CanonDef.
Found = true;
break;
}
if (auto *ND = dyn_cast<NamedDecl>(CanonMember))
if (ND->getDeclName() == D->getDeclName())
Candidates.push_back(ND);
} In this case, both of these checks fail:
Anyway, the reason I'm not sure that line is correct is because changing that fixes the ODR error but results in, uhh, this mess:
If I cut out all the irrelevant lines, what remains is:
But, it gets even weirder! If I change export void PrintHelloWorld() {
std::string s;
std::cout << HelloWorld();
} then it...works? Notice the problematic line In fact, creating a std::string s;
export void PrintHelloWorld() { std::cout << HelloWorld(); } I'm not sure if this is related or a separate issue altogether? I tried debugging it but failed to come up with anything concrete. |
This may not be correct. Since it prevents merging too. But what we want is to avoid complaining about the ODR violations. I don't have time to look into the codes now. But I have a guess. Can you compile your projects with -fexperimental-modules-reduced-bmi again and use the compile commands produced from that? |
I seem to get the same results, the
|
I finally got some time to look into the codes. And it looks like your suggested change is correct. The name is misleading. It named I want to land that. But I need to reduce a minimal example for it (without any standard headers) |
Solve clangd/clangd#2094 Due clangd will enable PCH automatically, the previous mechanism to skip ODR check in GMF may be invalid. This patch fixes this for a case.
I've land this: llvm/llvm-project@ca2351d @refi64 please test again |
Quick disclaimer: I am not an expert on Clang's innards, I basically hit this issue and started exploring the source code a few weeks ago to see it was a simple fix. Unfortunately...it is not. (I think.)
Consider the following files (mostly ripped from #1892, though that issue unrelated because the includes aren't in a GMF):
mod.cc
:mod2.cc
:clangd crashes with the following stack trace:
This was a bit confusing to me, because the clang CLI reports no issues, and after llvm/llvm-project@a0b6747, ODR checks shouldn't run for these anyway, right?
As it turns out, this is because of clangd's auto-generated prelude. Submodule IDs are never written to PCH files, so the module ownership never gets set to anything other than
Unowned
, and that + the missing owner module results inisExplicitGlobalModule()
andshouldSkipCheckingODR
beingfalse
.I don't know what an ideal fix for this is. My initial thought was to, when deserializing a PCH, just detect the
ShouldSkipCheckingODR
bit and set the corresponding decls to be owned by the current module's GMF. But deserialized decls can't have a local owning module, and I'm not really comfortable with changing that or trying to wire it up to the module IDs instead.This is reproducible with @ChuanqiXu9's
clangd-with-modules
branch and #66462. Based on the stack trace, I also think that llvm/llvm-project#82180 is also the same root cause, but I'm not sure.The text was updated successfully, but these errors were encountered: