[clang-repl] Extend the C support. #89804
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Vassil Vassilev (vgvassilev) ChangesThe IdResolver chain is the main way for C to implement lookup rules. Every new partial translation unit caused clang to exit the top-most scope which in turn cleaned up the IdResolver chain. That was not an issue for C++ because its lookup is implemented on the level of declaration contexts. This patch keeps the IdResolver chain across partial translation units maintaining proper C-style lookup infrastructure. Full diff: https://github.com/llvm/llvm-project/pull/89804.diff 3 Files Affected:
diff --git a/clang/lib/Interpreter/IncrementalParser.cpp b/clang/lib/Interpreter/IncrementalParser.cpp
index ef90fe9e6f5451..f1cb5fc870eb94 100644
--- a/clang/lib/Interpreter/IncrementalParser.cpp
+++ b/clang/lib/Interpreter/IncrementalParser.cpp
@@ -387,8 +387,7 @@ std::unique_ptr<llvm::Module> IncrementalParser::GenModule() {
void IncrementalParser::CleanUpPTU(PartialTranslationUnit &PTU) {
TranslationUnitDecl *MostRecentTU = PTU.TUPart;
- TranslationUnitDecl *FirstTU = MostRecentTU->getFirstDecl();
- if (StoredDeclsMap *Map = FirstTU->getPrimaryContext()->getLookupPtr()) {
+ if (StoredDeclsMap *Map = MostRecentTU->getPrimaryContext()->getLookupPtr()) {
for (auto &&[Key, List] : *Map) {
DeclContextLookupResult R = List.getLookupResult();
std::vector<NamedDecl *> NamedDeclsToRemove;
@@ -407,6 +406,16 @@ void IncrementalParser::CleanUpPTU(PartialTranslationUnit &PTU) {
}
}
}
+
+ // FIXME: We should de-allocate MostRecentTU
+ for (Decl *D : MostRecentTU->decls()) {
+ if (!isa<NamedDecl>(D))
+ continue;
+ // Check if we need to clean up the IdResolver chain.
+ NamedDecl *ND = cast<NamedDecl>(D);
+ if (ND->getDeclName().getFETokenInfo())
+ getCI()->getSema().IdResolver.RemoveDecl(ND);
+ }
}
llvm::StringRef IncrementalParser::GetMangledName(GlobalDecl GD) const {
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 452e00fa32b102..2a0f73b42d3088 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2282,7 +2282,8 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) {
// Remove this name from our lexical scope, and warn on it if we haven't
// already.
- IdResolver.RemoveDecl(D);
+ if (!PP.isIncrementalProcessingEnabled())
+ IdResolver.RemoveDecl(D);
auto ShadowI = ShadowingDecls.find(D);
if (ShadowI != ShadowingDecls.end()) {
if (const auto *FD = dyn_cast<FieldDecl>(ShadowI->second)) {
diff --git a/clang/test/Interpreter/execute.c b/clang/test/Interpreter/execute.c
new file mode 100644
index 00000000000000..44a3a32c930112
--- /dev/null
+++ b/clang/test/Interpreter/execute.c
@@ -0,0 +1,21 @@
+// REQUIRES: host-supports-jit
+// UNSUPPORTED: system-aix
+
+// RUN: cat %s | clang-repl -Xcc -xc -Xcc -Xclang -Xcc -verify | FileCheck %s
+// RUN: cat %s | clang-repl -Xcc -xc -Xcc -O2 -Xcc -Xclang -Xcc -verify| FileCheck %s
+int printf(const char *, ...);
+int i = 42; err // expected-error{{use of undeclared identifier}}
+int i = 42;
+struct S { float f; struct S *m;} s = {1.0, 0};
+// FIXME: Making foo inline fails to emit the function.
+int foo() { return 42; }
+void run() { \
+ printf("i = %d\n", i); \
+ printf("S[f=%f, m=0x%llx]\n", s.f, (unsigned long long)s.m); \
+ int r3 = foo(); \
+}
+run();
+// CHECK: i = 42
+// CHECK-NEXT: S[f=1.000000, m=0x0]
+
+%quit
|
vgvassilev
force-pushed
the
clang-repl-c
branch
from
ade17b8
to
50fa9c9
Compare
|
The pre-merge windows test failure in |
|
@AaronBallman, can you take a look at that patch, hopefully to move forward as seems the other reviewers are busy. |
vgvassilev
force-pushed
the
clang-repl-c
branch
2 times, most recently
from
72d655c
to
7acf367
Compare
|
The bots fail on windows due to |
The IdResolver chain is the main way for C to implement lookup rules. Every new partial translation unit caused clang to exit the top-most scope which in turn cleaned up the IdResolver chain. That was not an issue for C++ because its lookup is implemented on the level of declaration contexts. This patch keeps the IdResolver chain across partial translation units maintaining proper C-style lookup infrastructure.
| auto *ND = dyn_cast<NamedDecl>(D); | ||
| if (!ND) | ||
| continue; | ||
| // Check if we need to clean up the IdResolver chain. | ||
| if (ND->getDeclName().getFETokenInfo()) |
There was a problem hiding this comment.
As a one-liner rather than a four-liner.
if (auto *ND = dyn_cast<NamedDecl>(D);
ND && ND->getDeclName().getFETokenInfo())
There was a problem hiding this comment.
It always good to take rather than leave ;) I haven't got used to this syntax and I'd leave it :(
|
The failures on windows are due to "out of heap" error. Let's land this to test on the "real" bots. |
|
We've started seeing crashes in Fuchsia's Mac tooclhain's LLDB test suite: This is the only PR in the blamelist that looks plausible, but there isn't a clear connection to me (or a clear reason why it would be Mac-only). Does anything jump out, or am I barking up the wrong tree? EDIT: Blamelist: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-mac-x64/b8747305520511321169/blamelist Stack trace follows: |
This reverts commit 253c28f. This commit is causing failures on the lldb CI bots, e.g. https://ci.swift.org/view/all/job/llvm.org/view/LLDB/job/as-lldb-cmake/4307/ On my local macOS desktop build, ``` bin/lldb-dotest -p TestImportBuiltinFileID.py Assertion failed: (D->getLexicalDeclContext() == this && "Decl inserted into wrong lexical context"), function addHiddenDecl, file DeclBase.cpp, line 1692. 6 libsystem_c.dylib 0x0000000185f0b8d0 abort + 128 7 libsystem_c.dylib 0x0000000185f0abc8 err + 0 8 liblldb.19.0.0git.dylib 0x00000001311e5800 clang::DeclContext::addHiddenDecl(clang::Decl*) + 120 9 liblldb.19.0.0git.dylib 0x00000001311e5978 clang::DeclContext::addDecl(clang::Decl*) + 32 10 liblldb.19.0.0git.dylib 0x000000012f617b48 clang::Sema::ActOnStartTopLevelStmtDecl(clang::Scope*) + 64 11 liblldb.19.0.0git.dylib 0x000000012eaf76c8 clang::Parser::ParseTopLevelStmtDecl() + 208 12 liblldb.19.0.0git.dylib 0x000000012ec051fc clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) + 3412 13 liblldb.19.0.0git.dylib 0x000000012ec03274 clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) + 2020 14 liblldb.19.0.0git.dylib 0x000000012eaca860 clang::ParseAST(clang::Sema&, bool, bool) + 604 15 liblldb.19.0.0git.dylib 0x000000012e8554c0 clang::ASTFrontendAction::ExecuteAction() + 308 16 liblldb.19.0.0git.dylib 0x000000012e854c78 clang::FrontendAction::Execute() + 124 17 liblldb.19.0.0git.dylib 0x000000012e76dcfc clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 984 18 liblldb.19.0.0git.dylib 0x000000012e784500 compileModuleImpl(clang::CompilerInstance&, clang::SourceLocation, llvm::StringRef, clang::FrontendInputFile, llvm::StringRef, llvm::StringRef, llvm::function_ref<void (clang::CompilerInstance&)>, llvm::function_ref<void (clang::CompilerInstance&)>)::$_1::operator()() const + 52 ``` Reverting until Vassil has a chance to look int oit.
|
I reverted in to unblock the CI bots until Vassil has a chance to look at it. |
Thank you! I suspect that this is due to the changes in SemaDecl.cpp (https://github.com/llvm/llvm-project/pull/89804/files#diff-edac6256ac508912a16d0165b2f8cf37123dc2f40a147dca49a34c33f1db13ddR2288) I am trying to build lldb but if you have a hint how to reproduce it outside lldb would be great! |
|
@jasonmolenda, I am stuck. I could not find how the bot configures llvm and lldb. I built lldb and ran |
|
I was able to verify that the revert fixed the tests at least. |
I tried on a mac machine. Can I get access to that bot then? |
I am pretty sure that the failure ha to do with the incremental processing if-stmt in SemaDecl. I can provide a fix that does not trigger this for lldb but that would be a workaround. I suspect that change uncovers some inconsistency in either clang or lldb which I think we should fix. |
|
I did have a means to reproduce this locally, but it's been a long time since I've used it. I should be able to recreate the setup. I'll get back to you with the details when I can; this whole afternoon is booked through pretty solid with meetings for me I'm afraid. EDIT: It would be a ~10 step process involving installing a bunch of Chrome tools like cipd, cas, etc. I think everything should be open source and accessible, but it's still not very straightforward. The intent is to build a version of LLVM that is kept as hermetic as possible from the host system, so there's a lot that goes into that. |
|
Ok, maybe I can you test this patch for a temporary fix until we figure out what's going wrong. diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 7f6921ea22be..d771a060f305 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2285,7 +2285,7 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) {
// Partial translation units that are created in incremental processing must
// not clean up the IdResolver because PTUs should take into account the
// declarations that came from previous PTUs.
- if (!PP.isIncrementalProcessingEnabled())
+ if (!PP.isIncrementalProcessingEnabled() && !getLangOpts().IncrementalExtensions)
IdResolver.RemoveDecl(D);
// Warn on it if we are shadowing a declaration. |
This reverts commit dfdf1c5.
|
This still exhibits the same issue: I do think it should be possible to download a "source tarball" that allows easily reproducing the output of our bots on similar host configurations, but I don't think we're quite there yet. It's an AI on our end, for sure, especially since we often catch real issues upstream that other bots don't, owing to our unusual build. I asked around what people were doing, and it involved basically replicating the bot build by hand using a ton of undocumented knowledge and manual editing; it's not something that I could export any easier than it would be to create a better way to do this. |
|
fwiw if you have access to a mac, I don't think it's hard to build lldb+clang? I have swig, cmake, and ninja installed on my mac via homebrew. |
|
Just found out this keeps popping up from my notification, and I would like give my 2 cents, maybe can provide some insights: Looking at the backtrace of the failing tests, I noticed we ran into The other thing I noticed is that the failure looks like have something to do with the objc, can we also check the current language we're compiling when skipping removing the name for the lexical scope? |
Does not seem to be reproducing it for me: |
Hm, ok. I remember some discussions revolving around this but got the impression lldb turns that off. I think having it on is the right thing to do but that explains why my workaround did not work.
I am hoping to cover objc, too. That's why I am really interested in figuring out what is going on. Otherwise if we limit the feature to c/c++ maybe we will get away with the bot. I still want to see why it crashes though... |
|
@mysterymath, @jasonmolenda, ping -- I am still stuck in reproducing this issue. |
|
I set aside my current work, updated to current main sources, cherry-picked 253c28f, built it on my macOS system, did I'm not sure what you're asking help with, I don't know much of anything about how the clang setup in lldb is done - I happened to be watching the CI bots and saw they all started failing and reverted the change. You couldn't reproduce this behavior on a macOS system? it is an arm64 macOS system, I didn't try repo'ing on an Intel box. Earlier on this PR you mused about making this change in SemaDecl.cpp the test case still crashes for me with that change. |
This reverts commit dfdf1c5.
Original commit message:" [clang-repl] Extend the C support. (#89804) The IdResolver chain is the main way for C to implement lookup rules. Every new partial translation unit caused clang to exit the top-most scope which in turn cleaned up the IdResolver chain. That was not an issue for C++ because its lookup is implemented on the level of declaration contexts. This patch keeps the IdResolver chain across partial translation units maintaining proper C-style lookup infrastructure. " It was reverted in dfdf1c5 because it broke the bots of lldb. This failure was subtle to debug but the current model does not work well with ObjectiveC support in lldb. This patch does cleans up the partial translation units in ObjectiveC. In future if we want to support ObjectiveC we need to understand what exactly lldb is doing when recovering from errors...
|
@jasonmolenda, I struggled quite a bit in reproducing the test failures and everything but I have a fix. Thanks a lot for your time and I hope the fix works! |
|
The reland d999ce0 makes the clang reject the valid code now: Can you take a look? |
|
@hokein, ah, that's annoying. Can you provide the entire proprocessed file that does not work? I'd like to bisect and debug. |
|
I realize I do not entirely understand the role of the IdResolver chain in c++. Perhaps we are better of changing this line to: if (!PP.isIncrementalProcessingEnabled() || getLangOpts().ObjC)
IdResolver.RemoveDecl(D);to if (!PP.isIncrementalProcessingEnabled() || !getLangOpts().C)
IdResolver.RemoveDecl(D);This way will limit the scope to the actual intent without assuming the IdResolver changes are irrelevant for languages such as C++. Can you test if that works? |
|
Thanks for the prompt response. I think limiting it to C-only will fix the issue (note that there is no |
Makes sense. If that works for you, we can check this in. I want to somehow record this breakage in the form of a test for our future selves when decide to revisit this workaround-looking code. Out of curiosity, in what context you use |
The code snippet I provided is extracted from our internal test. We have an internal clang-tool (with the |
I am a bit overwhelmed right now, are you willing to open a PR with that change so we can merge it -- otherwise I could look later this week :( |
This is the processed file https://gist.github.com/hokein/e4a5881329c3956494afa2de7d350476.
Sure, I will prepare one. |
The incremental processing mode doesn't seem to work well for C++, see the llvm#89804 (comment) for details. This patches in llvm#94471 as well as applies some followup comments.
The incremental processing mode doesn't seem to work well for C++, see the #89804 (comment) for details.
The IdResolver chain is the main way for C to implement lookup rules. Every new partial translation unit caused clang to exit the top-most scope which in turn cleaned up the IdResolver chain. That was not an issue for C++ because its lookup is implemented on the level of declaration contexts.
This patch keeps the IdResolver chain across partial translation units maintaining proper C-style lookup infrastructure.