Re: [PING] [PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468)

[Jeff Law <law@redhat.com>]

On 03/01/2018 02:17 PM, Martin Sebor wrote:
> 
> I read you last reply as asking me to handle multiple edges.
Sorry, I should have been clearer.

You need to be prepared for the possibility of multiple edges and handle
them in a conservatively correct way.  The most likely way to get
multiple outgoing edges is going to be via exception handling.

In this code I think that's easily accomplished by making the code which
walks into the successor block(s) conditional on single_succ_p (bb) and
that the edge is not marked as EDGE_ABNORMAL.

> The original patch handled just one edge and didn't bother
> with EDGE_ABNORMAL because I wanted to keep it simple.
Understood.  But that's not safe in the sense that once you have
multiple successors, you don't know which one you will transfer control
to -- thus you have to check all of them for a suitable store.  If any
doesn't have a suitable store, then you'd have to warn.

Essentially the question you need to answer is "given I'm in block X, do
all paths from block X have a suitable store to terminate the string
prior to a use".  You can handle that in the trivial case with the code
you're proposing in this patch, and that's fine for gcc-8.

But the "right" way to answer that question is to look at the virtual
operand chains.  Though as we've discussed that may not necessarily be a
good thing.





  But
> it sounds like you want me to go back to the first version
> and just add a check for EDGE_ABNORMAL.  The attached patch
> does that (plus it skips over any non-debug statements in
> the successor block). 
Correct.  I think your original patch with a check for single_succ_p is
the direction I think we want for gcc-8.



 Besides the usual I retested it with
> the Linux kernel.  There are just three instances of
> the warning in my default configuration but I know there
> are a lot more in more extensive builds.
I've actually got a fair amount of data here on kernel builds using the
trunk on a variety of architectures.  I haven't gone through it all yet,
but there's certainly some string-op truncation warnings and a few
others.  I haven't figured out how best to aggregate that info and I
don't want to duplicate Arnd's work.




> 
> 
>> I glanced over the tests and I didn't see any that would benefit from
>> handling multiple edges or the recursion (every one of the dg-bogus
>> markers should be immediately transferring control to the null
>> termination statement AFAICT).
> 
> I've added some more test cases involving C++ exceptions (and
> found one false positive(*)) but I don't have a test to exercise
> blocks with multiple outgoing edges.  For future reference, how
> would I create one?
EH is the best bet to create them.  You might also be able to get one if
we've got fake edges in the CFG (necessary for things like
post-dominator computation).  async exceptions could likely create them
fairly easily.


My comment was more about not seeing the need to look into successor
blocks if there's more than one.


Can you file a bug report on the false positive so that we have a
reminder to revisit looking at the virtual operand chains as a better
solution here?

I think this version is OK.

Thanks, and again sorry for the confusion.


Jeff