869 make command-line arguments in VT non-static

[cz4rs]

draft for further discussion regarding #869

• promote ArgConfig to a component
• use default member initialization
• remove unnecessary includes from headers when possible

Fixes #869

[codecov]

Codecov Report

Merging #898 into develop will decrease coverage by 0.10%.
The diff coverage is 83.20%.

@@             Coverage Diff             @@
##           develop     #898      +/-   ##
===========================================
- Coverage    77.37%   77.26%   -0.11%     
===========================================
  Files          655      656       +1     
  Lines        25055    25076      +21     
===========================================
- Hits         19386    19376      -10     
- Misses        5669     5700      +31     

Impacted Files	Coverage Δ
src/vt/messaging/active.h	87.09% <ø> (ø)
src/vt/runtime/component/component_pack.h	100.00% <ø> (ø)
src/vt/runtime/runtime.h	80.00% <ø> (ø)
src/vt/termination/graph/epoch_graph.cc	0.00% <0.00%> (ø)
src/vt/termination/termination.h	100.00% <ø> (ø)
src/vt/trace/file_spec/spec.h	100.00% <ø> (ø)
src/vt/vrt/collection/balance/elm_stats.h	100.00% <ø> (ø)
...c/vt/vrt/collection/balance/lb_invoke/lb_manager.h	100.00% <ø> (ø)
src/vt/vrt/collection/balance/node_stats.cc	46.55% <0.00%> (ø)
src/vt/vrt/collection/balance/read_lb.h	53.33% <ø> (ø)
... and 32 more

[cz4rs]

