[cling] Create new CompoundStmt instead of replacing children #7488
Conversation
|
Starting build on |
|
The idea for this change comes from my own experiments with upgrading LLVM during my time as student worker (and I can't reliably tell if I wrote the code during paid hours or in my free time, hence the funny list of authorships). It would allow to drop one possibly contentious patch for Clang, so I wanted to get the change out for feedback. One thing I currently don't understand are two test failures in diff --git a/interpreter/llvm/src/tools/clang/include/clang/AST/Stmt.h b/interpreter/llvm/src/tools/clang/include/clang/AST/Stmt.h
index 403b88ac3a..5e1581f098 100644
--- a/interpreter/llvm/src/tools/clang/include/clang/AST/Stmt.h
+++ b/interpreter/llvm/src/tools/clang/include/clang/AST/Stmt.h
@@ -131,7 +131,8 @@ protected:
unsigned : NumStmtBits;
- unsigned NumStmts : 32 - NumStmtBits;
+ unsigned WasReplaced : 1;
+ unsigned NumStmts : 32 - (NumStmtBits + 1);
/// The location of the opening "{".
SourceLocation LBraceLoc;
@@ -1328,6 +1329,7 @@ public:
explicit CompoundStmt(SourceLocation Loc)
: Stmt(CompoundStmtClass), RBraceLoc(Loc) {
CompoundStmtBits.NumStmts = 0;
+ CompoundStmtBits.WasReplaced = 0;
CompoundStmtBits.LBraceLoc = Loc;
}
diff --git a/interpreter/llvm/src/tools/clang/lib/AST/Stmt.cpp b/interpreter/llvm/src/tools/clang/lib/AST/Stmt.cpp
index 0a4d403106..cc8c6888b8 100644
--- a/interpreter/llvm/src/tools/clang/lib/AST/Stmt.cpp
+++ b/interpreter/llvm/src/tools/clang/lib/AST/Stmt.cpp
@@ -293,6 +293,7 @@ CompoundStmt::CompoundStmt(ArrayRef<Stmt *> Stmts, SourceLocation LB,
SourceLocation RB)
: Stmt(CompoundStmtClass), RBraceLoc(RB) {
CompoundStmtBits.NumStmts = Stmts.size();
+ CompoundStmtBits.WasReplaced = 0;
setStmts(Stmts);
CompoundStmtBits.LBraceLoc = LB;
}
@@ -316,6 +317,7 @@ CompoundStmt *CompoundStmt::CreateEmpty(const ASTContext &C,
void *Mem =
C.Allocate(totalSizeToAlloc<Stmt *>(NumStmts), alignof(CompoundStmt));
CompoundStmt *New = new (Mem) CompoundStmt(EmptyShell());
+ New->CompoundStmtBits.WasReplaced = 0;
New->CompoundStmtBits.NumStmts = NumStmts;
return New;
}which smells fishy (but I don't want to go down that rabbit hole if the patch is not deemed suitable for other reasons)... |
|
Build failed on mac11.0/cxx17. Errors:
|
As Node is still a CompoundStmt, this will call the same function recursively and again visit all previously visited and replaced children, which doesn't seem necessary.
For the update of LLVM 9, Cling required another patch to Clang for replacing the children of a CompoundStmt. Instead solve this by creating a new CompoundStmt with the right Stmts attached. Co-authored-by: Jonas Hahnfeld <[email protected]> Co-authored-by: Jonas Hahnfeld <[email protected]>
hahnjo
force-pushed
the
cling-replaceStmts
branch
from
f586e02
to
5a3c007
Compare
|
Starting build on |
|
I think that we don't actually need the recursive visits in |
|
@hahnjo, thanks for working on this! Could you share some more information about the test failures? Just like you, I am quite puzzled why that fixes any tests... The only reason that I can think of is we did not properly reverted vgvassilev/clang@fce2607 which does not seem the case... Can you remove the fixup patch to see current failures? PS: Maybe somehow clad picks up old headers/libraries? |
|
@vgvassilev sure:
That's the funny thing: the PR doesn't contain the fixup, but Jenkins is happy anyway. It could be that it doesn't include clad (which would be bad) or that some of my changes either introduce or uncover bad loads of uninitialized memory or use-after-free issues. I'm currently building an instrumented version of ROOT with AddressSanitizer... |
|
@hahnjo, is your build incremental? Like, you built the master, then rebuild the PR code? If so, clad won't be rebuilt as any of the other |
Yes, I'm building incrementally, but even removing |
|
Ok, good, progress! I'd propose to move forward with this change and address the clad issue separately. |
|
Eventually yes (feel free to review the current changes), but I don't like having failing tests. Green Jenkins being one thing, but I also want all to pass locally 😃 |
|
Hm, the "finding" from ASan was #7968 aka Clad built without ASan and setting some memory that the sanitizer afterwards doesn't know about. Both tests, |
|
Could you delete your build folder, reconfigure and rebuild? |
|
Okay, I just found where I made the mistake: While deleting |
|
Yep, a clean build passed all tests. And to prove the point, I then switched back to |
For the update of LLVM 9, Cling required another patch to Clang for replacing the children of a
CompoundStmt. Instead solve this by creating a newCompoundStmtwith the rightStmts attached.