Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor <msebor at gmail dot com>]

> > David, in the way of feedback, I spent hours debugging the simple
> > multiline test I added.  It seems that DejaGnu is exquisitely
> > sensitive to whitespaces in the multiline output.  I appreciate
> > that spacing is essential to lining up the caret and the tildes
> > with the text of the program but tests fail even when all the
> > expected output is lined up right in the multiline directive but
> > off by a space (or tab) with respect to the actual output.  That
> > DejaGnu output doesn't make this very clear at all makes debugging
> > these types of issues a pain.  I wonder if there's a better to do
> > this.
>
> Gah; I'm sorry this was such a pain to do.
>
> I tend to just copy&paste the stuff I want to quote directly from
> Emacs's compilation buffer.

Yes, I did start with that.  The problem is that I have a script
that I run to help massage my patches to the format required by
the GCC Coding Conventions.  The script mainly replaces blocks
of spaces with tabs. So while my initial patch worked, the final
one didn't because of the tabs, and it took me hours to figure
out why.  Because I couldn't see the difference I thought it was
a bug in DejaGnu or some other tool that was different between
the two machines I was using for testing the two patches.  Until
it occurred to me to diff the test itself... (FWIW, I think this
style convention is bad and should be abolished in favor of using
only plain spaces.)

> However a nit, which I think is why you had problems...
...
> You have two pairs of dg-begin/end-multiline-output, but I think it's
> better to have four; use a single begin/end pair for each call to
> diagnostic_show_locus.  Hopefully doing that ought to make things a bit
> less sensitive to whitespace, and easier to debug: each begin/end can
> be handled by itself, whereas by combining them it's harder for
> multiline.exp to detect them.  If combined, they can only be detected
> if all of the dg-warning/dg-messages work (and are thus pruned by
> prune.exp), if any aren't pruned, the multiline outputs will also fail.
>  Maybe this exacerbated the problem?

Maybe.  I remember experimenting with the multiline directives when
I first added the test and was struggling to get it to work.  I think
the problems I was having were unrelated to tabs but probably had
something to do with the exact number of leading spaces.

FWIW, to make the multiline directives less prone to this type of
a problem it seems that instead of counting each leading space and
tab character and treating them as ordinary they could verify that
the first non-whitespace character on each line of the actual output
lines up with the first non-whitespace character of the expected
output with some constant offset.  That way spaces vs tabs shouldn't
matter as long as they were used consistently (or they could be made
not to matter at all if a tab was treated as a single space).  What
do you think about that?

> So something like this, grouping the 4 diagnostics together with their
> diagnostic_show_locus output by opening and closing comments (line
> numbers will need adjusting):

Thanks.  Let me apply it and see how it works.

> Another nit, if I may: FWIW I'm not in love with the wording of the messages.  Sorry to bikeshed, but how about:
>   warning: buffer overflow will occur when writing terminating NUL
> and:
>   note: formatted output of 2 bytes into a destination of size 1
> or somesuch.

I won't claim the text of the messages is perfect but I did spend
a lot of time tweaking them.  That's not to say they can't be
improved but changing them means quite a bit of work adjusting
the tests.  At this point, I'd like to focus on getting the patch
committed.  After that, if there's still time, I'm happy to take
feedback and tweak the diagnostics based on it.

Thanks again for your help and the suggestions above!

Martin