I tried to debug the issue, when compiling a MOD file called ca_ch. My current understanding is the following.

We can see nrn_init_ca_ch being called from:

void allocate_data_in_mechanism_nrn_init() {
    _nrn_skip_initmodel = true;
    for (int i = 0; i < nrn_nthread; ++i) {  // could be parallel
        NrnThread& nt = nrn_threads[i];
        for (NrnThreadMembList* tml = nt.tml; tml; tml = tml->next) {
            Memb_list* ml = tml->ml;
            mod_f_t s = corenrn.get_memb_func(tml->index).initialize;
            if (s) {
                (*s)(&nt, ml, tml->index);
            }
        }
    }
    _nrn_skip_initmodel = false;
}

https://github.com/neuronsimulator/nrn/blob/17be061e031701736cb03a5088b7ddf253a069a4/src/coreneuron/sim/finitialize.cpp#L21-L38?plain=1
From which we've learnt that it's sending down tml pointers. Which might be allocated/populated like this:

void Phase2::populate(NrnThread& nt, const UserParams& userParams) {
    // ...
    auto& nrn_prop_param_size_ = corenrn.get_prop_param_size();
    auto& nrn_prop_dparam_size_ = corenrn.get_prop_dparam_size();
    // ...

    for (auto tml = nt.tml; tml; tml = tml->next) {
        Memb_list* ml = tml->ml;
        int type = tml->index;
        int layout = corenrn.get_mech_data_layout()[type];
        int n = ml->nodecount;
        int sz = nrn_prop_param_size_[type];
        offset = nrn_soa_byte_align(offset);

        ml->data = nt._data + offset;
        offset += nrn_soa_padded_size(n, layout) * sz;
    }
    // ...
}

https://github.com/neuronsimulator/nrn/blob/17be061e031701736cb03a5088b7ddf253a069a4/src/coreneuron/io/phase2.cpp#L937-L1344?plain=1
Summary:

• For each mech we set the pointer to be offset into some global allocation.
• offset is the product of nrn_soa_padded_size(n, layout) which is essentially the number of rows in the SoA (including some padding); and sz.
• sz is whatever is stored in nrn_prop_param_size_.

So we now need to hunt down how nrn_prop_param_size_ is set. Most likely from here:

void hoc_register_prop_size(int type, int psize, int dpsize) {
    // ...
    corenrn.get_prop_param_size()[type] = psize;
    corenrn.get_prop_dparam_size()[type] = dpsize;
    // ...
}

https://github.com/neuronsimulator/nrn/blob/17be061e031701736cb03a5088b7ddf253a069a4/src/coreneuron/mechanism/register_mech.cpp#L192-L206?plain=1
Which is a function called from compiled mod files, e.g.,

        hoc_register_prop_size(mech_type, float_variables_size(), int_variables_size());

with

    static inline int float_variables_size() {
        return 23;
    }

We can see it crash here:
https://github.com/neuronsimulator/nrn/actions/runs/8298830006/job/22713118409?pr=2779#step:16:3877

in a manner that would be consistent with the bug, e.g. a SEGFAULT.

@pramodk I think this is the type of test we were thinking of. Please let me know if you see anything wrong with the test.

✔️ d3920b5 -> Azure artifacts URL

This looks suspicious, because data pads the number of rows:

This is currently my understanding of what's affected. Updated: 2024-03-22 10:06

@pramodk I think this is the type of test we were thinking of. Please let me know if you see anything wrong with the test.

the test LGTM.

(Sorry, somehow this mention was not filtered in my mailbox, and hence went into the pile of GitHub emails)

I've pushed the changes I've made so far. The test_red_green.py now passes (locally):

Clearly, a lot of refactoring and more testing left to be done.

This sketches out how it works for recording variables.

This is what what I understand about transferring data/pointers from NRN to CoreNEURON. If there's pointers to array RANGE variables, they'll likely be incorrect.

We can also see that we need a way of getting the substructure, i.e. array_dims and array_prefix_sums.

Please retry analysis of this Pull-Request directly on SonarCloud

✔️ a48a578 -> Azure artifacts URL

✔️ b59524a -> Azure artifacts URL

✔️ 4b74073 -> Azure artifacts URL

While reviewing my own code before suggesting to merge, I noticed that there was a "TODO fix". The comment was there because at the time I was hoping that the code was unused. I think that's wrong, because permute happens when using the GPU. Therefore, the copy back was likely wrong under those circumstances.

I've pushed a commit to address the issue: 3f3221c

✔️ 3f3221c -> Azure artifacts URL

Please retry analysis of this Pull-Request directly on SonarCloud

✔️ 0faf5f5 -> Azure artifacts URL

Logfiles from GitLab pipeline #206322 (:white_check_mark:) have been uploaded here!

Status and direct links:

• ✅ mac_m1_cmake_build: [cmake, ON, OFF, OFF, address]
• ✅ spack_setup
• ✅ build:nmodl
• ✅ build:neuron:nmodl:intel:legacy
• ✅ build:neuron:nmodl:intel:shared
• ✅ build:neuron:nmodl:nvhpc:acc:legacy
• ✅ build:neuron:nmodl:nvhpc:acc:shared
• ✅ build:neuron:nmodl:nvhpc:omp:legacy
• ✅ build:neuron:nmodl:nvhpc:omp
• ✅ test:neuron:nmodl:intel:legacy
• ✅ test:neuron:nmodl:intel:shared
• ✅ test:neuron:nmodl:nvhpc:acc:legacy
• ✅ test:neuron:nmodl:nvhpc:acc:shared
• ✅ test:neuron:nmodl:nvhpc:omp:legacy
• ✅ test:neuron:nmodl:nvhpc:omp

@nrnhines @pramodk please read: #2779 (comment). It explains a recent change and there's one more commit which is clean up of that change.

I think the PR is now ready to merge.

please read: #2779 (comment). It explains a recent change and there's one more commit which is clean up of that change.

that certainly make sense!

Good to merge! 🚀