Skip to content

Avoid segfault if user fails to call setdata before a mech FUNCTION.#2460

Closed
nrnhines wants to merge 20 commits into
masterfrom
hines/setdata
Closed

Avoid segfault if user fails to call setdata before a mech FUNCTION.#2460
nrnhines wants to merge 20 commits into
masterfrom
hines/setdata

Conversation

@nrnhines

@nrnhines nrnhines commented Aug 14, 2023

Copy link
Copy Markdown
Member

Prior to merging this PR, #2504 should be merged.

Closes #686

This PR solves a legacy issue having to do with interpreter calls to density mechanism FUNCTION and PROCEDURE which
make use of RANGE variables. In that case it was necessary to first call h.setdata_suffix(segment) in order to specify which instance of the mechanism range variable data was to be used by the subsequent function call. The problem is that it is understandably the case that the user forgets that h.setdatamust first be called, or that in the potentially extensive interval between the h.setdata call and the subsequent function call, the mechanism instance was destroyed by intervening statements. In either case, a segfault would be generated.

The SOA and Data Handle improvements make it possible to fix this issue, by generating a recoverable error message. This is implemented in the nocmodl translator with the introduction of

static _nrn_non_owning_id_without_container _prop_id{};

to shadow the legacy

static Prop* _extcall_prop;

When the function is called by the interpreter, if _prop_id is not valid, an error message is generated.

The implementation checks whether a FUNCTION or PROCEDURE in fact uses a RANGE variable and, if not, a preliminary call to h.setdata is not required.

POINT_PROCESS behavior of FUNCTION and PROCEDURE is ok since each function call already only uses sec(x).mech.func(args) syntax so that instance is determined at time of the call.

This is most important for HOC users. A subsequent PR (#2475) builds on this for Pyhon to allow something like

soma(0.5).hd.alpl(-65)

which automatically sets the data appropriately. For Python, setdata will never be needed.

@nrnhines nrnhines marked this pull request as draft August 14, 2023 11:36
Comment thread src/nmodl/nocpout.cpp Outdated
Comment thread src/nmodl/nocpout.cpp
Comment thread src/nmodl/nocpout.cpp Outdated
Comment thread src/nmodl/nocpout.cpp Outdated
@azure-pipelines

Copy link
Copy Markdown

✔️ fc67786 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Comment thread src/nmodl/nocpout.cpp Outdated
@azure-pipelines

Copy link
Copy Markdown

✔️ 510e072 -> Azure artifacts URL

@codecov

codecov Bot commented Aug 19, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2460 (08faeb8) into master (5868cdd) will decrease coverage by 0.65%.
The diff coverage is 100.00%.

❗ Current head 08faeb8 differs from pull request most recent head 7307381. Consider uploading reports for the commit 7307381 to get more accurate results

@@            Coverage Diff             @@
##           master    #2460      +/-   ##
==========================================
- Coverage   61.49%   60.84%   -0.65%     
==========================================
  Files         623      622       -1     
  Lines      119197   119003     -194     
==========================================
- Hits        73296    72412     -884     
- Misses      45901    46591     +690     
Files Coverage Δ
src/nmodl/nocpout.cpp 95.27% <100.00%> (+0.19%) ⬆️
src/nmodl/parsact.cpp 87.71% <100.00%> (+0.02%) ⬆️
src/nmodl/parse1.ypp 82.59% <100.00%> (+0.27%) ⬆️
src/nrnoc/membfunc.cpp 66.66% <100.00%> (+1.28%) ⬆️

... and 24 files with indirect coverage changes

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

@bbpbuildbot

This comment has been minimized.

@nrnhines nrnhines mentioned this pull request Aug 25, 2023
5 tasks
@azure-pipelines

Copy link
Copy Markdown

✔️ 6174b37 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@azure-pipelines

Copy link
Copy Markdown

✔️ a75659b -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@azure-pipelines

Copy link
Copy Markdown

✔️ 3adc3d4 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@azure-pipelines

Copy link
Copy Markdown

✔️ 600a4ab -> Azure artifacts URL

…ier_without_container

Allows checking of _extcall_prop validity by shadowing with an id.
@azure-pipelines

Copy link
Copy Markdown

✔️ d107912 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@nrnhines nrnhines requested a review from 1uc September 16, 2023 14:57
@bbpbuildbot

This comment has been minimized.

@azure-pipelines

Copy link
Copy Markdown

✔️ bab11c8 -> Azure artifacts URL

@nrnhines nrnhines requested a review from ohm314 October 2, 2023 13:05
@azure-pipelines

Copy link
Copy Markdown

✔️ b2c182b -> Azure artifacts URL

Comment thread src/nmodl/nocpout.cpp Outdated
@bbpbuildbot

This comment has been minimized.

@nrnhines

nrnhines commented Oct 2, 2023

Copy link
Copy Markdown
Member Author

I think I need some help with the Sonar complaints. The first one I looked at

static std::unordered_map<Symbol*, Info> funcs;
Global variables should be const.

puzzled me. What, precisely, is supposed to be const? The Symbol*? Is there a way to experiment with changes without continually pushing to the PR?

@1uc

1uc commented Oct 2, 2023

Copy link
Copy Markdown
Collaborator

You'll need to ignore that complaint in the context of NEURON. Modern C++ codes don't allow global variables, with only very few exceptions. I think they're making an exception of read-only global variables, which is why they talk about const.

@azure-pipelines

Copy link
Copy Markdown

✔️ 08faeb8 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@azure-pipelines

Copy link
Copy Markdown

✔️ 7307381 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@nrnhines nrnhines enabled auto-merge (squash) October 3, 2023 09:29
@nrnhines nrnhines disabled auto-merge October 3, 2023 09:30
@nrnhines nrnhines enabled auto-merge (squash) October 3, 2023 09:31
@nrnhines

nrnhines commented Oct 3, 2023

Copy link
Copy Markdown
Member Author

Any reasons remaining that this PR should not receive an approving review?

auto-merge was automatically disabled October 5, 2023 11:47

Merge queue setting changed

@sonarqubecloud

Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 12 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 12 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@pramodk

pramodk commented Oct 16, 2023

Copy link
Copy Markdown
Member

Any reasons remaining that this PR should not receive an approving review?

I guess we just didn't get time. Sorry for that.

Closing this as #2475 includes this already.

@pramodk pramodk closed this Oct 16, 2023
@pramodk

pramodk commented Oct 17, 2023

Copy link
Copy Markdown
Member

@nrnhines : a minor thing - should we delete this branch then?

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.

seg fault when calling mechanism function that depends on location without setdata

5 participants