[RFC] Fix clang analysis warnings. (1) #1389

elmart · 2014-11-04T14:40:08Z

This is the first of a series of PR's aimed at taking the number of clang analysis warnings down to zero.

IMPORTANT: Commits contain links to online clang report entries, to ease review. I'm not sure if clang report generated links are deterministic. It that's not the case, then links would become stale on next report. So, if this can happen, please review this before merging other things into master (which would trigger a new clang report). UPDATE: Links are broken and won't be fixed. See below.

In principle, one warning is resolved in one commit (to match granularity of clang report and ease identification). But, when possible, warnings sharing a common cause/fix and belonging to the same function are grouped.

Following classification is used to describe warnings, after they have been resolved.

Code	Type	Meaning
CW	Cautionary warning	Signaled circumstance is not necessarily an issue, but for some reason it's considered better to avoid it.
FP	False positive	Error condition cannot really occur, because of things we know, but the compiler doesn't. These are usually solved just asserting what the compiler needs to know.
MI	Multithreading issue	Error condition can be a real issue, but only under multithreading conditions. These are usually caused by use of global state and are usually solved by making local copies instead.
HI	Harmless issue	Error condition is in fact an issue, but a harmless one. These are usually solved removing/editing whatever is causing the warning.
RI	Real issue	Error condition can in fact occur under normal circumstances and has some undesirable effects.�

Code is added as last part of commit messages to allow for a quick glimpse on diagnosed warning types without having to read full commit message.

This PR gets rid of 23 warnings in:

curshor_shape.c
screen.c
ops.c
regexp.c
edit.c

tarruda · 2014-11-04T14:42:28Z

Great stuff!

fwalch · 2014-11-04T14:48:17Z

This is the first of a series of PR's aimed at taking the number of clang analysis warnings down to zero.

I'm not sure this is ~~possible~~ always the best solution (e.g. #985 (comment)), but big 👍 for working towards this goal!

IMPORTANT: Commits contain links to online clang report entries, to ease review. I'm not sure if clang report generated links are deterministic.

Unfortunately, they are not. Maybe we can come up with something in bot-ci at some point (it's on the to do list).

elmart · 2014-11-04T14:55:12Z

Thx. This first PR has been faster to do than I expected, in fact. If I continue at the same rate and don't find showstoppers, I think I can have all warnings solved in 3 or 4 days. I prefer making several separated PR's, though, not to make PR's too heavy to review, and because of the stale links problem.

elmart · 2014-11-04T15:04:01Z

Aaaarg!!

Links are, in fact, already broken (pity, they were ok 15 min ago).
I think links really make review much more attractive/quick, as you really need the clan report entry to follow what's happening, and having to manually search for entry in clang report slows it down / makes it unattractive.

Therefore, I'm ok to fix links and repush, but I would need some guarantees that no other merges will occur in between, destroying links again. To all commiters @tarruda @justinmk @jszakmeister Can I count on that?

justinmk · 2014-11-04T15:16:02Z

Don't fight the links, it's a lost cause. Should be easy enough for reviewers to relate the associated files on http://neovim.org/doc/reports/clang/

And thanks so much for working on this!

elmart · 2014-11-04T15:24:24Z

@justinmk Ok. For warning identification in clang report:

Full commit messages include a section like this:

Problem: Assigned value is garbage or undefined @ 187.
         http://neovim.org/doc/reports/clang/report-7b7d61.html#EndPath.

containing <full warning message as it appeared in clang report> @ <line_number>.

Once you have the clang report in front of you, click line number column once to order by line number ascending, and then click file column once to order by file name ascending.
Then, the order of clang report entries should match commit order. This is, I had clang report ordered that way and proceeded one by one, in order, in the involved files.

tarruda · 2014-11-04T16:02:11Z

Therefore, I'm ok to fix links and repush, but I would need some guarantees that no other merges will occur in between, destroying links again. To all commiters @tarruda @justinmk @jszakmeister Can I count on that?

I won't be merging anything else today

Problem: Assigned value is garbage or undefined @ 187. http://neovim.org/doc/reports/clang/report-7b7d61.html#EndPath. Diagnostic: False positive. Rationale : `colonp`, must be `>= modep, or null` by `vim_strchr` postcondition. At this point we also it's not null and it's not equal to `modep`, by previous code. So, it must be `> modep`. Resolution: Assert `colonp > modep`.

Problems: Argument with 'nonnull' attribute passed null @ 277. http://neovim.org/doc/reports/clang/report-9c3614.html#EndPath Result of operation is garbage or undefined @ 281. http://neovim.org/doc/reports/clang/report-45efbf.html#EndPath Argument with 'nonnull' attribute passed null @ 306. http://neovim.org/doc/reports/clang/report-ffb84f.html#EndPath Result of operation is garbage or undefined @ 311. http://neovim.org/doc/reports/clang/report-d04333.html#EndPath Argument with 'nonnull' attribute passed null @ 315. http://neovim.org/doc/reports/clang/report-786819.html#EndPath Uninitialized argument value @ 328. http://neovim.org/doc/reports/clang/report-2a5506.html#EndPath Diagnostic: Multithreading issues. Rationale : All reported problems can only occur if accesed globals change state while executing function, which could only happen in a multithreaded environment. Resolution: Use local variables. Note that this change alters function semantics, as now function only depends on global values at entry time. This shouldn't be a problem, though, as new semantics should be in fact better.

Problem: Dead initialization @ 3477. http://neovim.org/doc/reports/clang/report-94b736.html#EndPath Diagnostic: Harmless issue. Rationale : `len` is assigned a new value just some lines below. So, this just seems something due to old-style variable declarations. Resolution: We could just remove initialization, but prefer moving declaration down to point of initialization.

Problem: Dead assigment. http://neovim.org/doc/reports/clang/report-7362ba.html#EndPath Diagnostic: Harmless issue. Rationale : `boguscols` is in fact unread by downstream code. Resolution: Comment out. This is preferred here over just removing the line because involved logic is complex, and future readers of this code could find this extra knowledge useful to understand what the code is doing.

Problem: Dead assignment @ 7535. http://neovim.org/doc/reports/clang/report-19a5cd.html#EndPath Diagnostic: Harmless issue. Rationale : `length = msg_col;` is unconditionally executed after this. Resolution: Remove assignment.

Problem: Dead assignment @ 7711. http://neovim.org/doc/reports/clang/report-835eb6.html#EndPath Diagnostic: Harmless issue. Rationale : `scol` is only used within `FOR_ALL_TABS` body, which assigns another value to `scol` at the beginning of each iteration. If `FOR_ALL_TABS` body would not execute (no tabs) nothing harmful would happen, as code following `FOR_ALL_TABS` doesn't use `scol`. Resolution: Remove.

Problem: Argument with 'nonnull' attribute passed null @ 3540. http://neovim.org/doc/reports/clang/report-fc14e0.html#EndPath. Diagnostic: False potitive. Rationale : `count` should be >= 2 by function precondition. Resolution: Assert precondition.

Problems: Result of operation is garbage or undefined @ 5087. http://neovim.org/doc/reports/clang/report-2e3118.html#EndPath Result of operation is garbage or undefined @ 5149. Diagnostic: Multithreading issues. Rationale : All reported problems can only occur if accesed globals change state while executing function, which could only happen in a multithreaded environment. Resolution: Use local variables (copy globals on entry). Note that this change alters function semantics, as now function only depends on global values at entry time. This shouldn't be a problem, though, as new semantics should be in fact better.

Problem: Derefence of null pointer @ 1208. http://neovim.org/doc/reports/clang/report-24b5ca.html#Path10 Diagnostic: False positive. Rationale : Error is reported to happen if after `if (*newp == NULL) {` body, `*newp` continues being NULL, and false branch of following `if (*newp != NULL)` is taken. Now, `vim_strsave` cannot return NULL, so error cannot happen. Resolution: Remove dead code (leftover since OOM refactors).

Problem: Dereference of null pointer @ 1312. http://neovim.org/doc/reports/clang/report-b1d09a.html#EndPath Diagnostic: Multithreading issue. Rationale : Suggested error path contains two succesive calls to `regnext(scan)`, first of which returning nonnull, the second one returning null. This can only occur if global `reg_toolong` accesed in `regnext()` changes between the calls. Resolution: Use local variable to cache first `regnext(scan)` result. Note that this change alters function semantics, as now function only issues one call instead of two, reusing the result for the second time. This shouldn't be a problem, though, as new semantics should be in fact be better.

Problem: Argument with 'nonnull' attribute passed null @ 5632. http://neovim.org/doc/reports/clang/report-041a0e.html#EndPath. Diagnostic: False positive. Rationale : `p = reg_getline(clnum)` above should not be null, because `clnum` starts at `start_lnum` and only increments after that. Resolution: Assert p not null.

Problem : Dereference of null pointer @ 3234. Diagnostic : False positive. Rationale : `wp` is local static, so maintains value between calls. First time function is called for a given flag will have `buf == curbuf`, implying `wp` initialization. Resolution : Assert variable always having been initialized.

Problems : Dereference of null pointer @ 3615. Dereference of null pointer @ 3764. Diagnostic : False positives. Rationale : `ins_buf` is local static, so maintains value between calls. This function will be called first when `compl_started` is false, and in that case it initializes `ins_buf`. After that, it can be called multiple times with `compl_started` true, where `ins_buf` will be updated but not to null. So, when arriving to both points, `ins_buf` should never be null. Resolution : Assert `ins_buf` at both points.

Problem : Uninitialized argument value @ 6296. Diagnostic : False positive. Rationale : Error occurs if n <= 1. That's not possible because n >= 1 due to `MB_BYTE2LEN` postcondition and n != 1 because we are in the else branch. Resolution : Assert n > 1.

Problem : Assigned value is garbage or undefined @ 6359. Diagnostic : Multithreading issue. Rationale : Problem only occurs if global `State` changes while function is executing. Resolution : Use local copy of global in function.

Problem : Result of operation is garbage or undefined @ 7460. Diagnostic : Multithreading issue. Rationale : Problem occurs if any of globals `enc_utf8`, `p_deco is modified while function is executing. Resolution : Use local copy of globals.

elmart · 2014-11-06T09:10:03Z

Added some more commits and updated fixed warnings count to 23.
I won't be adding more to this PR, not to do it too heavy to review.

elmart · 2014-11-06T09:46:03Z

@fwalch I take your considerations into account. For the time being, though, all warnings examined had a (IMO) satisfactory solution.

Problems : Assigned value is garbage or undefined @ 127. Assigned value is garbage or undefined @ 152. Diagnostic : Multithreading issues. Rationale : Error could only occurr if global `enc_utf8` changed while the function is executing. Resolution : Use local copy of global var.

fwalch · 2014-11-08T16:37:05Z

src/nvim/edit.c

@@ -6286,6 +6290,7 @@ static void mb_replace_pop_ins(int cc)
        break;
      } else {
        buf[0] = c;
+        assert(n > 1);
        for (i = 1; i < n; ++i)


While you're at it, maybe you can move the definition of i into this for?

Answered here, because relevant to future reviewers too.
I hope you agree. ;-)

elmart · 2014-11-08T17:41:45Z

To reviewers:

Please consider that this series of PR's will touch a lot of different files, in many parts of the code that for the most part haven't been touched.

So, I can't pretend to refactor every improvable thing in any line touched.
That would make this task never-ending, and would make review really hard, because we would be mixing a lot of different tasks.

Here I try to stay focused on this task's goal: visiting every warning, checking whether it signals a real problem or not, and making the most sensible but minimum changes to fix the problem and remove the warning.

For sure, at many places, there will be many other improvements that could have been done, but all of those places should be sooner or later visited by a PR with a refactoring intent.
As said above, that's not our goal here.

So, please report defects in diagnostic/rationale/resolution or any other thing I may have overlooked, but don't report about other possible refactorings not having to do with fixing warnings.

I hope you all agree. Thanks ;-)

aktau · 2014-11-09T11:47:55Z

Referring to: elmart@a32442d#diff-652ba8f711669fd3aa981ba690a56f43R243

Resolution: Use local variables.
Note that this change alters function semantics, as now
function only depends on global values at entry time.
This shouldn't be a problem, though, as new semantics should
be in fact better.

You know, this is in fact mostly equivalent to passing those globals as parameters. If there aren't a whole lot of callsites, perhaps we can consider that? What do the others think?

aktau · 2014-11-09T12:45:31Z

LGTM.

justinmk · 2014-11-09T16:28:35Z

Running bot-ci to get a baseline, then will merge and re-run.

Fix clang analysis warnings.

fwalch · 2014-11-09T18:33:10Z

Running bot-ci to get a baseline, then will merge and re-run.

Would you mind trying neovim/bot-ci#24 to restart the build?

justinmk · 2014-11-09T18:36:35Z

@fwalch I can try later, didn't see your comment.

elmart · 2014-11-09T20:19:04Z

Hi guys. I'm sorry but today I couldn't get in front of my computer until now.

@aktau

You know, this is in fact mostly equivalent to passing those globals as parameters. If there aren't a whole lot of callsites, perhaps we can consider that? What do the others think?

Yes. I would say they're completely equivalent if locals are defined just on function entry.
I did in fact consider taking that solution, but in the end, I decided not to do it, because of two reasons:

Stay focused. You know, refactorings tend to cascade, and though this refactoring was an easy one, I preferred to remain local, and not got out changing other files.
Much more important than the one before:

If you start adding parameters everywhere you need a global, effects quickly compose, and soon you end up with long lists of parameters. Taking that to the extreme to remove all global state can result in horrible code.

So, I do think we really have to, at some point, tackle the problem of refactoring all current global state into a more manageable thing. But we do have to think carefully how to do that, in order to end up with really better-quality code.

For example, many variables that are now at the same level could probably be encapsulated in different "objects", so that passing parameters around can be done in a sane way. It's very different passing a user options object than passing a parameter for every single option you' d like to access.

So, seeing that solving the problem well was more complicated than just adding some parameters, I decided to remain with the problem at hand (solving warnings) and stay local in my solution, leaving proper global state refactoring as a separated task to be tackled in future.

Hope you agree.

@justinmk Thanks for merging. I can't wait to see that badge go down!
@fwalch Let's do that bot work a little!

UPDATE: I will have batch 2 ready in one or two days.

elmart · 2014-11-09T20:45:39Z

@justinmk Did you make the bot run? Badge is still at 70...

aktau · 2014-11-09T21:31:28Z

Yes. I would say they're completely equivalent if locals are defined just on function entry.
I did in fact consider taking that solution, but in the end, I decided not to do it, because of two reasons:

You're right. On one hand, I love it when the amount of knowledge needed for a function is clearly delineated by its parameters. On the other hand, for some functions this might get messy real quick (e.g.: uses more than 4 globals), which would be better served with a big state object (a Vim or vim_t struct). Let's leave it for later indeed.

fwalch · 2014-11-09T21:50:22Z

@justinmk Did you make the bot run? Badge is still at 70...

He did, but the build timed out trying to install the clang Ubuntu packages because llvm.org seems to be down.

elmart · 2014-11-09T22:00:07Z

@fwalch

Ok. It seems luck is not with us today. Never mind. As somebody said, we shall overcome. ;-)
BTW, when we can finally see the badge go down on clang warnings, I expect it to improve coverity score too. Let's see.

justinmk · 2014-11-09T23:45:40Z

On the other hand, for some functions this might get messy real quick (e.g.: uses more than 4 globals), which would be better served with a big state object (a Vim or vim_t struct). Let's leave it for later indeed.

I agree with this, and I wonder if it would be a bad idea to introduce a "stub" object that we can start passing around that slowly accretes globals. E.g., it may start out with only p_mco, and all call sites for p_mco would then require that object instead. This way we could migrate functions in small increments, and eventually all functions would have this "world object". During the transition, the "world object" would itself be a (static/private) global, only it happens to also be passed as a function argument.

@elmart I also think prepending l_ is a good pattern (e.g. for the case in elmart@a32442d). We should keep the variable names recognizable with the legacy Vim names to help with merging Vim patches.

justinmk · 2014-11-10T15:19:17Z

@elmart If I read https://s3.amazonaws.com/archive.travis-ci.org/jobs/40496130/log.txt correctly, we're down to 47 clang warnings now! The badge hasn't updated yet, seems to be aggressively cached....

aktau · 2014-11-10T15:21:09Z

The page the badge links to also lists 47, so I think it's safe to assume that's correct. Pretty awesome!

elmart · 2014-11-10T15:39:27Z

Great! In my previous comments I was stating that this PR contained fixes for 24 warnings. So, when you said 23 warnings had been eliminated, I thought one of my fixes should have failed (strange, cause I have checked them locally before pushing, but who knows, maybe new code changed something). However, I have now counted them again, and they are in fact 23 (I had made a mistake in my previous count). So, 47 was exactly what expected (47 == 70 - 23). Pity that we're having such bad luck with llvm.org. What a weird thing...

On the other hand, what I find a bit puzzling is that coverity score has not only not diminished, but incremented by 1. I thought there would be some things catched by both tools, so coverity score would also improve. Don't know. Let 's wait to see what happens with next PR's.

BTW, I will have batch 2 ready to review in a couple of hours. I estimate there will be 4 PR's in total to take the warning count down to zero, which I now consider perfectly doable.

Editing previous comments to correct number of fixes included to 23.

elmart · 2014-11-10T15:46:40Z

The page the badge links to also lists 47

True! I hadn't realized it.

jszakmeister · 2014-11-10T17:41:50Z

Pity that we're having such bad luck with llvm.org. What a weird thing...

In case anyone is curious, the issue was reported a couple of days ago, and Chandler Carruth responded with this today:

Just FYI,

Folks are aware and there is nothing we can do. There appears to be a network issue in the building where the server is currently located. As soon as it is resolved thing should come back up, but no word yet on when to expect that.

It looks like at least some part of it is back up now though.

elmart force-pushed the clang-analysis-fixes branch from 03050ef to 05c9c7f Compare November 4, 2014 14:42

justinmk added the robustness label Nov 4, 2014

elmart force-pushed the clang-analysis-fixes branch from 05c9c7f to bb20631 Compare November 4, 2014 16:27

elmart added 16 commits November 6, 2014 09:51

Fix warnings: screen.c: showmode(): Dead assignment: HI.

cc996e7

Problem: Dead assignment @ 7535. http://neovim.org/doc/reports/clang/report-19a5cd.html#EndPath Diagnostic: Harmless issue. Rationale : `length = msg_col;` is unconditionally executed after this. Resolution: Remove assignment.

elmart force-pushed the clang-analysis-fixes branch from bb20631 to 74817b5 Compare November 6, 2014 08:56

elmart mentioned this pull request Nov 7, 2014

[RDY] Fix clang analysis warnings. (2) #1431

Merged

fwalch reviewed Nov 8, 2014
View reviewed changes

justinmk added a commit that referenced this pull request Nov 9, 2014

Merge pull request #1389 from elmart/clang-analysis-fixes

e9ac693

Fix clang analysis warnings.

justinmk merged commit e9ac693 into neovim:master Nov 9, 2014

This was referenced Nov 12, 2014

[RDY] Fix clang analysis warnings. (3) #1460

Merged

[RFC] Fix clang analysis warnings. (4) #1478

Merged

elmart deleted the clang-analysis-fixes branch November 15, 2014 16:51

elmart mentioned this pull request Jan 26, 2015

[RFC] Remove global variable screen_char_attr in screen.c #1889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Fix clang analysis warnings. (1) #1389

[RFC] Fix clang analysis warnings. (1) #1389

elmart commented Nov 4, 2014

tarruda commented Nov 4, 2014

fwalch commented Nov 4, 2014

elmart commented Nov 4, 2014

elmart commented Nov 4, 2014

justinmk commented Nov 4, 2014

elmart commented Nov 4, 2014

tarruda commented Nov 4, 2014

elmart commented Nov 6, 2014

elmart commented Nov 6, 2014

fwalch Nov 8, 2014

elmart Nov 8, 2014

elmart commented Nov 8, 2014

aktau commented Nov 9, 2014

aktau commented Nov 9, 2014

justinmk commented Nov 9, 2014

fwalch commented Nov 9, 2014

justinmk commented Nov 9, 2014

elmart commented Nov 9, 2014

elmart commented Nov 9, 2014

aktau commented Nov 9, 2014

fwalch commented Nov 9, 2014

elmart commented Nov 9, 2014

justinmk commented Nov 9, 2014

justinmk commented Nov 10, 2014

aktau commented Nov 10, 2014

elmart commented Nov 10, 2014

elmart commented Nov 10, 2014

jszakmeister commented Nov 10, 2014

[RFC] Fix clang analysis warnings. (1) #1389

[RFC] Fix clang analysis warnings. (1) #1389

Conversation

elmart commented Nov 4, 2014

tarruda commented Nov 4, 2014

fwalch commented Nov 4, 2014

elmart commented Nov 4, 2014

elmart commented Nov 4, 2014

justinmk commented Nov 4, 2014

elmart commented Nov 4, 2014

tarruda commented Nov 4, 2014

elmart commented Nov 6, 2014

elmart commented Nov 6, 2014

fwalch Nov 8, 2014

Choose a reason for hiding this comment

elmart Nov 8, 2014

Choose a reason for hiding this comment

elmart commented Nov 8, 2014

To reviewers:

aktau commented Nov 9, 2014

aktau commented Nov 9, 2014

justinmk commented Nov 9, 2014

fwalch commented Nov 9, 2014

justinmk commented Nov 9, 2014

elmart commented Nov 9, 2014

elmart commented Nov 9, 2014

aktau commented Nov 9, 2014

fwalch commented Nov 9, 2014

elmart commented Nov 9, 2014

justinmk commented Nov 9, 2014

justinmk commented Nov 10, 2014

aktau commented Nov 10, 2014

elmart commented Nov 10, 2014

elmart commented Nov 10, 2014

jszakmeister commented Nov 10, 2014