Meaningful error messages when number of subscripts differs between compile and runtime by nrnhines · Pull Request #2024 · neuronsimulator/nrn

nrnhines · 2022-10-13T11:00:40Z

Variables have always been allowed to be redeclared. But if the number of subscripts changes after compilation of a procedure, the stack can become corrupted, overflow, or generate a message far from the cause of the error.
This PR generates meaningful error message when the number of subscripts for a variable at compile time differs from the
number of subscripts at runtime due to redeclaring the variable after compiling a function.
A HOC example that used to segfault

double x[1][1][1]
proc foo() { local i
    for i = 1, 10000 {
        x = i
    }
}
double x[1]
foo()

now generates the message

nrniv: array dimension of x now 1 (at compile time it was 3)

…ffers.

azure-pipelines · 2022-10-13T11:34:01Z

✔️ c4cdc82 -> Azure artifacts URL

… three).

azure-pipelines · 2022-10-16T19:29:47Z

✔️ 34bdfda -> Azure artifacts URL

codecov-commenter · 2022-10-20T12:30:00Z

Codecov Report

Merging #2024 (d586408) into master (b47712b) will increase coverage by 0.17%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master    #2024      +/-   ##
==========================================
+ Coverage   48.66%   48.84%   +0.17%     
==========================================
  Files         518      518              
  Lines      115929   116022      +93     
==========================================
+ Hits        56416    56670     +254     
+ Misses      59513    59352     -161

Impacted Files	Coverage Δ
src/nrnoc/secref.cpp	`27.80% <50.00%> (+5.14%)`	⬆️
src/oc/code.cpp	`81.29% <91.66%> (+0.34%)`	⬆️
src/oc/hoc_oop.cpp	`75.25% <96.42%> (+0.69%)`	⬆️
src/nrnpython/nrnpy_hoc.cpp	`69.33% <97.72%> (+2.47%)`	⬆️
src/ivoc/matrix.cpp	`51.08% <100.00%> (+0.35%)`	⬆️
src/nrncvode/netcvode.cpp	`88.50% <100.00%> (ø)`
src/nrnpython/nrnpy_nrn.cpp	`74.32% <100.00%> (+0.01%)`	⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

azure-pipelines · 2022-10-20T13:18:21Z

✔️ eddcadc -> Azure artifacts URL

alexsavulescu · 2022-10-20T14:39:23Z

heavy_check_mark eddcadc -> Azure artifacts URL

https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/3290522300

alexsavulescu · 2022-10-20T16:21:12Z

https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/3290522300

~30 models now exhibit dimension errors

nrnhines · 2022-10-21T11:53:47Z

~30 models now exhibit dimension errors

Can you rerun. Any that are Vector[0].x is array not function. Use x[...] syntax can be fixed easily. If some are not, I'll have to look in more detail

azure-pipelines · 2022-10-21T12:15:41Z

✔️ 4df6b9c -> Azure artifacts URL

alexsavulescu · 2022-10-21T16:32:52Z

✔️ 4df6b9c -> Azure artifacts URL

https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/3298992007

alexsavulescu · 2022-10-22T17:11:18Z

✔️ 4df6b9c -> Azure artifacts URL

https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/3298992007

Now we have 25 with is array not function and 1 with array dimension.

olupton · 2022-11-02T09:48:35Z

@olupton This contains a few changes that are also helpful for my changes to olupton/data-structures that focus on validity checking of python _ref_var pointers. If acceptable, merging will help me get going on this again.

OK -- I can review the content of the PR. The last run of the ModelDB CI (https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/3344566067) was still showing some differences with these changes, e.g.:

model 168314 %neuron-executable%: Matrix[7].x is array not function. Use x[...] syntax
models 187610, 237728, 258867 and 260967 %neuron-executable%: bad stack access: expecting double; really stack_ndim_datum 1

Is this expected / something you're aware of and happy with?

nrnhines · 2022-11-02T09:56:57Z

model 168314 %neuron-executable%: Matrix[7].x is array not function. Use x[...] syntax
models 187610, 237728, 258867 and 260967 %neuron-executable%: bad stack access: expecting double; really stack_ndim_datum 1

Thanks! I guess it is back to the drawing board in regard to the bad stack access. I'll have to examine that more closely. The Matrix fix I need to do in ModelDB.

azure-pipelines · 2022-11-02T10:06:01Z

