[cxxmodules] Avoid lookup from system headers #11514

hahnjo · 2022-10-07T12:58:58Z

Avoid loading ROOT modules while Clang is instantiating a template class from STL, leading to ODR checks with an incomplete class.

phsft-bot · 2022-10-07T12:59:05Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

core/metacling/src/TClingCallbacks.cxx

vgvassilev · 2022-10-08T14:32:56Z

@junaire, can you test if this solves also your problems?

core/metacling/src/TClingCallbacks.cxx

vgvassilev · 2022-10-08T14:41:37Z

core/metacling/src/TClingCallbacks.cxx

+   auto *D = dyn_cast<Decl>(DC);
+   if (D) {
+      SourceLocation Loc = D->getLocation();
+      if (Loc.isValid() && SemaR.getSourceManager().isInSystemHeader(Loc)) {


We could ask if Decls OwningModule is a system one. This way we could "annotate" modules which we do not need to mess with. We consider modules starting with ROOT_ system modules because a pure [system] tag also changes a little the semantics of the whole thing but I think we are fine to do that check here.

I think we probably can explore a little more the heuristics you came up with as it looks promising. Namely, can a declaration context of a module be 'extended with' declarations from a module that is not listed among the module dependencies? I suspect the only cases where this can happen is friend declarations and ADL lookups. I don't think we need to support that... Eg. what happens if we have a module that adds another std::swap with MyClass parameter - are we expected to autoload the module exporting MyClass?

I like asking the OwningModule and changed both the existing and the new check 😃

I think we probably can explore a little more the heuristics you came up with as it looks promising.

Yes, we likely need to understand the other failure modes and then abstract that to a heuristic / check that we can add here. For now I just want to make both LookupObject methods bail out on the same conditions, namely a Decl coming from a system module.

Hm well, I guess this doesn't work for the nortcxxmod case? 😬

I don't think we care for the nortcxxmod as the PCH is preloaded and we don't react to entities that come from an ASTFile. I don't think we have "system" headers which are not in the PCH.

I reverted back to asking isInSystemHeader which did better in the tests...

junaire · 2022-10-08T16:18:54Z

@junaire, can you test if this solves also your problems?

I haven't tested this, but @hahnjo said this almost fixes my issue in #10910, only roottest-root-html-runMakeIndex needs some tweaking for the generated header file. But I think this may not be a full "workaround" as it doesn't handle the root issue. In fact, Jonas did mention this doesn't help with the problems about TSeq, which means we still have to preload ROOTDataFrame.

I also made my own fix (4757b60) and I feel like it may be a better direction to work on (even though it also failed to fix TSeq...)

junaire · 2022-10-08T17:26:22Z

@junaire, can you test if this solves also your problems?

I haven't tested this, but @hahnjo said this almost fixes my issue in #10910, only roottest-root-html-runMakeIndex needs some tweaking for the generated header file. But I think this may not be a full "workaround" as it doesn't handle the root issue. In fact, Jonas did mention this doesn't help with the problems about TSeq, which means we still have to preload ROOTDataFrame.

I also made my own fix (4757b60) and I feel like it may be a better direction to work on (even though it also failed to fix TSeq...)

FWIW I pushed a new commit to #10910 and it looks like surprisingly fixes all issues even without preloading ROOTDataFrame! But now let's see if Jenkins is happy or not. Fingers crossed :)

junaire · 2022-10-09T05:32:32Z

@junaire, can you test if this solves also your problems?

I haven't tested this, but @hahnjo said this almost fixes my issue in #10910, only roottest-root-html-runMakeIndex needs some tweaking for the generated header file. But I think this may not be a full "workaround" as it doesn't handle the root issue. In fact, Jonas did mention this doesn't help with the problems about TSeq, which means we still have to preload ROOTDataFrame.
I also made my own fix (4757b60) and I feel like it may be a better direction to work on (even though it also failed to fix TSeq...)

FWIW I pushed a new commit to #10910 and it looks like surprisingly fixes all issues even without preloading ROOTDataFrame! But now let's see if Jenkins is happy or not. Fingers crossed :)

OK, so we still got some failures but I can't see it locally. I'll try to reproduce them, but I feel like that's the right direction. CC @hahnjo @vgvassilev

hahnjo · 2022-10-10T08:48:29Z

@vgvassilev as I wrote on Mattermost, this change was particularly written to address Jun's problem. roottest-root-html-runMakeIndex is a separate thing that needs addressing anyhow; it's only now visible because all the other failures are gone.

FWIW I don't agree that the workarounds (hacks) introduced in #10910 are a good way to go, as shown by the many failing tests. The scope of the changes is too big and there are valid reasons to do something with Decls currently being defined, as evidenced by the many failing tests. Adding more conditions will eventually only lead to internally inconsistent states...

junaire · 2022-10-10T09:13:03Z

@vgvassilev as I wrote on Mattermost, this change was particularly written to address Jun's problem. roottest-root-html-runMakeIndex is a separate thing that needs addressing anyhow; it's only now visible because all the other failures are gone.

FWIW I don't agree that the workarounds (hacks) introduced in #10910 are a good way to go, as shown by the many failing tests. The scope of the changes is too big and there are valid reasons to do something with Decls currently being defined, as evidenced by the many failing tests. Adding more conditions will eventually only lead to internally inconsistent states...

The "workaround" introduced in #10910 is not only intended to address my problem but also serves as a more generic fix. It makes more sense right? disable the callback when we're instantiating templates.

as shown by the many failing tests.

Ahh... yes and no, actually we're almost getting there. Currently, it is just some strange failures in some specific build bots (ROOT-debian10-i386 and mac builders) We also haven't tested that separately so who knows what triggered the failure.

I'm not really sure which way is the best to go, we still need time to discover, can you check if that patch fixes your issue? Also, I would appreciate it if you can take a look at the failures to see if you have a clue or not :)

hahnjo · 2022-10-10T09:21:37Z

@junaire tests should be clean in CI builds, as you can see for this PR. Any failures are introduced by the code changes, possibly due to exposing other issues (as was the case here, leading to this fix). And it's not only some obscure platforms, but both macOS and two of the three Linux platforms. But we should discuss this on the PR itself, not here...

junaire · 2022-10-10T09:26:48Z

@junaire tests should be clean in CI builds, as you can see for this PR. Any failures are introduced by the code changes, possibly due to exposing other issues (as was the case here, leading to this fix). And it's not only some obscure platforms, but both macOS and two of the three Linux platforms. But we should discuss this on the PR itself, not here...

Sorry about discussing unrelated content here, I sent you a message in mattermost. Anyway, thanks for your help!

phsft-bot · 2022-10-10T09:56:40Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2022-10-10T10:06:48Z

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2022-10-10T10:06:28.432Z] FAILED: tmva/sofie/test/CMakeFiles/SofieCompileModels_ROOT.util

phsft-bot · 2022-10-10T10:54:38Z

Build failed on ROOT-ubuntu18.04/nortcxxmod.
Running on sft-ubuntu-1804-3.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.autoloading.headerParsingOnDemand.roottest_root_meta_autoloading_headerParsingOnDemand_no_autoparse_read

phsft-bot · 2022-10-10T11:07:56Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

projectroot.tree.dataframe.test.gtest_tree_dataframe_test_dataframe_cache

vgvassilev · 2022-10-10T12:52:57Z

This change looks good to me, do we have a plan what to do with the current failures?

hahnjo · 2022-10-10T12:57:28Z

@vgvassilev the failure is in a nortcxxmod, that's why I wondered about this case. The previous implementation looking at the SourceLocations worked, so there must be some differences...

vgvassilev · 2022-10-10T13:29:39Z

@vgvassilev the failure is in a nortcxxmod, that's why I wondered about this case. The previous implementation looking at the SourceLocations worked, so there must be some differences...

I suspect that std::pair<edm::Value> returned true which we can classify more accurately now. Template instantiations are a weird beast that can contain both content from a system header and non-system headers. I suspect that hasOwningModule is false. For the PCH usually it is enough to use isFromASTFile.

PS: On a second thought that would probably fail elsewhere..

Avoid loading ROOT modules while Clang is instantiating a template class from STL, leading to ODR checks with an incomplete class.

phsft-bot · 2022-10-11T11:27:41Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2022-10-11T12:03:24Z

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.python.distrdf.common.roottest_python_distrdf_common_test_all

phsft-bot · 2022-10-11T12:46:40Z

Build failed on ROOT-ubuntu18.04/nortcxxmod.
Running on sft-ubuntu-1804-3.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2022-10-11T12:46:21.868Z] fatal error: PCH file '/mnt/build/workspace/root-pullrequests-build/build/etc//allDict.cxx.pch' not found: module file not found
[2022-10-11T12:46:24.399Z] FAILED: tmva/sofie/test/CMakeFiles/SofieCompileModels_ROOT.util

