New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RDY] Clang analyzer: fix dead stores / reduce scope #985

Merged
merged 7 commits into from Aug 8, 2014

Conversation

Projects
None yet
6 participants
@fwalch
Copy link
Member

fwalch commented Jul 23, 2014

Remove dead assignments as pointed out by static analysis. Here is a static analysis report that includes the changes of this PR.

I didn't fix the dead assignment in garray.c, because otherwise the build would fail with error: ignoring return value of ‘xstpcpy’, declared with attribute warn_unused_result, and I didn't know what to do about that.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jul 23, 2014

I didn't fix the dead assignment in garray.c, because otherwise the build would fail with error: ignoring return value of ‘xstpcpy’, declared with attribute warn_unused_result, and I didn't know what to do about that.

I guess the last (x)stpcpy could best be replaced with a bog-standard strcpy. That's my fault, though. I hadn't realized at that time that clang would flag that as a dead store.

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Jul 23, 2014

@aktau: Thanks! Added a commit that does that. In ~15min, the above static analysis report should be updated.

Edit: All dead store warnings are now gone.

@@ -1374,9 +1374,6 @@ int arabic_shape(int c, int *ccp, int *c1p, int prev_c, int prev_c1,
/* half-shape current and previous character */
shape_c = half_shape(prev_c);

/* Save away current character */
curr_c = c;

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

It's only a dead store because all of the following if branches will set curr_c. What if the code below is changed? We may end up with an uninitialized curr_c. What if declare and initialize curr_c here (int curr_c = c;)? It's a good practice to initialize variables. Specially before a set of branches that write to it.

This comment has been minimized.

@jszakmeister

jszakmeister Jul 23, 2014

Member

What if the code below is changed?

Then wouldn't the compiler presumably through a warning saying that we're using an uninitialized variable? I agree that moving the declaration to here would be a good idea though.

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

Then wouldn't the compiler presumably through a warning saying that we're using an uninitialized variable?

Yes, but warnings are usually ignored, however warnings in Neovim are errors (-Werror) and then your argument is valid.

@@ -3585,7 +3584,6 @@ int vim_vsnprintf(char *str, size_t str_m, char *fmt, va_list ap, typval_T *tvs)
* zero value is formatted with an
* explicit precision of zero */
precision = num_of_digits + 1;
precision_specified = 1;

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

This is another dead store that I don't think should be removed.

@@ -3321,7 +3321,6 @@ int vim_vsnprintf(char *str, size_t str_m, char *fmt, va_list ap, typval_T *tvs)
case 'c':
case 's':
case 'S':
length_modifier = '\0';

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

This is also a "let's be safe and reinitialize it" store.

@@ -2591,7 +2591,6 @@ do_set (
/* find end of name */
key = 0;
if (*arg == '<') {
nextchar = 0;

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

The code in this function is really complicated. Without this initialization we may easily introduce a bug once we try to refactor it.

@@ -1546,7 +1546,6 @@ static int nfa_regatom(void)
/* Try equivalence class [=a=] and the like */
if (equiclass != 0) {
nfa_emit_equi_class(equiclass);
result = OK;

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

Another "just in case" assignment that is good to have.

This comment has been minimized.

@aktau

aktau Jul 23, 2014

Member

Can we signal to the analyzer to ignore things like this (e.g.: like NOLINT)?

This comment has been minimized.

@fwalch

fwalch Jul 23, 2014

Member

Hm, seems like I didn't give all of this enough thought. However, if one of the removed stores results in an access to an uninitialized variable, the static analyzer would catch this, right?

Is there a way to suppress the static analysis warnings? We'd need to keep track of "real" warnings and "won't fix" warnings somehow.

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

@fwalch the compiler will warn you if it's NOT initialized. If it's wrongly initialized we got a bug.

@@ -3758,7 +3758,6 @@ win_line (
/* Get rid of the boguscols now, we want to draw until the right
* edge for 'cursorcolumn'. */
col -= boguscols;
boguscols = 0;

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

The win_line function starts in line 2177 and ends in line 4139. We should definitely refactor it. Removing the dead store makes it even more fragile.

@@ -7364,7 +7363,6 @@ int showmode(void)
msg_puts_attr(edit_submode_extra, sub_attr);
}
}
length = 0;

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

Same here.

@@ -7738,7 +7735,7 @@ static void win_redr_ruler(win_T *wp, int always)
int o;
int this_ru_col;
int off = 0;
int width = Columns;
int width;

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

You can declare and initialize width close to the if that sets it (line ~7799).

  validate_virtcol_win(wp);
  if (       redraw_cmdline
             || always
             || wp->w_cursor.lnum != wp->w_ru_cursor.lnum
             || wp->w_cursor.col != wp->w_ru_cursor.col
             || wp->w_virtcol != wp->w_ru_virtcol
             || wp->w_cursor.coladd != wp->w_ru_cursor.coladd
             || wp->w_topline != wp->w_ru_topline
             || wp->w_buffer->b_ml.ml_line_count != wp->w_ru_line_count
             || wp->w_topfill != wp->w_ru_topfill
             || empty_line != wp->w_ru_empty) {
    cursor_off();
// HERE
    int width;
    if (wp->w_status_height) {
      row = wp->w_winrow + wp->w_height;
      fillchar = fillchar_status(&attr, wp == curwin);
      off = wp->w_wincol;
      width = wp->w_width;
    } else {
      row = Rows - 1;
      fillchar = ' ';
      attr = 0;
      width = Columns;
      off = 0;
    }
@@ -3784,7 +3784,6 @@ current_search (
orig_pos = curwin->w_cursor;

pos = curwin->w_cursor;
start_pos = VIsual;

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

The else branch of this if has orig_pos = pos = start_pos = curwin->w_cursor;. You can actually remove start_pos from that line too [1]. Then, on line 3848 you can replace start_pos = pos; with pos_T start_pos = pos;.

[1] It wasn't considered a dead store because the orig_pos = pos = start_pos = curwin->w_cursor line sets and reads it, however we can see that start_pos is unnecessary there.

@@ -1428,7 +1428,6 @@ static void find_word(matchinf_T *mip, int mode)
// prefix ID.
// Repeat this if there are more flags/region alternatives until there
// is a match.
res = SP_BAD;

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

You can move the declaration and initialization to the place where res is being used (line 1633).

      // HERE INIT res
      int res = SP_BAD;
      if (flags & WF_BANNED)
        res = SP_BANNED;
      else if (flags & WF_REGION) {
        // Check region.
        if ((mip->mi_lp->lp_region & (flags >> 16)) != 0)
          res = SP_OK;
        else
          res = SP_LOCAL;
      } else if (flags & WF_RARE)
        res = SP_RARE;
      else
        res = SP_OK;
@@ -1606,7 +1605,6 @@ static void find_word(matchinf_T *mip, int mode)
mip->mi_compoff = (int)(p - mip->mi_fword);
}
}
c = mip->mi_compoff;

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

Another place where removing the dead store may be bad.

@@ -3780,8 +3777,7 @@ char_u *did_set_spelllang(win_T *wp)
filename = false;
if (len > 3 && lang[len - 3] == '_') {
region = lang + len - 2;
len -= 3;
lang[len] = NUL;
lang[len - 3] = NUL;

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

At first impression, I thought this would better stay, but after checking the code I see that it doesn't make sense to update len. Updating it may actually be more confusing.

@@ -5723,7 +5723,6 @@ static void syntime_report(void)
" TOTAL COUNT MATCH SLOWEST AVERAGE NAME PATTERN"));
MSG_PUTS("\n");
for (int idx = 0; idx < ga.ga_len && !got_int; ++idx) {
spp = &(SYN_ITEMS(curwin->w_s)[idx]);

This comment has been minimized.

@philix

philix Jul 23, 2014

Member

Good call here. Now you can reduce the scope of spp by declaring it inside the last for that actually uses it.

@@ -1566,7 +1565,6 @@ win_equal_rec (
}

for (fr = topfr->fr_child; fr != NULL; fr = fr->fr_next) {
n = m = 0;

This comment has been minimized.

@philix
@philix

This comment has been minimized.

Copy link
Member

philix commented Jul 23, 2014

I just learned: dead store warnings are very helpful when you want to reduce the scope of variables. And reducing the scope of variable is a safe way to remove dead stores.

Dead stores have been discussed before [1]. I'm glad you actually went through them all finding many cases that were worth fixing.

[1] #167 (comment)

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jul 25, 2014

I just learned: dead store warnings are very helpful when you want to reduce the scope of variables. And reducing the scope of variable is a safe way to remove dead stores.

This is indeed a good observation. Scope reduction is one of my pastimes when I'm already in an area of neovim code. Clang can help us see some egregious cases (dead stores). Big thumbs up for this commit (when the issues have been ironed out).

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Jul 29, 2014

I for one learned that I shouldn't mindlessly modify code just to lower some warnings counter. Thanks, @philix, for looking at all the warnings in such detail.

Unfortunately, there doesn't seem to be a good way to suppress specific clang analysis warnings.

I updated the commits with @philix's comments and additionally reduced the scope of some more variables. I noticed that some comments are in deprecated /* */ style, but I didn't touch them, as I didn't want to add too much noise to this PR.. or should I change this in functions I am working on anyway?

I hope that I didn't change too much and this will build; on my machine, I'm stuck in make deps somehow:

[ 91%] Generating /home/florian/Projects/neovim/.deps/usr/bin/moon, /home/florian/Projects/neovim/.deps/usr/bin/moonc

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jul 29, 2014

I for one learned that I shouldn't mindlessly modify code just to lower some warnings counter. Thanks, @philix, for looking at all the warnings in such detail.

This is something that's often overlooked: when coverity and clang report something, it's usually easy to make it go away, but it can be quite difficult to make it go away in a clean way that won't come back and actually reduces the lines of code required.

I hope that I didn't change too much and this will build; on my machine, I'm stuck in make deps somehow:

That's weird, I thought the "generating" step was only for automatic header generation?

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Jul 29, 2014

I hope that I didn't change too much and this will build; on my machine, I'm stuck in make deps somehow:

That's weird, I thought the "generating" step was only for automatic header generation?

The problem was that I did make distclean and some dependency could not be downloaded; I now use neovim/deps instead of building myself.

The build didn't pass, so I got too enthusiastic with the scope changes.. I'll fix my commits.

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Jul 29, 2014

Build passes now.. I thought that start_pos could be eliminated in search.c, but overlooked that pos might be modified on line 3854.

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Jul 30, 2014

Updated once more to move more variable declarations closer to their usage in search.c.

@justinmk justinmk referenced this pull request Jul 31, 2014

Merged

[RFC] August Newsletter #58

0 of 4 tasks complete

@fwalch fwalch changed the title [RFC] Clang analyzer: fix dead stores [RFC] Clang analyzer: fix dead stores / reduce scope Aug 1, 2014

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 7, 2014

Rebased from master.

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 7, 2014

Except small suggestions LGTM. 👍 for the actual using of clang analysis :-)

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 7, 2014

Incorporated your comments, thanks @Hinidu!

@fwalch fwalch changed the title [RFC] Clang analyzer: fix dead stores / reduce scope [RDY] Clang analyzer: fix dead stores / reduce scope Aug 7, 2014

@fwalch fwalch changed the title [RDY] Clang analyzer: fix dead stores / reduce scope [WIP] Clang analyzer: fix dead stores / reduce scope Aug 7, 2014

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 7, 2014

Changes LGTM 👍

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 7, 2014

Juicy analyzer warning decreases, I like it! LGTM.

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 7, 2014

Clang doesn't like the change from #define to const size_t:

/home/travis/build/neovim/neovim/src/nvim/screen.c:7813:19: error: variable length array folded to constant array as an extension

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 7, 2014

Clang doesn't like the change from #define to const size_t:

It's strange. cite StackOverflow:

1. static const int var = 5;
2. #define var 5
3. enum { var = 5 };

Under C99, all of these can be used for local arrays.
Technically, using (1) would imply the use of a VLA (variable-length array),
though the dimension referenced by 'var' would of course be fixed at size 5.
@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 7, 2014

I used only const at first, changed it to static const now. But still doesn't work..

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 7, 2014

Perhaps I found the answer:

Differences between all c* and gnu* modes:

  • Arrays that are VLA’s according to the standard, but which can be constant folded by the frontend are treated as fixed size arrays. This occurs for things like “int X[(1, 2)];”, which is technically a VLA. c* modes are strictly compliant and treat these as VLAs.

If I understand correctly clang warn us that this array compiled as can be replaced by fixed size array... Though it seems strange to me. What do you think guys?

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 7, 2014

It seems like this is the result of -Wpedantic -std=gnu99: ref
Right now I don't have better solution than revert to #define... Maybe anyone else have some thoughts?

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 7, 2014

It's strange. cite StackOverflow

Ugh indeed. it bites me every time as well. One of the few points C++ has over C99. Yea. I don't even know if we can define a function local enum. Otherwise the define seems best. No need to go creating VLAs on some compilers.

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 7, 2014

It seems like this is the result of -Wpedantic -std=gnu99: ref
Right now I don't have better solution than revert to #define... Maybe anyone else have some thoughts?

We could suppress the warning using -Wno-gnu-folding-constant, but I'm not sure if that's worth it.. changed it back to #define.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 7, 2014

We could suppress the warning using -Wno-gnu-folding-constant, but I'm not sure if that's worth it.. changed it back to #define.

Let's stick to the standard on this one :). For statically size arrays, either a #define or an enum.

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 7, 2014

Build was successful now, marking [RDY]. Thanks, guys!

@fwalch fwalch changed the title [WIP] Clang analyzer: fix dead stores / reduce scope [RDY] Clang analyzer: fix dead stores / reduce scope Aug 7, 2014

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 7, 2014

@fwalch 👍

justinmk added a commit that referenced this pull request Aug 8, 2014

Merge pull request #985 from fwalch/clang-analyzer-dead-assignments
Clang analyzer: fix dead stores / reduce scope

@justinmk justinmk merged commit 19f44fd into neovim:master Aug 8, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 8, 2014

Nice work everyone!

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 9, 2014

-10 warnings! Good first step!

@fwalch fwalch deleted the fwalch:clang-analyzer-dead-assignments branch Aug 9, 2014

Grimy pushed a commit to Grimy/neovim that referenced this pull request Jan 7, 2015

Merge pull request neovim#985 from davidjb/master
Enable Function syntax for keyword seeding

dwb pushed a commit to dwb/neovim that referenced this pull request Feb 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment