8000 Handle failures in initialization and add cling output features. by marsupial · Pull Request #38 · root-project/cling · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Handle failures in initialization and add cling output features. #38

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 13 commits into from
Closed

Handle failures in initialization and add cling output features. #38

wants to merge 13 commits into from

Conversation

marsupial
Copy link
Contributor

I can't split these as the second relies on the first.

@Axel-Naumann
Copy link
Member

That's an awesome set of features! Thank you!

@vgvassilev vgvassilev self-assigned this Aug 20, 2016
vgvassilev
vgvassilev reviewed Aug 20, 2016
View reviewed changes
lib/Interpreter/CIFactory.cpp
// Add configuration paths to interpreter's include files.
std::vector<const char*>& args,
bool Verbose) {
static std::vector<std::string> sArguments;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you implement that without statics? Statics are troublesome for multi interpreter setups.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally agree, but was following the pattern of AddHostArguments.
I thought it did statics for two reasons:
Avoid redoing work.
Make sure all children get the same flags as parent. In an interpreted environment there's no guarantee that would be true across calls if they are not static.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that makes sense. In this case we force slave interpreters to be with the same invocation flags. Could you explain this constraint in a comment.

@vgvassilev
Copy link
Member

Could you also add the pch and -E support in the release notes?

@marsupial
Copy link
Contributor Author

I'm not sure I'm understanding the logic behind replacing actual error handling with asserts.
It only does something in Debug, in Release it's totally silent.
If you use cling as a library, it aborts the running process.
There are valid cases for process to be fine if cling couldn't initialize an Interpreter fully, writing output through clang is one of them.

I understand ROOT is the primary client and you want to keep it happy, but I think it's the hosts responsibility to make sure it is setup went fine. Hence the isValid methods.

Is it too problematic to add assert(m_Interp->isValid() && "Initialization failed") in ROOT?

vgvassilev
vgvassilev reviewed Aug 20, 2016
View reviewed changes
lib/Interpreter/Interpreter.cpp

// Tell the diagnostic client that we are entering file parsing mode.
DiagnosticConsumer& DClient = getCI()->getDiagnosticClient();
DClient.BeginSourceFile(getCI()->getLangOpts(), &PP);

llvm::SmallVector<IncrementalParser::ParseResultTransaction, 2>
IncrParserTransactions;
m_IncrParser->Initialize(IncrParserTransactions, isChildInterp);
if (!m_IncrParser->Initialize(IncrParserTransactions, isChildInterp)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am opposed to this approach in general. I think we should always have a well-constructed and initialized interpreter, this reduces a lot of debugging. What would be the use case here?

@vgvassilev
Copy link
Member

Indeed we could add this assertion on the user side. One of the issues would be that we don't really know where exactly it failed. While I am inclined to allow this in some places in cling, I don't feel that the interpreter initialization time should be one of them. Unless there is some very obvious reason or a use case which I am missing.

For the library case: Aborting the process is not always a bad thing, especially when we know we are not in a valid state, thus most likely the produced results would be bogus. If we want to continue no matter what, think we need to implement a macro that expands asserts to printing errors and continuing the execution (where possible). This way we would be able to survive asserts in llvm and clang, too. I'd prefer this approach.

@marsupial
Copy link
Contributor Author

One of the issues would be that we don't really

Well I've been logging to llvm::errs() in the places where failure can be detected/logged.

I don't feel that the interpreter initialization time should be one of them. Unless there is some very obvious reason or a use case which I am missing.

I'm of the opinion initialization has to be one of the cases where failure can occur without aborting.

Think of a web-browser, would it be ok to crash the whole thing just because a JavaScript engine wasn't properly initialized, or should that failure be detected and the page rendered differently?

Even more important to me is that users will have work (possibly unsaved) before libCling is invoked. If libCling fails: throwing a warning is fine, but flat out abort is unacceptable. I think beyond cling & ROOT this would be the majority of use cases too, you're a subsystem not the owning process.

I do agree that aborting is acceptable in some case, but see them as extreme cases where any consistancy can no longer be guaranteed.

I see the real issue begin that construction & initialization is intertwined, not only between one object, but several. But any change to this would be a much larger interface change than proposed here.

@marsupial
Copy link
Contributor Author

Looking at it the changes you're requesting would require:

  1. Another CIFactory::createCI variant
  2. createCIImpl being split into two functions.
  3. Movement of InvocationOptions out of Interpreter
  4. Addition of canAbort argument (could default to true) to both Interpeter and IncrementalParser constructors

While I generally agree with 1-3:
3 would be a larger interface change.
4 is a bit ugly.

pcanal
pcanal reviewed Aug 26, 2016
View reviewed changes
lib/Interpreter/CIFactory.cpp
if (CC1Args == NULL) {
delete Invocation;
return 0;
if (!Compilation) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a variable name that is 'distinct' from a class name (to avoid confusion between constructor and variable)?

@marsupial marsupial force-pushed the pr6 branch 7 times, most recently from 95020ae to 04d8765 Compare September 3, 2016 02:38
@marsupial marsupial force-pushed the pr6 branch 2 times, most recently from 812eb6d to 8c8de33 Compare September 10, 2016 09:13
@vgvassilev
Copy link
Member

Could you rebase this PR, please?

Would this be able to handle:

clang ...opts... -E > PP.out
cling -include PP.out

This would greatly help reducing cling-related issues.

@marsupial
Copy link
Contributor Author

I'm not sure if you asking whether it records the session or output can be fed back in.
Former no, later yes.
Which also makes it possible to use the preprocessor in tests.
cling -C -E File.C > PPFile.C
cat PPFile.C | cling | FileCheck PPFile.C

@vgvassilev
Copy link
Member

@marsupial that should be good enough and it could be improved in future. Please rebase to continue the review and merge it in.

@marsupial marsupial force-pushed the pr6 branch 3 times, most recently from 93bc486 to 5fe6276 Compare January 29, 2017 13:46
@marsupial
Add argument to IncrementalParser::commitTransaction to avoid clearin…
adcf470 
…g the DiagnosticsConsumer.

Remove temporary variable.
@marsupial
Check initialization is proceeding as expecting.
3d1c38e 
Don't blindly run through initialization; make sure things are going according to plan.
If not handle it and run destructors without crashing.
Return standard error codes from main.
@marsupial
Stop initialization if the PCH contained errors.
5b73d5f
@marsupial
Feature to generate precompiled headers.
1892d32
@marsupial
Handle -E flag to dump preprocessor definitions.
cd21b41
@marsupial
Early out Interpreter initialization when invoked via '-v' or '-versi…
0351ac8 
…on'.
@marsupial
Setup Diagnostics earlier in initialization.
3ea824f
@marsupial
Cleanup main and add InvocationOptions::IsInteractive method.
dbbfb50
@marsupial
Don’t let an invalid PCH file stop an interactive session from running.
3881957
@marsupial
Copy link
Contributor Author

This has been rebased to master.
I'd rather not have to go through that again so at a minimum there should be a definitive decision whether it will get in or not.

@Axel-Naumann
Copy link
Member

Thanks. I have discussed with Vassil; this is good to go. I'll test before pushing. If I have any further comments it'll be either due to a test failure or after the merge.

@Axel-Naumann
Copy link
Member

Merged, thanks a lot!

Could you provide a couple of lines describing this PR's features, for the announcement of the next cling release?

@marsupial
Copy link
Contributor Author

Are the additions in the ReleaseNotes not enough?

@marsupial marsupial closed this Feb 2, 2017
@marsupial marsupial deleted the pr6 branch February 2, 2017 18:44
@Axel-Naumann
Copy link
Member

Yes they are - and that's probably also why I subconsciously thought of it. Thanks!

FonsRademakers pushed a commit that referenced this pull request Oct 11, 2018
@pcanal
Fix cling::Interpreter::compileFunction getting confused by trailing …
ad59359 
…transactions.

This fixes https://sft.its.cern.ch/jira/browse/ROOT-9672 by having
cling::Interpreter::DeclareCFunction return the transaction containing the
compiled code.

With the previous code, cling::Interpreter::compileFunction will get confused by
transaction created during the callbacks executed during the
cling::IncrementalParser::commitTransaction of the main transaction.

Reproducer:

With a main composed of 'only':

int main(int argc, char ** argv)
{
  char const * class_string = (argc == 2) ? argv[1] : "std::vector<int>";
  auto const result [[gnu::unused]] = TClass::GetClass(class_string);
  return 0;
}

which is a representation of real use case (in a more complex setup) in ART.
We were getting:

Error in <TClingCallFunc::make_wrapper>: Failed to compile
  ==== SOURCE BEGIN ====
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wformat-security"
__attribute__((used)) extern "C" void __cf_0(void* obj, int nargs, void** args, void* ret)
{
   if (ret) {
      (*(TStreamerInfo**)ret) = new TStreamerInfo();
      return;
   }
   else {
      new TStreamerInfo();
      return;
   }
}
#pragma clang diagnostic pop
  ==== SOURCE END ====
Error in <TClingCallFunc::ExecT>: Called with no wrapper, not implemented!
Error in <TVirtualStreamerInfo::Factory>: The plugin handler for TVirtualStreamerInfo was found but failed to create the factory object!

The reason is that during TClingCallFunc::make_wrapper, the call to cling::Interpreter::compileFunction ends with:

    if (const llvm::GlobalValue* GV
        = getLastTransaction()->getModule()->getNamedValue(name))

However in the 'broken' case, the getLastTransaction does not return the transaction for the code being compiled by DeclareCFunction but instead the one used/created at:

#0  cling::IncrementalParser::endTransaction (this=0x4a2980, T=0x8c0fb0) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/interpreter/cling/lib/Interpreter/IncrementalParser.cpp:345
#1  0x00007fffeebc7899 in cling::Interpreter::PushTransactionRAII::pop (this=0x7fffffffcb00) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/interpreter/cling/lib/Interpreter/Interpreter.cpp:111
#2  0x00007fffeebc785e in cling::Interpreter::PushTransactionRAII::~PushTransactionRAII (this=0x7fffffffcb00, __in_chrg=<optimized out>)
    at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/interpreter/cling/lib/Interpreter/Interpreter.cpp:106
#3  0x00007fffeebeb659 in cling::LookupHelper::findScope (this=0x4a9dd0, className=..., diagOnOff=cling::LookupHelper::NoDiagnostics, resultType=0x7fffffffcd08, instantiateTemplate=false)
    at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/interpreter/cling/lib/Interpreter/LookupHelper.cpp:466
#4  0x00007fffeeabe0df in TCling::CheckClassInfo (this=0x4a0550, name=<optimized out>, autoload=<optimized out>, isClassOrNamespaceOnly=<optimized out>)
    at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/metacling/src/TCling.cxx:3630
#5  0x00007ffff7c3040d in TClass::Init (this=this@entry=0xdafd20, name=name@entry=0x7ffff7cb7638 "TGlobal", cversion=cversion@entry=2, typeinfo=typeinfo@entry=0x7ffff7d8b6d8 <typeinfo for TGlobal>, isa=isa@entry=0x477430,
    dfil=dfil@entry=0x7ffff7cb8cab "TGlobal.h", ifil=<optimized out>, dl=<optimized out>, il=<optimized out>, givenInfo=<optimized out>, silent=<optimized out>)
    at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/meta/src/TClass.cxx:1431
#6  0x00007ffff7c3a1b8 in TClass::TClass (this=0xdafd20, name=0x7ffff7cb7638 "TGlobal", cversion=<optimized out>, info=..., isa=0x477430, dfil=0x7ffff7cb8cab "TGlobal.h",
    ifil=0x7ffff7cccf88 "/local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/meta/src/TGlobal.cxx", dl=27, il=25, silent=false) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/meta/src/TClass.cxx:1273
#7  0x00007ffff7c3a72a in ROOT::Cr
8000
eateClass (cname=0x7ffff7cb7638 "TGlobal", id=id@entry=2, info=..., isa=isa@entry=0x477430, dfil=dfil@entry=0x7ffff7cb8cab "TGlobal.h",
    ifil=ifil@entry=0x7ffff7cccf88 "/local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/meta/src/TGlobal.cxx", dl=27, il=25) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/meta/src/TClass.cxx:5607
#8  0x00007ffff7c4b552 in ROOT::Internal::TDefaultInitBehavior::CreateClass (il=25, dl=27, ifil=0x7ffff7cccf88 "/local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/meta/src/TGlobal.cxx", dfil=0x7ffff7cb8cab "TGlobal.h",
    isa=0x477430, info=..., id=2, cname=<optimized out>, this=0x7ffff7da7508 <ROOT::Internal::DefineBehavior(void*, void*)::theDefault>) at /home/pcanal/root_builds/v6-14-00-patches/opt/include/Rtypes.h:176