hahnjo · 2022-10-19T08:44:43Z

ping...

pcanal · 2022-10-19T16:15:35Z

disable the callback when we're instantiating templates.

As long as we do before hand the side effects. ie seeing templateclass<MyClass> we need to load the module for MyClass first.

vgvassilev

This looks good to me assuming @pcanal does not have objections.

pcanal

Thanks.

hahnjo added the in:Cling label Oct 7, 2022

hahnjo requested a review from vgvassilev October 7, 2022 12:58

hahnjo self-assigned this Oct 7, 2022

hahnjo requested review from Axel-Naumann and pcanal as code owners October 7, 2022 12:58

pcanal reviewed Oct 7, 2022

View reviewed changes

core/metacling/src/TClingCallbacks.cxx Outdated Show resolved Hide resolved

vgvassilev reviewed Oct 8, 2022

View reviewed changes

hahnjo force-pushed the lookup-stl branch from 1a5bfa1 to eebd104 Compare October 10, 2022 09:56

[cxxmodules] Avoid lookup from system headers

fee7ced

Avoid loading ROOT modules while Clang is instantiating a template class from STL, leading to ODR checks with an incomplete class.

hahnjo force-pushed the lookup-stl branch from eebd104 to fee7ced Compare October 11, 2022 11:27

vgvassilev approved these changes Oct 19, 2022

