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

[cxxmodules] Add initial implementation of the semantic GMI. #5094

Merged

Conversation

vgvassilev
Copy link
Member

No description provided.

@vgvassilev vgvassilev requested a review from pcanal as a code owner March 6, 2020 14:07
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14
How to customize builds

@vgvassilev
Copy link
Member Author

@phsft-bot build!

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

@phsft-bot
Copy link
Collaborator

@vgvassilev vgvassilev force-pushed the cxxmodules_enable_gmi_by_default branch from fd9cca9 to a7869de Compare March 28, 2020 16:28
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

@phsft-bot
Copy link
Collaborator

@vgvassilev
Copy link
Member Author

We seem to fail to generate the dtor of ParamFunctorHandler via instantiating the ctor ~ParamFunctorHandler -> ~ParamFunctionBase-> ~ParamFunctorHandler. For some reason the template instantiator thinks that it does not need to instantiate the dtor.

Minimal reproducer:

cat stressMC_reduced.cxx 
#include <TTree.h>
#include <TF1.h>
using namespace ROOT::Math;

class StatFunction : public ROOT::Math::IParamFunction {
public:

   unsigned int NPar() const override { return 0; }
   const double * Parameters() const override { return nullptr; }
   ROOT::Math::IGenFunction * Clone() const override { return nullptr; }
   void SetParameters(const double * p) override { }

   // test cumulative function
  void Test() {
   TF1 * f = new TF1("ftemp",ParamFunctor(*this),0,0,0);
  }


private:


   double DoEvalPar(double x, const double * ) const override {
     return 0;
   }

};


typedef StatFunction Dist_beta;
int testStatFunctions() {
   Dist_beta dist;
   dist.Test();
   return 0;
}


class VectorTest {
public:
   template<class V>
   double testWrite(V & dataV, std::string typeName="") {
      TTree tree("VectorTree","");

      // need to add namespace to full type name
      if (typeName == "") {
      }

      tree.Fill();

      return 0;
   }
};

int stressMC_reduced(double nscale = 1) {
   return 0;
}

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-fedora31/noimt.
Running on root-fedora-31-1.cern.ch:/build/workspace/root-pullrequests-build
See cdash.
See console output.

Failing tests:

@vgvassilev vgvassilev force-pushed the cxxmodules_enable_gmi_by_default branch from ce22a8f to 41189a1 Compare June 16, 2020 13:31
@vgvassilev vgvassilev marked this pull request as ready for review June 16, 2020 13:31
@vgvassilev vgvassilev requested a review from etejedor as a code owner June 16, 2020 13:31
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac1015/cxx17, windows10/cxx14
How to customize builds

@vgvassilev vgvassilev force-pushed the cxxmodules_enable_gmi_by_default branch from 41189a1 to 1ac186c Compare June 16, 2020 14:43
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac1015/cxx17, windows10/cxx14
How to customize builds

@vgvassilev vgvassilev changed the title [cxxmodules] Enable global module indexing by default. [cxxmodules] Add initial implementation of the semantic GMI. Jun 16, 2020
@vgvassilev
Copy link
Member Author

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac1015/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

@vgvassilev
Copy link
Member Author

Build failed on ROOT-performance-centos7-multicore/default.
Running on olhswep22.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See cdash.
See console output.

Failing tests:

* [projectroot.test.test_stressIOPlugins_xroot](https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/87069/testReport/projectroot/test/test_stressIOPlugins_xroot/)

* [projectroot.tree.treeplayer.test.gtest_tree_treeplayer_test_treeprocessormt_remotefiles](https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/87069/testReport/projectroot.tree.treeplayer/test/gtest_tree_treeplayer_test_treeprocessormt_remotefiles/)

* [projectroot.runtutorials.tutorial_dataframe_df102_NanoAODDimuonAnalysis](https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/87069/testReport/projectroot/runtutorials/tutorial_dataframe_df102_NanoAODDimuonAnalysis/)

* [projectroot.runtutorials.tutorial_dataframe_df101_h1Analysis](https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/87069/testReport/projectroot/runtutorials/tutorial_dataframe_df101_h1Analysis/)

* [projectroot.runtutorials.tutorial_dataframe_df103_NanoAODHiggsAnalysis](https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/87069/testReport/projectroot/runtutorials/tutorial_dataframe_df103_NanoAODHiggsAnalysis/)

* [projectroot.runtutorials.tutorial_tmva_tmva103_Application](https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/87069/testReport/projectroot/runtutorials/tutorial_tmva_tmva103_Application/)

Those fail all over the place.

} else if (NamespaceDecl *NSD = llvm::dyn_cast<NamespaceDecl>(R))
Register(NSD, /*AddSingleEntry=*/ false);
else if (TypedefNameDecl *TND = dyn_cast<TypedefNameDecl>(R))
Register(TND);
Copy link
Member

Choose a reason for hiding this comment

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

Can't typedef decl be repeated (in several header files and thus several modules)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it should define the same entity, otherwise modules ODR hash will complain. So whichever we pick should be fine. I suspect that may lead to loading the 'wrong' module to resolve the intended typedef but cannot do anything better.

Alternatively, we could load all modules which have this typedef name -- that would be redundant because if the typedefs are all the same what's the point; if the typedefs are different we will get an ODR violation diagnostic.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to load the pcm for the target of the typedef?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we need to load something that has a clang::Decl for the missing identifier

// ... unless it's a .pcm.lock file, which indicates that someone is
// in the process of rebuilding a module. They'll rebuild the index
// at the end of that translation unit, so we don't have to.
if (llvm::sys::path::extension(D->path()) == ".pcm.lock")
Copy link
Member

Choose a reason for hiding this comment

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

What happens in the follow scenario:

(1) Process A starts and build a module and thus creates a .pcm.lock
(2) Process A is put to sleep (and/or is very very slow)
(3) some other pcm is created fully (and have names such it is early in the directory iteration).
(4) Process B starts and try to write the index and gives ups
(5) Process B finishes
(6) Process A wakes ups and finish write the module.idx.

Are the extra pcm creates in step (3) included in the module.idx (create by Process A)

Copy link
Member Author

Choose a reason for hiding this comment

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

We will not add it in the index probably.

Note that this is pre-existing llvm code and the ROOT GMI does not actually use it. It provides a table with ExternalIDs

Copy link
Member

Choose a reason for hiding this comment

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

We will not add it in the index probably.

I see. Then the comment is not appropriate for our use case [And the result is (silently) sub-optimal, isn't it?]

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. Let’s see if this will improve when we upgrade llvm.

@vgvassilev vgvassilev force-pushed the cxxmodules_enable_gmi_by_default branch from 1ac186c to 6c26e5f Compare June 16, 2020 16:40
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac1015/cxx17, windows10/cxx14
How to customize builds

@vgvassilev vgvassilev merged commit f6e69e8 into root-project:master Jun 17, 2020
@vgvassilev vgvassilev deleted the cxxmodules_enable_gmi_by_default branch June 17, 2020 04:36
@phsft-bot
Copy link
Collaborator

Build failed on ROOT-fedora31/noimt.
Running on root-fedora-31-1.cern.ch:/build/workspace/root-pullrequests-build
See cdash.
See console output.

Errors:

  • [2020-06-17T05:08:36.625Z] CMake Error at /build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:950 (message):

@vgvassilev
Copy link
Member Author

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants