Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Martin Sebor <msebor@gmail.com>]

On 11/23/2016 12:47 PM, Jeff Law wrote:
> On 11/23/2016 12:32 PM, Martin Sebor wrote:
>
>>> My worry here would be a hash collision.  Then we'd be using object
>>> sizes from the wrong function.
>>
>> Ah, right, that might explain the ICE I just noticed during Ada
>> bootstrap.  Is there some other way to uniquely identify a function?
>> A DECL_UID maybe?
> DECL_UID would be your best bet.  But ISTM that trying to avoid
> invocations by reusing data from prior passes is likely to be more
> fragile than recomputing on a per-pass basis -- as long as the number of
> times we need this stuff is small (as I suspect it is).
>
>>
>>>
>>> Isn't the goal here to be able to get format-length warnings when there
>>> aren't explicit calls to _b_o_s in the IL?   Can't you initialize the
>>> object-size framework at the start of your pass and tear it down when
>>> your pass is complete?  You could do that by exporting the init/fini
>>> routines and calling them directly, or by wrapping that in a class and
>>> instantiating the class when you need it.
>>>
>>> That would avoid having to worry about the GC system entirely since you
>>> wouldn't have stuff living across passes.
>>
>> Yes, that is the immediate goal of this patch.  Beyond it, though,
>> I would like to call this function from anywhere, including during
>> expansion (as is done in my patch for bug 53562 and related).
> But why not detect the builtins during your pass and check there.  ie, I
> don't see why we necessarily need to have checking and expansion
> intertwined together.  Maybe I'm missing something.  Is there something
> that makes it inherently easier or better to implement checking during
> builtin expansion?

I hadn't thought of extending the gimple-ssa-sprintf pass to all
the memxxx and strxxx builtins.  The _chk functions are already
being handled in builtins.c so calling compute_builtin_object_size
for the non-checking ones there and detecting overflow in those
was an easy and, I had hoped, non-controversial enhancement to make.
In a discussion of bug 77784 (handled in the patch for bug 53562)
Jakub also expressed a preference for some of the diagnostics
staying in builtins.c.

I also suspect that the answer to your question is yes.  Range
information is pretty bad in the gimple-ssa-sprintf pass (it looks
like it runs after EVRP but before VRP).  Maybe the pass should run
after VRP?

That said, I defer to you on how to proceed here.  I'm prepared
to do the work(*) but I do worry about jeopardizing the chances
of this patch and the others making it into 7.0.

Martin

PS If I understand what you are suggesting this would mean
extending the gimple-ssa-sprintf pass to the memxxx and strxxx
functions and running the pass later, after VRP.