View reviewed changes

pcanal approved these changes Oct 20, 2022

View reviewed changes

hahnjo merged commit 40aa68d into root-project:master Oct 21, 2022

hahnjo deleted the lookup-stl branch October 21, 2022 06:40

[cxxmodules] Avoid lookup from system headers #11514

[cxxmodules] Avoid lookup from system headers #11514

Uh oh!

Conversation

hahnjo commented Oct 7, 2022

Uh oh!

phsft-bot commented Oct 7, 2022

Uh oh!

Uh oh!

vgvassilev commented Oct 8, 2022

Uh oh!

Uh oh!

vgvassilev Oct 8, 2022

Choose a reason for hiding this comment

Uh oh!

hahnjo Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

hahnjo Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

vgvassilev Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

hahnjo Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

junaire commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junaire commented Oct 8, 2022

Uh oh!

junaire commented Oct 9, 2022

Uh oh!

hahnjo commented Oct 10, 2022

Uh oh!

junaire commented Oct 10, 2022

Uh oh!

hahnjo commented Oct 10, 2022

Uh oh!

junaire commented Oct 10, 2022

Uh oh!

phsft-bot commented Oct 10, 2022

Uh oh!

phsft-bot commented Oct 10, 2022

Errors:

Uh oh!

phsft-bot commented Oct 10, 2022

Failing tests:

Uh oh!

phsft-bot commented Oct 10, 2022

Failing tests:

Uh oh!

vgvassilev commented Oct 10, 2022

Uh oh!

hahnjo commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vgvassilev commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phsft-bot commented Oct 11, 2022

Uh oh!

phsft-bot commented Oct 11, 2022

Failing tests:

Uh oh!

phsft-bot commented Oct 11, 2022

Errors:

Uh oh!

hahnjo commented Oct 19, 2022

Uh oh!

pcanal commented Oct 19, 2022

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

pcanal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

junaire commented Oct 8, 2022 •

edited

Loading

hahnjo commented Oct 10, 2022 •

edited

Loading

vgvassilev commented Oct 10, 2022 •

edited

Loading