#9  ROOT::TGenericClassInfo::GetClass (this=0x7ffff7dab660 <ROOT::GenerateInitInstanceLocal(TGlobal const*)::instance>) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/meta/src/TGenericClassInfo.cxx:250
#10 0x00007ffff7b1a2d8 in TGlobal::Class () at /home/pcanal/root_builds/v6-14-00-patches/opt/core/base/G__Core.cxx:17156
#11 0x00007ffff7ac01de in TGlobal::IsA (this=0xee3bc0) at /home/pcanal/root_builds/v6-14-00-patches/opt/include/TGlobal.h:48
#12 TGlobal::CheckTObjectHashConsistency (this=0xee3bc0) at /home/pcanal/root_builds/v6-14-00-patches/opt/include/TGlobal.h:48
#13 0x00007ffff7be9dcd in TObject::CheckedHash (this=0xee3bc0) at /home/pcanal/root_builds/v6-14-00-patches/opt/include/TObject.h:314
#14 THashTable::GetCheckedHashValue (this=0xe65a20, obj=0xee3bc0) at /home/pcanal/root_builds/v6-14-00-patches/opt/include/THashTable.h:94
#15 THashTable::Add (this=0xe65a20, obj=0xee3bc0) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/cont/src/THashTable.cxx:96
#16 0x00007ffff7be6bf1 in THashList::AddLast (this=this@entry=0x5be690, obj=obj@entry=0xee3bc0) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/cont/src/THashList.cxx:100
#17 0x00007ffff7c4e0d1 in TListOfDataMembers::AddLast (this=0x5be690, obj=0xee3bc0) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/meta/src/TListOfDataMembers.cxx:103
#18 0x00007ffff7ab8785 in TList::Add (obj=0xee3bc0, this=0x5be690) at /home/pcanal/root_builds/v6-14-00-patches/opt/include/TList.h:87
#19 TROOT::GetListOfGlobals (this=0x7ffff7da7a60 <ROOT::Internal::GetROOT1()::alloc>, load=load@entry=false) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/base/src/TROOT.cxx:1767
#20 0x00007fffeeab1058 in TCling::HandleNewDecl (this=0x4a0550, DV=0xedf238, isDeserialized=isDeserialized@entry=true, modifiedTClasses=...) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/metacling/src/TCling.cxx:555
#21 0x00007fffeeabb785 in TCling::UpdateListsOnCommitted (this=0x4a0550, T=...) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/metacling/src/TCling.cxx:6115
#22 0x00007fffeebd0103 in cling::MultiplexInterpreterCallbacks::TransactionCommitted (this=0x57fe20, T=...) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/interpreter/cling/lib/Interpreter/MultiplexInterpreterCallbacks.h:76
#23 0x00007fffeed05d71 in cling::IncrementalParser::commitTransaction (this=0x4a2980, PRT=..., ClearDiagClient=true) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/interpreter/cling/lib/Interpreter/IncrementalParser.cpp:532
#24 0x00007fffeed06399 in cling::IncrementalParser::Compile (this=0x4a2980, input=..., Opts=...) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/interpreter/cling/lib/Interpreter/IncrementalParser.cpp:663
#25 0x00007fffeebcbc4e in cling::Interpreter::DeclareInternal (this=0x4a0f30, input=..., CO=..., T=0x7fffffffd680) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/interpreter/cling/lib/Interpreter/Interpreter.cpp:1195
#26 0x00007fffeebca8e8 in cling::Interpreter::declare (this=0x4a0f30, input=..., T=0x7fffffffd680) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/interpreter/cling/lib/Interpreter/Interpreter.cpp:823
#27 0x00007fffeebcb560 in cling::Interpreter::DeclareCFunction (this=0x4a0f30, name=..., code=..., withAccessControl=true) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/interpreter/cling/lib/Interpreter/Interpreter.cpp:1096
#28 0x00007fffeebcb862 in cling::Interpreter::compileFunction (this=0x4a0f30, name=..., code=..., ifUnique=false, withAccessControl=true)
    at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/interpreter/cling/lib/Interpreter/Interpreter.cpp:1140
