On Tue, 2016-03-22 at 10:28 -0400, David Malcolm wrote:
> On Tue, 2016-03-01 at 20:18 +0100, Richard Biener wrote:
> > On March 1, 2016 7:51:01 PM GMT+01:00, David Malcolm <
> > dmalc...@redhat.com> wrote:
> > > The wording of our output from -Wmisleading-indentation is rather
> > > confusing, as noted by Reddit user "sysop073" here:
> > > https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisle
> > > ad
> > > ingindentation_vs_goto_fail/d0eonwd
> > >
> > > > The way they split up the warning looks designed to trick you.
> > > > sslKeyExchange.c:631:8: warning: statement is indented as if it
> > > > were
> > > guarded by... [-Wmisleading-indentation]
> > > > goto fail;
> > > > ^~~~
> > > > sslKeyExchange.c:629:4: note: ...this 'if' clause, but it is
> > > > not
> > > > if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) !=
> > > > 0)
> > > > ^~
> > > > You read the first half and it sounds like goto fail; is
> > > > guarding
> > > something. Why would it not be:
> > > > sslKeyExchange.c:631:8: warning: statement is wrongly
> > > > indented...
> > > [-Wmisleading-indentation]
> > > > goto fail;
> > > > ^~~~
> > > > sslKeyExchange.c:629:4: note: ...as if it were guarded by this
> > > > 'if'
> > > clause
> > > > if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) !=
> > > > 0)
> > > > ^~
> > >
> > > I agree that the current wording is suboptimal; certainly the
> > > wording
> > > would be much clearer if the wording of the "warning" only spoke
> > > about
> > > the
> > > statement in question, and the "note"/inform should then talk
> > > about
> > > the
> > > not-really-guarding guard.
> > >
> > > One rewording could be:
> > >
> > > sslKeyExchange.c:631:8: warning: statement is misleadingly
> > > indented...
> > > [-Wmisleading-indentation]
> > > goto fail;
> > > ^~~~
> > > sslKeyExchange.c:629:4: note: ...as if it were guarded by this
> > > 'if'
> > > clause, but it is not
> > > if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
> > > ^~
> > >
> > > However another Reddit user ("ksion") noted here:
> > > https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisle
> > > ad
> > > ingindentation_vs_goto_fail/d0eqyih
> > > that:
> > > > This is just passive voice, there is nothing tricky about it.
> > > > What I find more confusing -- and what your fix preserves -- is
> > > > the
> > > > reversed order of offending lines of code in the source file
> > > > and
> > > > the
> > > message.
> > > >
> > > > I'd rather go with something like this:
> > > > sslKeyExchange.c:629:4: warning: indentation of a statement
> > > > below
> > > this 'if' clause... [-Wmisleading-indentation]
> > > > if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) !=
> > > > 0)
> > > > ^~
> > > > sslKeyExchange.c:631:8: note: ...suggests it is guarded by the
> > > > 'if'
> > > clause, but it's not
> > > > goto fail;
> > > > ^~~~
> > > > You can even see how the indentation is wrong in the very error
> > > message.
> > >
> > > which suggests reversing the order of the messages, so that they
> > > appear
> > > in "source" order.
> > >
> > > I think this is a big improvement in the readability of the
> > > warning.
> > >
> > > The attached patch implements such a change, so that the warning
> > > is
> > > issued on the supposed guard clause, followed by the note on the
> > > statement that isn't really guarded.
> > >
> > > Some examples:
> > >
> > > Wmisleading-indentation-3.c:18:3: warning: this 'for' clause does
> > > not
> > > guard... [-Wmisleading-indentation]
> > > for (i = 0; i < 10; i++)
> > > ^~~
> > > Wmisleading-indentation-3.c:20:5: note: ...this statement, but
> > > the
> > > latter is indented as if it does
> > > prod[i] = a[i] * b[i];
> > > ^~~~
> > > Wmisleading-indentation-3.c: In function 'fn_6':
> > > Wmisleading-indentation-3.c:39:2: warning: this 'if' clause does
> > > not
> > > guard... [-Wmisleading-indentation]
> > > if ((err = foo (b)) != 0)
> > > ^~
> > > Wmisleading-indentation-3.c:41:3: note: ...this statement, but
> > > the
> > > latter is indented as if it does
> > > goto fail;
> > > ^~~~
> > >
> > > I'm not totally convinced by my new wording; maybe the note could
> > > also mention the kind of clause ('if'/'while'/'else'/'for') for
> > > clarity, maybe something like:
> > >
> > > Wmisleading-indentation-3.c: In function 'fn_6':
> > > Wmisleading-indentation-3.c:39:2: warning: this 'if' clause does
> > > not
> > > guard... [-Wmisleading-indentation]
> > > if ((err = foo (b)) != 0)
> > > ^~
> > > Wmisleading-indentation-3.c:41:3: note: ...this statement, but
> > > the
> > > latter is misleadingly indented
> > > as if it is guarded by the 'if'
> > > goto fail;
> > > ^~~~
> > >
> > > Also, it's slightly clunkier when it comes to macros, e.g.:
> > >
> > > Wmisleading-indentation-3.c: In function 'fn_14':
> > > Wmisleading-indentation-3.c:60:3: warning: this 'for' clause does
> > > not
> > > guard... [-Wmisleading-indentation]
> > > for ((VAR) = (START); (VAR) < (STOP); (VAR++))
> > > ^
> > > Wmisleading-indentation-3.c:65:3: note: in expansion of macro
> > > 'FOR_EACH'
> > > FOR_EACH (i, 0, 10)
> > > ^~~~~~~~
> > > Wmisleading-indentation-3.c:67:5: note: ...this statement, but
> > > the
> > > latter is indented as if it does
> > > bar (i, i);
> > > ^~~
> > >
> > > That said, the reordering idea is something I'd like to do for
> > > GCC
> > > 6.
> > > Failing that, there's the tweak to the wording suggested at the
> > > top.
> > >
> > > OK for trunk? (assuming we can agree on the wording, and that the
> > > latest
> > > version passes testing)
> >
> > OK if others don't disagree.
>
> Thanks.
>
> I took the liberty of tweaking the wording to the longer one given
> above, which repeats the type of the clause (for clarity), refreshed
> it
> to cover new tests for -Wmisleading-indentation, verified bootstrap
> and
> regrtest, and committed it as r234403 (attached, for reference).
>
> Here's what the new wording looks like on CVE-2014-1266:
>
> sslKeyExchange.c: In function ‘SSLVerifySignedServerKeyExchange’:
> sslKeyExchange.c:629:3: warning: this ‘if’ clause does not guard... [
> -Wmisleading-indentation]
> if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
> ^~
> sslKeyExchange.c:631:5: note: ...this statement, but the latter is
> misleadingly indented as if it is guarded by the ‘if’
> goto fail;
> ^~~~
>
> I plan to update the website accordingly.
I've committed the corresponding change to the website; see the
attached.
Validated; I've also manually doublechecked the built pages on the live
site.
Dave
Index: htdocs/gcc-6/changes.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-6/changes.html,v
retrieving revision 1.71
diff -u -p -r1.71 changes.html
--- htdocs/gcc-6/changes.html 14 Mar 2016 10:16:16 -0000 1.71
+++ htdocs/gcc-6/changes.html 22 Mar 2016 15:39:36 -0000
@@ -198,12 +198,12 @@ within strings:
<a href="https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-1266";>CVE-2014-1266</a>:
<blockquote><pre>
<b>sslKeyExchange.c:</b> In function <b>'SSLVerifySignedServerKeyExchange'</b>:
-<b>sslKeyExchange.c:631:8:</b> <span class="boldmagenta">warning:</span> statement is indented as if it were guarded by... [<span class="boldmagenta">-Wmisleading-indentation</span>]
- <span class="boldmagenta">goto</span> fail;
- <span class="boldmagenta">^~~~</span>
-<b>sslKeyExchange.c:629:4:</b> <span class="boldcyan">note:</span> ...this 'if' clause, but it is not
+<b>sslKeyExchange.c:629:3:</b> <span class="boldmagenta">warning:</span> this <b>'if'</b> clause does not guard... [<span class="boldmagenta">-Wmisleading-indentation</span>]
<span class="boldcyan">if</span> ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
<span class="boldcyan">^~</span>
+<b>sslKeyExchange.c:631:5:</b> <span class="boldcyan">note:</span> ...this statement, but the latter is misleadingly indented as if it is guarded by the <b>'if'</b>
+ <span class="boldmagenta">goto</span> fail;
+ <span class="boldmagenta">^~~~</span>
</pre></blockquote>
This warning is enabled by <code>-Wall</code>.</li>
</ul>
Index: htdocs/gcc-6/porting_to.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-6/porting_to.html,v
retrieving revision 1.20
diff -u -p -r1.20 porting_to.html
--- htdocs/gcc-6/porting_to.html 2 Mar 2016 21:36:56 -0000 1.20
+++ htdocs/gcc-6/porting_to.html 22 Mar 2016 15:39:36 -0000
@@ -382,12 +382,12 @@ the code might mislead a human reader ab
<blockquote><pre>
<b>sslKeyExchange.c:</b> In function <b>'SSLVerifySignedServerKeyExchange'</b>:
-<b>sslKeyExchange.c:631:8:</b> <span class="boldmagenta">warning:</span> statement is indented as if it were guarded by... [<span class="boldmagenta">-Wmisleading-indentation</span>]
- <span class="boldmagenta">goto</span> fail;
- <span class="boldmagenta">^~~~</span>
-<b>sslKeyExchange.c:629:4:</b> <span class="boldcyan">note:</span> ...this 'if' clause, but it is not
+<b>sslKeyExchange.c:629:3:</b> <span class="boldmagenta">warning:</span> this <b>'if'</b> clause does not guard... [<span class="boldmagenta">-Wmisleading-indentation</span>]
<span class="boldcyan">if</span> ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
<span class="boldcyan">^~</span>
+<b>sslKeyExchange.c:631:5:</b> <span class="boldcyan">note:</span> ...this statement, but the latter is misleadingly indented as if it is guarded by the <b>'if'</b>
+ <span class="boldmagenta">goto</span> fail;
+ <span class="boldmagenta">^~~~</span>
</pre></blockquote>
<p>