✔️ 007145e -> Azure artifacts URL

azure-pipelines · 2022-11-02T15:06:37Z

✔️ a88d395 -> Azure artifacts URL

nrnhines · 2022-11-02T15:35:13Z

Decided to allow legacy Matrix.x(i,j) to avoid changing a model in ModelDB. Hopefully, the nrn-modeldb-ci will pass now. I'll try my hand at getting it started.

nrnhines · 2022-11-02T16:00:26Z

Sorry. Could not remember how to specify use of the hines/vardimfix branch for a nrn-modeldb-ci action.

azure-pipelines · 2022-11-02T16:04:53Z

✔️ d586408 -> Azure artifacts URL

olupton · 2022-11-02T16:35:39Z

Sorry. Could not remember how to specify use of the hines/vardimfix branch for a nrn-modeldb-ci action.

It uses the Python wheels, so you take the URL posted by the bot:

✔️ d586408 -> Azure artifacts URL

and manually launch the pipeline:

which I did to get https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/3379383559

nrnhines · 2022-11-02T17:58:00Z

Hmm. Another surprise. 114355 now generates a bad stack access: expecting stack_ndim_datum; really double
Will look into it.

azure-pipelines · 2022-11-02T19:38:27Z

✔️ a2cce15 -> Azure artifacts URL

olupton

Generally looks good to me, I launched https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/3383688282 comparing this against neuron-nightly (i.e. the current master) to check one more time.

olupton · 2022-11-03T08:46:54Z

Generally looks good to me, I launched https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/3383688282 comparing this against neuron-nightly (i.e. the current master) to check one more time.

This one looks good. We should follow up later on today's diffs between neuron (8.2.1) and neuron-nightly (master).

azure-pipelines · 2022-11-03T12:31:14Z

✔️ 55f75f9 -> Azure artifacts URL

pramodk · 2022-11-04T08:17:17Z

@wvangeit @DrTaDa: Just want to inform you about this PR/change in the current master / future releases of NEURON. In one of our internal GitLab CI test I saw today error like:

This is one of the old circuits (accessible in /gpfs/bbp.cscs.ch/project/proj12/jenkins/cellular/circuit-1k/) and may not be relevant for new circuits but mentioning this anyway.

Just a note: On our internal discussion @olupton clarified following:
There will be some kind of variable called axon that was initially declared/used as a scalar and subsequently got used as a vector. That was never safe, and now it’s an error…
(note that in the description of the PR, “compile time” generally means HOC compile time — roughly when the .hoc file is parsed, not C/C++/CMake/… compile time)

olupton · 2022-11-04T15:12:40Z

@nrnhines a distilled version of the above failure (thanks a lot to @WeinaJi 💪🎉 ) is:

begintemplate Cell
public axon
create axon[2]
endtemplate Cell
objref CellRef
CellRef = new Cell()
// Expression A:
CellRef.axon[1](0)
// Expression B:
CellRef.axon[0] connect CellRef.axon[1](0), 1

where expression A produces:

nrniv: [...](...) syntax only allowed for array range variables: axon

and if it is commented out, expression B produces:

nrniv: array dimension of axon now 1 (at compile time it was 0)
  in vardimtests/test10.hoc near line 8
  CellRef.axon[0] connect CellRef.axon[1](0), 1
                                               ^

what do you think the best fix would be here?

This demonstrates an error that started appearing in BBP-internal models after #2024 was merged.

nrnhines · 2022-11-04T15:40:59Z

CellRef.axon[1](0)

Expression A is an invalid statement in hoc world (in python world it refers to the segment at x=0)
Expression B is supposed to work. I'll look into it now. It is a special case I missed.

Fixes an error introduced by #2024 in some models.

nrnhines added 2 commits October 12, 2022 17:22

stack_ndim_datum type for error when compile time and runtime nsub di…

0bf5b53

…ffers.

Fix 'make test' places where extra ndim stack item was unexpected.

c4cdc82

nrnhines marked this pull request as draft October 13, 2022 11:00

nrnhines added 6 commits October 13, 2022 14:40

black format

3a36f3f

Disallow use of [][]... syntax for function arg lists.

4b9eb00

Save some check array program space (now usually two slots instead of…

0073300

… three).

Error messages when python accesses a hoc var with changed dimensions.

32452b6

added dimension changed tests

242f16c

black for python formatting

34bdfda

This was referenced Oct 17, 2022

