Skip to content
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

[Cling] Interpreter regression 6.24/06 -> 6.25/02 #9664

Closed
1 task done
greenc-FNAL opened this issue Jan 22, 2022 · 5 comments · Fixed by #9691
Closed
1 task done

[Cling] Interpreter regression 6.24/06 -> 6.25/02 #9664

greenc-FNAL opened this issue Jan 22, 2022 · 5 comments · Fixed by #9691

Comments

@greenc-FNAL
Copy link
Contributor

greenc-FNAL commented Jan 22, 2022

  • Checked for duplicates

Describe the bug

With 6.24/06 vanilla and v6-24-00-patches e9d29ae, the test runtime_cxxmodules_addr in the provided reproducer
passes regardless of the presence or absence of the feature runtime_cxxmodules. With 6.25/02—and master, per @pcanal—it fails with a segmentation violation trying to autoparse art::detail::are_cv_compatible<const int, const int*>.

Expected behavior

To Reproduce

  • Ensure you are using ROOT 6.25/02 (or see below)
  • Ensure you are using CMake >= 3.22.0
  • Execute the provided runit script to configure, build, and execute the tests.

Setup

  • SLF7
  • CMAKE_CXX_STANDARD=17

Additional context

Preliminary investigation by @pcanal (to ensure that I hadn't messed up something basic in the configuration or XML) indicates that the problem was introduced by PR #7488 and not fixed by its partial reversion in 2d304cf. According to @pcanal, a reversion of both 2d304cf and 34590ae resolves this issue.

Reproducer: runtime_cxxmodules-reproducer.tar.gz

@hahnjo
Copy link
Member

hahnjo commented Jan 25, 2022

the problem was introduced by PR #7488 and not fixed by its partial reversion in 2d304cf

Small correction: Commit 2d304cf is not a reversion, but a cleanup of the infrastructure for the previous implementation; it happened in the same PR.

Reproducer: runtime_cxxmodules-reproducer.tar.gz

I have to say, this was quite hard to use and it took me some time to guess how things are supposed to work:

  1. Don't install a released version of cetmodules, but use the one in third_party/ - I eventually found standalone/runit.
  2. The cmake configuration failed even with CMake 3.22 or CMake master: $<EQUAL> parameter is not a valid integer. - I replaced it with STREQUAL (in third_party/cetmodules/Modules/BuildDictionary.cmake).
  3. I have to specify CMAKE_CXX_STANDARD=17 manually for the reproducer project.

Anyway, I have it running now with runtime_cxxmodules_9583_good passing and runtime_cxxmodules_9583_bad crashing:

#5  0x00007ff9c7acc0f0 in clang::Stmt::getEndLoc() const () from /home/jhahnfel/ROOT/build/lib/libCling.so
#6  0x00007ff9c797c8c8 in clang::FunctionDecl::setBody(clang::Stmt*) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#7  0x00007ff9c5688477 in cling::AutoSynthesizer::Transform(clang::Decl*) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#8  0x00007ff9c56a3f17 in cling::DeclCollector::TransformDecl(clang::Decl*) const () from /home/jhahnfel/ROOT/build/lib/libCling.so
#9  0x00007ff9c56a409f in cling::DeclCollector::Transform(clang::DeclGroupRef&) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#10 0x00007ff9c56a4241 in cling::DeclCollector::HandleTopLevelDecl(clang::DeclGroupRef) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#11 0x00007ff9c5b21150 in clang::MultiplexConsumer::HandleTopLevelDecl(clang::DeclGroupRef) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#12 0x00007ff9c643e4b7 in clang::Sema::InstantiateFunctionDefinition(clang::SourceLocation, clang::FunctionDecl*, bool, bool, bool) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#13 0x00007ff9c643da0f in clang::Sema::PerformPendingInstantiations(bool) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#14 0x00007ff9c5ecaefe in clang::Sema::ActOnEndOfTranslationUnitFragment(clang::Sema::TUFragmentKind) [clone .part.1427] () from /home/jhahnfel/ROOT/build/lib/libCling.so
#15 0x00007ff9c5ecafdd in clang::Sema::ActOnEndOfTranslationUnit() () from /home/jhahnfel/ROOT/build/lib/libCling.so
#16 0x00007ff9c5d5279e in clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, bool) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#17 0x00007ff9c5668e00 in cling::IncrementalParser::ParseInternal(llvm::StringRef) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#18 0x00007ff9c5669d0c in cling::IncrementalParser::Compile(llvm::StringRef, cling::CompilationOptions const&) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#19 0x00007ff9c55e001a in cling::Interpreter::loadHeader(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cling::Transaction**) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#20 0x00007ff9c56b81d6 in cling::MetaSema::actOnLCommand(llvm::StringRef, cling::Transaction**) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#21 0x00007ff9c56b8319 in cling::MetaSema::actOnxCommand(llvm::StringRef, llvm::StringRef, cling::Value*) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#22 0x00007ff9c56c790f in cling::MetaParser::isXCommand(cling::MetaSema::ActionResult&, cling::Value*) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#23 0x00007ff9c56c7c14 in cling::MetaParser::isCommand(cling::MetaSema::ActionResult&, cling::Value*) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#24 0x00007ff9c56b2310 in cling::MetaProcessor::process(llvm::StringRef, cling::Interpreter::CompilationResult&, cling::Value*, bool) () from /home/jhahnfel/ROOT/build/lib/libCling.so
#25 0x00007ff9c54fa3ec in HandleInterpreterException(cling::MetaProcessor*, char const*, cling::Interpreter::CompilationResult&, cling::Value*) () from /home/jhahnfel/ROOT/build/lib/libCling.so

(would have been nice to get this stack trace with the original report)

@hahnjo
Copy link
Member

hahnjo commented Jan 25, 2022

What Cling is really crashing on is the definition of art::ensurePointer or more precisely any function definition with a try-catch block as body (which I just learned is valid C++...). The problem can be seen with

.rawInput
void f() try { } catch (...) { }

(on the ROOT prompt)

@vgvassilev
Copy link
Member

What Cling is really crashing on is the definition of art::ensurePointer or more precisely any function definition with a try-catch block as body (which I just learned is valid C++...). The problem can be seen with

.rawInput
void f() try { } catch (...) { }

(on the ROOT prompt)

Yeah, that's a common pitfall, where the assumption that the function's body is a CompoundStmt is incorrect. I suspect the fix is trivial?

@hahnjo
Copy link
Member

hahnjo commented Jan 25, 2022

Well not entirely trivial, there's already code to handle this:

else if (CXXTryStmt *TS = dyn_cast_or_null<CXXTryStmt>(Body))
Body = m_AutoFixer->Fix(TS);

The problem is that apparently the SourceLocation is not properly set. I'm looking into it.

hahnjo added a commit to hahnjo/roottest that referenced this issue Jan 25, 2022
hahnjo added a commit to hahnjo/root that referenced this issue Jan 25, 2022
This was an oversight in commit 34590ae: After fixing the handler
blocks, the code needs to create new CXXCatchStmts to eventually
construct the CXXTryStmt.

Fixes root-project#9664
@hahnjo
Copy link
Member

hahnjo commented Jan 25, 2022

Or maybe I just completely broke that code path with said commit...

hahnjo added a commit to hahnjo/roottest that referenced this issue Jan 26, 2022
hahnjo added a commit that referenced this issue Jan 26, 2022
This was an oversight in commit 34590ae: After fixing the handler
blocks, the code needs to create new CXXCatchStmts to eventually
construct the CXXTryStmt.

Fixes #9664
hahnjo added a commit that referenced this issue Jan 26, 2022
This was an oversight in commit 34590ae: After fixing the handler
blocks, the code needs to create new CXXCatchStmts to eventually
construct the CXXTryStmt.

Fixes #9664

(cherry picked from commit 94091b4)
hahnjo added a commit to root-project/roottest that referenced this issue Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants