Re: [PATCH] RFC: On-demand locations within string-literals

[David Malcolm <dmalcolm at redhat dot com>]

On Sat, 2016-07-23 at 15:35 -0600, Martin Sebor wrote:
> On 07/08/2016 03:49 PM, David Malcolm wrote:
> > This patch implements precise tracking of source locations for the
> > individual chars within string literals, so that we can e.g.
> > underline
> > specific ranges in -Wformat diagnostics.
> ...
> > Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
> > 
> > Thoughts?
> 
> I applied the patch to my -Wformat-length branch and managed to
> get it to work in the sprintf-length pass in the middle end, but
> I'm not sure I did it right (I suspect not, or at least not the
> way it should be done).  I couldn't find other places where the
> bits it depends on are used (I copied bits and pieces of the code
> I used from the C front end until it all fell into place ).  It
> would be great if there was a tutorial on how to plug it in where
> it isn't used yet (as in the middle end).  For example, how do
> I determine what CLK_XXX constant I should pass to the
> cpp_create_reader() function in the middle end?

Thanks for trying it out.  I've been reworking the patch, and I have a
much more robust version that I think is close to being ready, with an
easier interface, some bug fixes, and, ahem, EBCDIC support...  and
more usefully to the average user, gracefully handling of other
encodings.

I'd hoped to post the new version on Monday (along with code that wires
it up into c-common/c-format.c).

Though reading your post, I realize now that my interface is in c
-common.h, and that's not going to be usable for you from the middle
end.  Presumably I'm going to need to rework things to be usable from a
gimple-*.c file.  Is it acceptable to somehow wire this up to a
langhook, so that locations for sprintf-style strings are only
available for c-family, with a graceful fallback on other frontends?

Alternatively, I can post the c-common.h interface for review, and we
can get that good enough for trunk, and then we can work on
generalizing it so it's usable from the middle-end for your new pass.

> I ran my tests with the patch and while it handled a good number
> of test cases it eventually crashed.  A test case for the ICE is
> attached though it needs my -Wformat-length patch to trigger it.
> You may be able to tell what's wrong from the debugger context
> (included in the test case).  If not, you may be able to
> reproduce it by applying my latest patch and hacking the three
> argument overload of location_from_offset() the way I did (in
> the xyz.i comment).  Or I can send you my latest patch with all
> this in place.

My patch has changed a lot, so I don't know how useful debugging is
going to be.  I'd be interested in seeing your latest patch.

> Beyond that, the range normally works fine, except when macros
> are involved like they are in my tests.  You can see the effect
> in the range.out file.  (This works without your patch but it
> could very well be because I didn't set it up right.)

Sadly I can't figure out what's going wrong - but the code's changed a
lot at my end since then.  Sorry.

> That's all I have for now.

Martin
> 

Thanks for posting this, it's very helpful.
Dave

>