Add test showing bug with HOC variables that get redeclared with different numbers of indices #2009

Closed

Make stack error recovery more robust #2026

Merged

alexsavulescu added 3 commits October 20, 2022 13:56

import3d_sec.hoc: contourcenter: perim.x is an array

a0ddf72

rxd tests: only one pump.mod

c282ad1

Merge branch 'master' into hines/vardimfix

07e5371

alexsavulescu added 2 commits October 20, 2022 14:45

format

33e7fda

format...

eddcadc

nrnhines marked this pull request as ready for review October 20, 2022 14:40

nrnhines added 2 commits October 21, 2022 13:42

Error message for Vector.x(...) instead of Vector.x[...]

56e3c1a

Merge branch 'master' into hines/vardimfix

4df6b9c

alexsavulescu mentioned this pull request Oct 21, 2022

ctest/pytest: use --capture=tee-sys #2035

Merged

SectionRef.child[index] always needs index.

a88d395

Allow legacy Matrix.x(i,j) as well as recommened Matrix.x[i][j]

d586408

Allow Section.child(i) syntax as well as recommended Section.child[i]

a2cce15

olupton approved these changes Nov 3, 2022

View reviewed changes

Comment thread share/lib/hoc/import3d/import3d_sec.hoc

Comment thread src/nrnoc/secref.cpp Outdated

Comment thread src/oc/code.cpp Outdated

nrnhines added 3 commits November 3, 2022 11:47

simplify to no arg to bool hoc_stack_type_is_ndim()

6a197e0

Replace Vector.x(i) syntax with recommended Vector.x[i]

258269e

format

55f75f9

nrnhines merged commit a8362dc into master Nov 3, 2022

nrnhines deleted the hines/vardimfix branch November 3, 2022 14:07

olupton added a commit that referenced this pull request Nov 4, 2022

Add new HOC test.

6df4e53

This demonstrates an error that started appearing in BBP-internal models after #2024 was merged.

olupton mentioned this pull request Nov 4, 2022

Add new HOC test. #2065

Closed

alexsavulescu mentioned this pull request Nov 4, 2022

Should not allow array (re)declarations that change the number of indices #2010

Closed

nrnhines mentioned this pull request Nov 4, 2022

section connect obj.secarray[i](0), 1 is valid syntax #2066

Merged

olupton pushed a commit that referenced this pull request Nov 4, 2022

section connect obj.secarray[i](0), 1 is valid syntax (#2066)

67e5614

Fixes an error introduced by #2024 in some models.

olupton mentioned this pull request Feb 27, 2023

Fix: array dimension of o now 1 (at compile time it was 0) ModelDBRepository/146949#3

Merged

Uh oh!

Conversation

nrnhines commented Oct 13, 2022

Uh oh!

azure-pipelines Bot commented Oct 13, 2022

Uh oh!

azure-pipelines Bot commented Oct 16, 2022

Uh oh!

codecov-commenter commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

azure-pipelines Bot commented Oct 20, 2022

Uh oh!

alexsavulescu commented Oct 20, 2022

Uh oh!

alexsavulescu commented Oct 20, 2022

Uh oh!

nrnhines commented Oct 21, 2022

Uh oh!

azure-pipelines Bot commented Oct 21, 2022

Uh oh!

alexsavulescu commented Oct 21, 2022

Uh oh!

alexsavulescu commented Oct 22, 2022

Uh oh!

olupton commented Nov 2, 2022

Uh oh!

nrnhines commented Nov 2, 2022

Uh oh!

azure-pipelines Bot commented Nov 2, 2022

Uh oh!

azure-pipelines Bot commented Nov 2, 2022

Uh oh!

nrnhines commented Nov 2, 2022

Uh oh!

nrnhines commented Nov 2, 2022

Uh oh!

azure-pipelines Bot commented Nov 2, 2022

Uh oh!

olupton commented Nov 2, 2022

Uh oh!

nrnhines commented Nov 2, 2022

Uh oh!

azure-pipelines Bot commented Nov 2, 2022

Uh oh!

olupton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olupton commented Nov 3, 2022

Uh oh!

azure-pipelines Bot commented Nov 3, 2022

Uh oh!

pramodk commented Nov 4, 2022

Uh oh!

olupton commented Nov 4, 2022

Uh oh!

nrnhines commented Nov 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Oct 20, 2022 •

edited

Loading