NEURON uses reserved identifiers & doesn't define its API

[olupton]

Context

This issue mixes two somewhat different issues, on the basis that it makes sense to solve / consider them at the same time:

• As discussed in https://github.com/neuronsimulator/nrn/issues/1963, the NEURON model of linking together code compiled at different times with different toolchains on different machines imposes implicit restrictions on what data types can be used in what places. It is currently difficult to express succinctly and accurately what those restrictions they are and which methods they apply to, let alone provide useful diagnostics and enforcement (via CI) that the restrictions are respected. Mistakes tend to result in non-obvious and painful to debug issues with Python wheels.
• NEURON itself, and the code it generates using nocmodl, uses identifiers that are reserved for the C++ implementation (they start with underscores in the global namespace, see https://en.cppreference.com/w/cpp/language/identifiers). This is bad practice and may lead to surprising problems in the future.

Overview of the issue

Currently the main issue linked to https://github.com/neuronsimulator/nrn/issues/1963 relates to the pre- and post-C++11 ABI for std::string. In the manylinux2014 image used to build Python wheels, the old C++11-noncompliant version of std::string is used. When nrnivmodl is run on a user machine using any reasonable toolchain, the C++11 compliant version is used. See this page for more information.

This issue with std::string will likely (to be checked!) go away when we eventually move to using manylinux_2_28-based images to build the Python wheels. One thing to explore at that stage will be whether it causes issues to run nrnivmodl on a user machine with a toolchain based on an older GCC than was used to compile NEURON. manylinux_2_28 images currently use GCC 12.

One thing we can do to reduce exposure to issues such as this one would be to define and restrict the interface between code generated from MOD files (which is built inside nrnivmodl) and code that forms part of the main NEURON library (which is built earlier, potentially on a different machine using a different toolchain). At the moment this interface is only implicitly defined, and there is no explicit restriction on what data types may be passed back and forth across the interface between generated code and the NEURON library.

If this interface was explicitly defined, it might be possible to use tools like abi-compliance-checker to check consistency between what we obtain in a "standard" build and what we obtain when using the Python wheels.

As part of explicitly defining this interface, we could also reorganise the naming conventions to avoid reserved identifiers.

The use of leading underscores is widespread because NMODL variables defined in MOD files cannot start with underscores, so this avoids clashes with the preprocessor macros that nocmodl generates to represent NMODL variables.

Expected result/behavior

Everything should work and we should never have to debug weird Python-wheel-specific bugs ever again.

Suggested solution

A tentative suggestion for how to address the above is as follows:

• nocmodl should stop emitting its own declarations for methods hidden inside the NEURON library.
  • otherwise they will be interpreted as declarations of neuron::mechanism::(anonymous namespace)::foo
• All methods that can be called from code generated from MOD files should live in a new neuron::mechanism:: namespace
  • This explicitly defines the interface discussed above.
  • Everything (functions, variables, sub-namespaces) inside that namespace should use names starting with _
  • For example, we might generate _register::_thread_table(...) instead of _nrn_thread_table_reg(...) in translated code, which would call neuron::mechanism::_register::_table_thread(...) (because the generated call is inside the neuron::mechanism namespace)
• The code generated from MOD files should be of the form:

  #include <...>
  #include <...>
  ... presumably global-scope VERBATIM blocks must go here because they might #include stuff ...
  namespace neuron::mechanism {
  namespace { // anonymous namespace, ~the same as declaring everything inside `static` / with internal linkage
  constexpr auto _some_constant = ...; // OK, not a reserved identifier because not in the global namespace
  using _local_type = ...; // OK, not reserved
  template <typename T>
  using _std_vector = std::vector<T>; // pattern can be used if we want to generate code using vectors, but `std` or `vector` is an NMODL variable name
  #define some_nmodl_variable ...
  ... generated code similar to today ...
  void foo() {
  _helpers::_some_helper(); // use something in neuron::mechanism::_helpers that is exported by the NEURON library
  }
  }  // anonymous namespace
  }  // namespace neuron::mechanism

• Methods under the neuron::mechanism:: namespace should be required to use "simple" data types. For example, std::string and types containing std::string member variables are banned. For example, we might only allow the use of plain C/C++ data types and T* where T is a forward-declared type.
• We should add MOD files to the test suite that declare "difficult" variable names, e.g.

  NEURON  {
  SUFFIX NameClashes
  RANGE detail, ends_with_underscore_, mechanism, Memb_list, neuron, Node, nrn, NrnThread, Prop, std
  ...

  (for extra "fun": define variables called int, double, ...)

• (We should potentially rename the nmodl_variable_columnindex macros; the ends_with_underscore_ example above results in ends_with_underscore__columnindex, which contains a double underscore. That is a reserved identifier, but I am not 100% sure if it is a permissible macro name -- probably not)

[olupton]

cc: @alexsavulescu @pramodk @nrnhines; feedback welcome...

[nrnhines]

The ABI part of the issue seems hard. Is it similar to an old issue with Windows where FILE was different between mingw and Python2.3 ? I believe libnrnpython3.so is ABI compatible with Python3.x and the only thing requiring different libraries for each x was the cython files. Does it seem useful to have libnrniv provide a list of structures and sizes and compare those sizes when a translated mod file is compiled or loaded? (e.g when the _reg_modname function is called?)

I take it, the namespace idea above is designed to take care of the name conflict part of the issue. You are right about the origin of the prefix _ hack for any name that could conflict with the outside world. #define has always been a problem for names like v, dt, t, in VERBATIM blocks. But the _ prefix is certainly easily changeable to something else.

I am hoping that NMODL overcomes all name conflict issues introduced by #define. Replacement of mod2c and nocmodl has been a goal from the beginning.

Bottom line is that your suggested solution seems reasonable and I'd be happy to help with it.

[olupton]

One consequence of a rule like:

For example, we might only allow the use of plain C/C++ data types and T* where T is a forward-declared type.

Is that it implies that VERBATIM blocks inside MOD files cannot access member variables of structs such as Node and Prop; such access would need to be redirected via accessor functions defined in NEURON itself. See https://github.com/ModelDBRepository/244262/pull/2 for an example of this in practice.

[nrnhines]

Looks like there might be a chance that FILE is an issue again with Windows11 and (at least Python11). The symptom is

nrniv -python

immediately exits to the terminal after the banner. (or after executing the arguments). Gdb says the issue is an unhandled signal way inside a system dll called from PyOS_readline a dozen or so python deep stack calls from

    PyRun_InteractiveLoop(hoc_fin, "stdin");

in nrn/src/nrnpython/nrnpython.cpp. The problem goes away with the temporary hack work around:

            char* z[1];
            z[0] = strdup("nrniv");
            Py_BytesMain(1, z);
            //PyRun_InteractiveLoop(hoc_fin, "stdin")