commit c6b68a6 removes all invocations of vt::theArgConfig that happen before Runtime::initializeComponents() is called, so at least some tests / examples can run without seg faults (this gives me 98% of tests passing on my machine and only 30% in CI, but that's for later analysis)

• most of the calls come from vt::runtime::Runtime constructor, with arguments::ArgParse::parse being prime example
• debug_print uses debug family arguments, so it would have to be avoided early or have specialized version with default options
• Context depends on user_argc_ and user_argv_, so it cannot be registered without getting these

@lifflander @pnstickne do you guys have any advice on this? does it even make sense to move initializeComponents() to the beginning of Runtime constructor?

[lifflander]

@lifflander @pnstickne do you guys have any advice on this? does it even make sense to move initializeComponents() to the beginning of Runtime constructor?

Ah, I see the problem that you are encountering. Basically, the arg config being a component means it starts up with the pack, but the vt::Runtime uses arguments pre-startup to configure the startup and output options. I think you should construct the arg config outside of the components pack and then move it into place for the component. So construct it early in each runtime. Let me know if you have questions on how to achieve this.

[cz4rs]

a couple of builds are failing when dereferencing MPI_Comm* comm during construction of theContext

Program received signal SIGSEGV, Segmentation fault.
0x00005555556fedda in vt::ctx::Context::Context (this=0x55555587b6d0, argc=<optimized out>, 
    argv=<optimized out>, is_interop=<optimized out>, comm=0x18)
77	  if (comm != nullptr and *comm != MPI_COMM_NULL) {
(gdb) bt
#0  0x00005555556fedda in vt::ctx::Context::Context (this=0x55555587b6d0, argc=<optimized out>, 
    argv=<optimized out>, is_interop=<optimized out>, comm=0x18)
#1  0x0000555555692ca3 in std::make_unique<vt::ctx::Context, int&, char**, bool&, int*> ()
#2  vt::runtime::component::ComponentConstructor<vt::ctx::Context, void, int&, char**, bool&, int*>::apply<int&, char**, bool&, int*> () 
#3  vt::runtime::component::Component<vt::ctx::Context>::staticInit<int&, char**, bool&, int*> ()
#4  vt::runtime::component::ComponentPack::registerComponent<vt::ctx::Context, vt::arguments::ArgConfig, int&, char**, bool&, int*>(vt::ctx::Context**, vt::runtime::component::BaseComponent::DepsPack<vt::arguments::ArgConfig>, int&, char**&&, bool&, int*&&)::{lambda()#1}::operator()() (this=<optimized out>)
#5  std::_Function_handler<void (), vt::runtime::component::ComponentPack::registerComponent<vt::ctx::Context, vt::arguments::ArgConfig, int&, char**, bool&, int*>(vt::ctx::Context**, vt::runtime::component::BaseComponent::DepsPack<vt::arguments::ArgConfig>, int&, char**&&, bool&, int*&&)::{lambda()#1}>::_M_invoke(std::_Any_data const&)
#6  0x00005555556b491a in std::function<void ()>::operator()() const (this=<optimized out>)
#7  vt::runtime::component::ComponentPack::construct (this=0x55555587ad60)
#8  0x000055555568bb44 in vt::runtime::Runtime::initializeComponents (this=this@entry=0x555555872040)
(...)

this doesn't happen when using Debug builds and even in the CI all builds apart from 4 gcc configurations are passing

I have tracked down the change that introduces this - capturing by reference when registering components:

@@ registry::AutoHandlerType ComponentPack::registerComponent(
-      [ref,this,cons...]() mutable {
+      [ref,this,&cons...]() mutable {

when we pass the MPI_Comm to registerComponent before the construction it has valid value, but somehow it gets changed into something like 0x18 (as in the stacktrace above)

edit:
after discussion, the right direction would be not to capture by reference
this requires solving following error when passing std::unique_ptr into component:

vt/runtime/component/component_pack.impl.h:68:7: error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = vt::arguments::ArgConfig; _Dp = std::default_delete<vt::arguments::ArgConfig>]’
   68 |       [ref,this,cons...]() mutable {
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
(...)
/usr/include/c++/9/bits/unique_ptr.h:414:7: note: declared here
  414 |       unique_ptr(const unique_ptr&) = delete;
      |       ^~~~~~~~~~

[lifflander]

@cz4rs I have a proper fix for the problem that supports move-only parameters along with l-val refs.

#972

You will want to remove the ComponentConstructor changes (for detecting arguments) from your branch as they will be merged as part of this #972 now.

[pnstickne]

I recommend using local variables for local mutation, saving the assignment to global state until as late as possible.

This reduces expressions and allows identification of "when" global modification occurs my limitation potential candidates.

[pnstickne]

#include "vt/runtime/component/component.h" should be removed from the header when not needed.

[lifflander]

I'm working a little refactoring to print before the runtime starts up. This is very useful for debugging so I don't want to loose that functionality.

[lifflander]

Here is an overview of what got changed by this pull request:

Clones removed
==============
+ src/vt/runtime/runtime.cc  -2
         

See the complete overview on Codacy

[lifflander]

there's a couple of calls, most importantly we probably want to respect vt_quiet
still, even when we want to check if theConfig is null, we have to first go through CUR_RT macro in runtime_get.cc, and this ends up depending on theContext

@cz4rs I've resolved a few problems.

• Added the debug prints back in, and now -vt_quiet is respected even when VT is not fully initialized or it is finalized (as long as vt::Runtime is constructed.
• Moved the shutdown back to where it was before
• Fixed a bug you introduced in test_signal_cleanup (not actually cleaning up) by changing it back
• Wrote detailed comments in runtime.cc about the lifecycle of the ArgConfig

  /// =========================================================================
  /// Notes on lifecycle for the ArgConfig/AppConfig
  /// =========================================================================
  ///
  /// - After \c vt::Runtime is constructed, the ArgConfig lives in
  ///   \c arg_config_ and \c app_config_ contains a pointer to the internals.
  ///
  /// - After the pack registers the ArgConfig component, \c arg_config_ is no
  ///   longer valid. The config is in a tuple awaiting construction---thus, we
  ///   can't easily access it. app_config_ remains valid during this time
  ///
  /// - After construction, the \c arg_config_ is in the component
  ///   \c theConfig() and can be accessed normally
  ///
  /// - After \c Runtime::finalize() is called but before the pack is destroyed,
  ///   we extract the \c ArgConfig from the component and put it back in
  ///   \c arg_config_ for use after.
  ///
  /// - From then on, until the \c vt::Runtime is destroyed or VT is
  ///   re-initialized \c arg_config_ will contain the configuration.
  ///
  ///  For this to all work correctly, the \c vt_debug_print infrastructure
  ///  calls \c vt::config::getConfig() which always grabs the correct app
  ///  config, either from the component singleton or the \c vt::Runtime
  ///
  /// =========================================================================

• Added an extraction method that is used before the component pack is destroyed so the arg_config_ is not lost (in case you re-init the runtime) and is moved back into place
• Fixed some const issues---made access to AppConfig safer in some contexts

[cz4rs]

@lifflander Great, thanks for the fixes and improvements (especially for documetning the lifecycle!)