UndefinedBehaviourSanitizer fixes #1479
Conversation
olupton
force-pushed
the
olupton/channel-benchmark
branch
from
c9a78d7
to
f8724fe
Compare
olupton
force-pushed
the
olupton/ubsan-fixes
branch
from
ebae4fe
to
5873be8
Compare
I hoped this would be readable-ish -- I chose to format each function I touched. Only formatting changed lines gave junk results (e.g. one line indented with spaces in the middle of a block indented with tabs. Obviously not running |
olupton
force-pushed
the
olupton/ubsan-fixes
branch
from
8d7c4aa
to
9391aba
Compare
|
nono - you're right.. it is readable, just takes a second. Also github could probably do a better job at highlighting actual changes (rather than indenting). |
|
9391abad740e7e7ef3f74b90215240869366d575 caused more problems than it solved; it fixed one bit of UB but created a segfault...and there is more UB being picked up by the sanitizer in related code. I'll revert that for now and see if there is any lower-hanging fruit to fix first. |
olupton
force-pushed
the
olupton/ubsan-fixes
branch
from
9391aba
to
7c628d0
Compare
olupton
force-pushed
the
olupton/ubsan-fixes
branch
from
7c628d0
to
49d6ada
Compare
Codecov Report
@@ Coverage Diff @@
## master #1479 +/- ##
==========================================
- Coverage 45.30% 45.30% -0.01%
==========================================
Files 551 551
Lines 111206 111212 +6
==========================================
+ Hits 50380 50381 +1
- Misses 60826 60831 +5
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
There was a problem hiding this comment.
Here are my quick comments:
- Summary : this PR adds new macro
carnage_columnindexto indicate the position of variable in array. This helps to avoid pointer arithmetic (esp when pointer is NULL). LGTM - With the current state of the PR, there is still inconsistency with the code being generated. There are quite some places where we are still generating the code in old style:
$ grep -r " - _p" ../src/nmodl/*
../src/nmodl/deriv.cpp: Sprintf(buf, "for(_i=0;_i<%d;_i++){_slist%d[%d+_i] = (%s + _i) - _p;"
../src/nmodl/deriv.cpp: Sprintf(buf, " _dlist%d[%d+_i] = (%s + _i) - _p;}\n"
../src/nmodl/deriv.cpp: Sprintf(buf, "_slist%d[%d] = &(%s) - _p;",
../src/nmodl/deriv.cpp: Sprintf(buf, " _dlist%d[%d] = &(%s) - _p;\n",
../src/nmodl/deriv.cpp:Sprintf(buf, "error=shoot(%d, &(%s) - _p, _pmatch_time, _pmatch_value, _state_match,\
../src/nmodl/deriv.cpp:Sprintf(buf, "for (_i=0; _i<%d; _i++) {_state_get[%d+_i] = (%s + _i) - _p;}\n",
../src/nmodl/deriv.cpp: Sprintf(buf, "_state_get[%d] = &(%s) - _p;\n", j, s->name);
../src/nmodl/deriv.cpp:Sprintf(buf, "for (_i=0; _i<%d; _i++) {_state_match[%d+_i] = (%s + _i) - _p;}\n",
../src/nmodl/deriv.cpp:Sprintf(buf, "_state_match[%d] = &(%s) - _p;\n", j, s->name);
../src/nmodl/kinetic.cpp: _RHS%d(_i) = -_dt1*(_p[_slist%d[_i]] - _p[_dlist%d[_i]]);\n\
../src/nmodl/kinetic.cpp: _RHS%d(_i) = -_dt1*(_p[_ix][_slist%d[_i]] - _p[_ix][_dlist%d[_i]]);\n\
../src/nmodl/kinetic.cpp: Sprintf(buf, "for(_i=0;_i<%d;_i++){_slist%d[%d+_i] = (%s + _i) - _p[_ix];"
../src/nmodl/kinetic.cpp: Sprintf(buf, " _dlist%d[%d+_i] = (D%s + _i) - _p[_ix];}\n"
../src/nmodl/kinetic.cpp: Sprintf(buf, "_slist%d[%d] = &(%s) - _p[_ix];",
../src/nmodl/kinetic.cpp: Sprintf(buf, " _dlist%d[%d] = &(D%s) - _p[_ix];\n",
../src/nmodl/sens.cpp:_slist%d[%d+_i] = (%s + _i) - _p; _dlist%d[%d+_i] = (%s + _i) - _p;}\n", SYM(q1)->araydim,
../src/nmodl/sens.cpp:Sprintf(buf, "_slist%d[%d] = &(%s) - _p; _dlist%d[%d] = &(%s) - _p;\n",
../src/nmodl/sens.cpp:_slist%d[%d+_i] = (%s + _i) - _p;}\n", SYM(q1)->araydim,
../src/nmodl/sens.cpp: Sprintf(buf, "_slist%d[%d] = &(%s) - _p;\n",
../src/nmodl/sens.cpp:_eplist%d[%d+_i] = (%s + _i) - _p; _emlist%d[%d+_i] = (%s + _i) - _p;}\n", SYM(q1)->araydim,
../src/nmodl/sens.cpp:Sprintf(buf, "_eplist%d[%d] = &(%s) - _p; _emlist%d[%d] = &(%s) - _p;\n",
../src/nmodl/simultan.cpp: Sprintf(buf, "for(_i=0;_i<%d;_i++){_slist%d[%d+_i] = (%s + _i) - _p;}\n"
../src/nmodl/simultan.cpp: Sprintf(buf, "_slist%d[%d] = &(%s) - _p;\n",
../src/nmodl/simultan.cpp: "for(_i=0;_i<%d;_i++){_slist%d[%d+_i] = (%s + _i) - _p;}\n",
../src/nmodl/simultan.cpp: // _slist1[0] = &(vi) - _p;- (driven by fear of macros & old nmodl/mod2c code) I am tagging @nrnhines to confirm if there are scenarios where such renaming/refactoring could interfere with the code.
Macros's are Evil. And more macros means more Evils! :)
olupton
force-pushed
the
olupton/ubsan-fixes
branch
from
97e27b9
to
7f4e67d
Compare
|
f7175cdfd247f8fb2283780ae5737525fbd74d9d fixes two more ubsan errors in the |
|
83b7a19b4db0dbc1c2ef884ec513f2d769495935 includes similar changes for all the other occurrences that my regexes found. Unlike the other commits, this one is not based on ubsan error messages or checking generated code. Let me know if you would prefer that this commit be dropped. The formatting is a bit of a pain. With hindsight I would not have tried to do this as I went, but rather just run |
|
I don't normally build with Interviews enabled, but as an aside there are two more errors there (which I have not fixed): |
olupton
force-pushed
the
olupton/ubsan-fixes
branch
2 times, most recently
from
727fd92
to
e893ab1
Compare
olupton
force-pushed
the
olupton/ubsan-fixes
branch
from
e893ab1
to
865aed4
Compare
|
Rebased on |
olupton
force-pushed
the
olupton/ubsan-fixes
branch
from
865aed4
to
2c16b0c
Compare
Sounds good. I would still like to get this merged and set up automated testing to flag regressions and the outstanding/known remaining issues. |
|
Overall, this change look fine to me. But there is one issue I found while testing cadecay.mod which has ion variable as a STATE and user has declared it as GLOBAL. If I am not mistaken, the original code generated is not 100% correct but it's not compilation error. I would like to ping @nrnhines to see what should be done here: Lets look at original code for above mentioned NEURON{
SUFFIX cad
USEION ca READ ica, cai WRITE cai
RANGE ica, channel_flow, depth, B
GLOBAL cai, tau, cainf
}
...
STATE {
cai (mM)
}
INITIAL {
cai = cainf
}Generated static double *_p;
static Datum *_ppvar;
#define t nrn_threads->_t
#define dt nrn_threads->_dt
#define depth _p[0]
#define ica _p[1]
#define channel_flow _p[2]
#define B _p[3]
#define Dcai _p[4]
#define _g _p[5]
#define _ion_ica *_ppvar[0]._pval
#define _ion_cai *_ppvar[1]._pval
#define _style_ca *((int*)_ppvar[2]._pvoid)
...
static void _setdata(Prop* _prop) {
_p = _prop->param;
_ppvar = _prop->dparam;
}
...
/* declare global and static user variables */
#define cainf cainf_cad
double cainf = 1e-05;
#define cai cai_cad
double cai = 0;
#define tau tau_cad
..
static void _initlists() {
int _i; static int _first = 1;
if (!_first) return;
_slist1[0] = &(cai) - _p;
_dlist1[0] = &(Dcai) - _p;
_first = 0;
}As Beside the above question, this PR now gives compilation error: -> Compiling cadecay.c
cadecay.c:452:15: error: use of undeclared identifier 'cai_columnindex'
_slist1[0] = cai_columnindex; _dlist1[0] = Dcai_columnindex;This is because we have following generated code: #define t nrn_threads->_t
#define dt nrn_threads->_dt
#define depth _p[0]
#define depth_columnindex 0
#define ica _p[1]
#define ica_columnindex 1
#define channel_flow _p[2]
#define channel_flow_columnindex 2
#define B _p[3]
#define B_columnindex 3
#define Dcai _p[4]
#define Dcai_columnindex 4
#define _g _p[5]
#define _g_columnindex 5
#define _ion_ica *_ppvar[0]._pval
#define _ion_cai *_ppvar[1]._pval
#define _style_ca *((int*)_ppvar[2]._pvoid)
/* declare global and static user variables */
#define cainf cainf_cad
double cainf = 1e-05;
#define cai cai_cad
double cai = 0;
#define tau tau_cad
static void _initlists() {
int _i; static int _first = 1;
if (!_first) return;
_slist1[0] = cai_columnindex;
_dlist1[0] = Dcai_columnindex;
_first = 0;
}As user has declared |
|
I believe GLOBAL STATE should be an error. I can't imagine a case where it makes sense. |
Thanks for confirming Michael. I think we can error on such MOD files as they result in wrong code. I will check this. After the above review I triggered nrn-modeldb-ci with the wheel generated from this PR against nrn-nightly i.e. master: https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/2008191663 The CI report has following summary:
So bit more work required to verify the issues with the above models. |
|
The 10 models that no longer compile in the diff above all seem to be because of the same issue with I am also looking at the other models that apparently have different results. So far all the cases I have looked at have appeared to be false positives due to the inclusion of time or date information in stdout/stderr. neuronsimulator/nrn-modeldb-ci#18 may help with this. See also: #1716 |
For some pointer `p` then `&p[14] - p` is undefined behaviour if `p` is null. This changes code generation to output `14` instead in that case.
olupton
force-pushed
the
olupton/ubsan-fixes
branch
from
6b2b7fc
to
ed24254
Compare
|
I am still getting to grips with the ModelDB CI, but I have still not seen any substantive differences caused by this PR. I would be happy to move ahead with it. |
* state variables can not be global, see #1479 (comment) * before code generation iterate through all symbols and error if there is any state variable declared as global.
|
I ran locally using the wheel from this PR's CI and yesterday's neuron-nightly wheel. My summary of the differences that are flagged by the comparison script in
+0.00082 kilocycles
-0.00075 kilocycles
+in nc_append11 0 0 0.005 3 10
+0 0
+0 0
-in nc_append11 1 1 0.005 3 10
-1 1
-1 0
I will look into the last two a little more. The |
|
Both 51781 and 138379 seem to produce inconsistent output. I ran each one four times, twice with this PR and twice without it. In both cases there are diffs between [PR#1, PR#2] and [reference#1, reference#2], so this does not seem to be a source of concern for this PR. |
|
neuronsimulator/nrn-modeldb-ci#20 was included in the comparison. |
|
I did the same exercise for 97756 and 116838. The tiny numerical differences in 97756 seem random and sometimes appear in repeated runs of the reference wheel. The diff in 116838 seems to be more consistent and to only appear between the reference and PR wheels. +TMAX: 9.9996359
-TMAX: 9.9999991and in gout -3629
+3621 |
|
I went through MOD files & generated |
|
I think it's fine without a rebase. The only files that would be updated are |
* state variables can not be global, see #1479 (comment) * before code generation iterate through all symbols and error if there is any state variable declared as global.
|
Somehow Github status is stuck but CIs are already green. So I will push the button! @nrnhines : Just FYI, we have created |
This PR contains fixes for issues uncovered by the UndefinedBehaviour sanitizer: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html. BlueBrain/CoreNeuron#652 contains similar fixes for CoreNeuron.
cabpump.c, vclmp.c: applying non-zero offset to null pointer
Code like:
was generated in the
_initlistsmethod, which triggers undefined behaviour if_pisnullptr:"Fixed" by changing the code generation to define
#define vi_columnindex 17and emit_slist1[0] = vi_columnindex;instead.Others
There are still several errors in
src/ivoc/ocobserv.h,src/nrncvode/netcvode.cppandsrc/nrniv/netpar.cpp. These are described in #1483 and should be fixed separately. #1518 also has some more fixes.