#29 0x00007fffeeafb83c in TClingCallFunc::compile_wrapper (withAccessControl=true, wrapper=..., wrapper_name=..., this=0xcf3c10) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/metacling/src/TClingCallFunc.cxx:270
#30 TClingCallFunc::make_wrapper (this=this@entry=0xcf3c10) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/metacling/src/TClingCallFunc.cxx:1096
#31 0x00007fffeeafbcb8 in TClingCallFunc::IFacePtr (this=this@entry=0xcf3c10) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/metacling/src/TClingCallFunc.cxx:2233
#32 0x00007fffeeafbe83 in TClingCallFunc::ExecT<long> (address=0x0, this=0xcf3c10) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/metacling/src/TClingCallFunc.cxx:2045
#33 TClingCallFunc::ExecInt (this=0xcf3c10, address=0x0) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/metacling/src/TClingCallFunc.cxx:2065
#34 0x00007ffff7c56e8d in TMethodCall::Execute (this=0xd97710, object=<optimized out>, retLong=@0x7fffffffd958: 0) at /local2/pcanal/cint_working/rootcling/v6-14-00-patches/core/meta/src/TMethodCall.cxx:457
#35 0x0000000000401009 in TMethodCall::Execute(long&) ()
#36 0x00000000004010ea in long TPluginHandler::ExecPluginImpl<>() ()
#37 0x000000000040106d in long TPluginHandler::ExecPlugin<>(int) ()
#38 0x0000000000400e21 in mytest() ()
#39 0x0000000000400e92 in main ()
@LoveHuZi LoveHuZi mentioned this pull request May 4, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@vgvassilev vgvassilev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marsupial @Axel-Naumann @vgvassilev @pcanal
0