Fun and games with, er, test coverage

[ewx]

Here's some gcov output:

       34:  599:  switch(pflags & (SSH_FXF_CREAT|SSH_FXF_TRUNC|SSH_FXF_EXCL)) {
        -:  600:  case 0:
       11:  601:    flags |= SSH_FXF_OPEN_EXISTING;
       11:  602:    break;
        -:  603:  case SSH_FXF_TRUNC:
        -:  604:    /* The drafts demand that SSH_FXF_CREAT also be sent making this formally
        -:  605:     * invalid, though there doesn't seem any good reason for them to do so:
        -:  606:     * the client intent seems clear.*/
        2:  607:    flags |= SSH_FXF_TRUNCATE_EXISTING;
        2:  608:    break;
        -:  609:  case SSH_FXF_CREAT:
        2:  610:    flags |= SSH_FXF_OPEN_OR_CREATE;
        2:  611:    break;
        -:  612:  case SSH_FXF_CREAT|SSH_FXF_TRUNC:
       17:  613:    flags |= SSH_FXF_CREATE_TRUNCATE;
       17:  614:    break;
        -:  615:  case SSH_FXF_CREAT|SSH_FXF_EXCL:
        -:  616:  case SSH_FXF_CREAT|SSH_FXF_TRUNC|SSH_FXF_EXCL: /* nonsensical */
    #####:  617:    flags |= SSH_FXF_CREATE_NEW;
    #####:  618:    break;
        -:  619:  default:
    #####:  620:    return SSH_FX_BAD_MESSAGE;
        -:  621:  }

Why are lines 617 and 618 never executed (that's what the hash signs mean)? (If you saw the answer on IRC then you're not eligible l-) Here's a couple of wrong answers:

• Because the tests don't exercise that case. They do, via SSH_FXF_CREAT|SSH_FXF_EXCL.
• Because you turned optimization on, despite the gcov manual telling you not to. Nope, -O0 and other options exactly as required.

(620 isn't executed because the tests don't go there. Apart from the code being obviously correct, no working client is going to send the server there anyway, so I don't really care.)

Comments

[simont]

<spoiler>Because SSH_FXF_CREATE_NEW is equal to zero, so the |= operation is null and the switch simply doesn't bother to do anything at all in that case?</spoiler>

[ewx.livejournal.com]

Close - it turns out that (at least in gcc 3.3 and 3.4) |= 0 produces no code at all even outside the context of a switch statement and even with -O0.

[gerald_duck]

I'm guessing it could happen if the values of the various SSH_FXF_* macros were bogus — i.e. not bitwise independent — but I can't offhand see a way that wouldn't produde a compiler error.

And surely in security-critical code you have to test cases no working client is going to exercise, in case a malicious client decides to have a go?

And surely

[ewx.livejournal.com]

True if you do arrange to run the server on a security boundary; which is easy to do, but not what it's actually for. If you just wanted public file serving you'd normally use something like HTTP instead.

[ewx.livejournal.com]

(But it's a good point; I'll leave a note in the README concerning what the test suite does and doesn't cover.)

[fanf]

Perhaps _EXCL == 0

I'm not sure _CREATE_NEW == 0 being the cause is consistent with -O0. Another possibility, that flags already has the _CREATE_NEW bit(s) set, would also require optimization for the compiler to discard the code.

[imc.livejournal.com]

_EXCL == 0 won't compile because it creates duplicate cases.

And when I tried it (gcc 4.1.1) _CREATE_NEW == 0 said that the instruction corresponding to line 617 was executed even with -O2 (and even though -O2 doesn't appear to produce any code for that instruction).

What puzzles me is that the numbers don't add up: the switch instruction was executed 34 times but the various cases were only executed 32 times in total (which leads me to think that line 617 was executed twice).

[ewx.livejournal.com]

Inspection of the code shows there's no instruction generated (in a simple test case - the code in question has been rearranged to confuse the profiler less, though the old arrangement is still there in bzr).

I tried a simpler test case and although inspection of the output reveals that no code is generated, the profiler does think the code ran.

        1:    5:int main(void) {
        1:    6:  int x = 99;
        -:    7:
        1:    8:  f();
        1:    9:  x |= 0;
        1:   10:  f();
        1:   11:  return x;
        -:   12:}

So perhaps in this case the profiler is being clever (or stupid in a way that gives superficially more correct results).

[pm215]

In practice gcc's -O0 option doesn't mean "no optimisation at all" but "don't do things that would make debugging too hard and don't slow the compilation down". In particular at least half of fold-const.c is enabled regardless of optimisation setting, including "x|0 => x". I think the rationale here is that it can actually speed the compile up by giving the later stages less work to do. (It also wouldn't surprise me if some backends assumed it had been run.)