fix SynthesizingCodeRAII for clang-repl#819
fix SynthesizingCodeRAII for clang-repl#819Vipul-Cariappa merged 2 commits intocompiler-research:mainfrom
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #819 +/- ##
=======================================
Coverage 79.56% 79.56%
=======================================
Files 11 11
Lines 4012 4012
=======================================
Hits 3192 3192
Misses 820 820
🚀 New features to boost your workflow:
|
Vipul-Cariappa
force-pushed
the
dev/RAII-clang
branch
from
09391fe to
97536e8
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
That should be the correct behavior. If there is a syntax error, the AST is broken and what code we are expected to jit? |
Broken AST is not sent to JIT. So, in theory, the JIT is not in a broken state. Only good ASTs (TranslationUnit) are JITed. |
Why do we need to reset the diagnostics, if everything was fine? |
Vipul-Cariappa
force-pushed
the
dev/RAII-clang
branch
from
97536e8 to
263e37c
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Vipul-Cariappa
force-pushed
the
dev/RAII-clang
branch
2 times, most recently
from
7a6af96 to
78241c8
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
| getSema().InstantiateFunctionDefinition(SourceLocation(), FD, | ||
| /*Recursive=*/true, | ||
| /*DefinitionRequired=*/true); | ||
| clang::DiagnosticsEngine& Diags = getSema().getDiagnostics(); |
There was a problem hiding this comment.
We need a fixme to move this in a raii.
There was a problem hiding this comment.
This is the only code path that requires this. Why create a RAII object for a one-off case?
Also, from the last meeting, what I understood is: The SynthesizingCodeRAII class should tackle failures at the JIT level. And to create a new RAII object for this (errors at the parser level). If the "diagnostics reset" is required in other code paths at that point, we should add a new RAII object.
There was a problem hiding this comment.
I think such a raii already exists in Sema.
lib/CppInterOp/CppInterOp.cpp
Outdated
| auto Symbol = compat::getSymbolAddress(I, linker_mangled_name); | ||
| llvm::orc::LLJIT& Jit = *compat::getExecutionEngine(I); | ||
| llvm::orc::ExecutionSession& ES = Jit.getExecutionSession(); | ||
| // FIXME: for getMainJITDylib |
There was a problem hiding this comment.
This should not be here. Did not plan to commit this. Note to self: remove this.
Vipul-Cariappa
force-pushed
the
dev/RAII-clang
branch
from
78241c8 to
33241a6
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
vgvassilev
left a comment
vgvassilev
left a comment
There was a problem hiding this comment.
Lgtm! Consider squash in merge.
2 participants
Description
This was a problem with the state of the IntermentalParser in clang.
If the
TranslationUnithas some problems, then it is not passed toExecute, and I believe it is not pushed intoPTUs.This is the minimum change that was required to get the tests to pass, in CppInterOp as well as cppyy.
I don't know if we will have problems later when using PCHs and modules.
Maybe we should consider upstreaming this?
Fixes # (issue)
Regression introduced by #787, that cases
test_leakcheck.pycppyy test to fail after running for an hour.Please tick all options which are relevant.
Testing
Please describe the test(s) that you added and ran to verify your changes.
Checklist