On 08/03/2017 02:45 AM, Richard Biener wrote:
On Wed, Aug 2, 2017 at 7:10 PM, Jeff Law <l...@redhat.com> wrote:
On 08/01/2017 03:25 AM, Richard Biener wrote:
On Tue, Aug 1, 2017 at 11:23 AM, Richard Biener
<richard.guent...@gmail.com> wrote:
On Tue, Aug 1, 2017 at 4:27 AM, Martin Sebor <mse...@gmail.com> wrote:
Richard,
in discussing this work Jeff mentioned that your comments on
the tree-ssa-alias.c parts would be helpful. When you have
a chance could you please give it a once over and let me know
if you have any suggestions or concerns? There are no visible
changes to existing clients of the pass, just extensions that
are relied on only by the new diagnostics.
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01264.html
I expect to revisit the sprintf %s patch mentioned below and
see how to simplify it by taking advantage of the changes
implemented here.
Your ptr_deref_alias_decl_p returns true when must_alias is true
but there is no must-alias relationship.
The ao_ref_init_from_ptr_and_size doesn't belong there.
I dislike all of the changes related to returning an alias "size".
Please keep away from the alias machinery.
Warning during folding is similarly bad design. Please don't add
more such things.
Thanks for putting this on my radar though.
Richard.
Oh, for a constructive comment this all feels like sth for either
sanitizers or a proper static analysis tool. As I outlined repeatedly
I belive GCC could host a static analysis tool but it surely should
not be interwinded into it's optimization passes or tools.
I disagree strongly here.
Sanitiers catch problems after the fact and only if you've compiled your
code with sanitization and manage to find a way to trigger the problem path.
Other static analysis tools are useful, but they aren't an integral part
of the edit, compile, debug cycle and due to their cost are often run
long after code was committed.
Finding useful warnings that can be issued as part of the compile, edit,
debug cycle is, IMHO, far more useful than sanitizers or independent
static checkers.
--
I think finding a way to exploit information that our various analyzers
can provide to give precise warnings is a good thing. So it seemed
natural that if we wanted to ask a must-alias question that we should be
querying the alias oracle rather than implementing that kind of query
within the sprintf warnings. I'm not sure why you'd say "Please keep
away from the alias machinery".
I'm a little confused here...
Well, simply because the way as implemented isn't a must-alias query
but maybe one that's good enough for warnings (reduces false positives
but surely doesn't eliminate them).
I'm very interested in reducing the rate of false positives in
these and all other warnings. As I mentioned in my comments,
I did my best to weed them out of the implementation by building
GDB, Glibc, Busybox, and the Linux kernel. That of course isn't
a guarantee that there aren't any. But the first implementation
of any non-trivial feature is never perfect, and hardly any
warning of sufficient complexity is free of false positives, no
matter here it's implemented (the middle-end, front-end, or
a standalone static analysis tool). If you spotted some cases
I had missed I'd certainly be grateful for examples. Otherwise,
they will undoubtedly be reported as more software is exposed
to the warning and, if possible, fixed, as happens with all
other warnings.
There's a must alias query already, in stmt_kills_ref_p. It's a matter
of refactoring to make a refs_must_alias_p.
Then propose that "overlap range" stuff separately.
I appreciate constructive suggestions for improvements and I will
look into the stmt_kills_ref_p suggestion. But since my work
elicited such an strong response from you I feel I should explain:
I tried to make my changes as unintrusive as possible. The alias
oracle is a new area for me and I didn't want to make mistakes in
the process of making overly extensive modifications to it.
But I'm against lumping this all in as an innocent change suggesting
the machinery can do sth (must alias) when it really can't. I also
do not like adding a "must alias" bool to the may-alias APIs as
the implementation is fundamentally may-alias, must-alias really
is very different.
I certainly want to do the right thing and implement the warning
in a way that makes the most sense. As I said, I'll look into
the refactoring, but since my testing shows the current code to
work well as is, it would be helpful if you could provide more
details about what it is that concerns you with it and what cases
of false positives you are worried about. (Examples of code that
demonstrate the false positives would be especially valuable.)
That being said, after much thought, I do have to let you know
that I take offense at both your tone and your insinuation that
I tried to sneak in some subversive changes. I did what I thought
was right. If it can be done better I'm glad to hear the details
of what's wrong with it and in what ways the approach you prefer
is better. But I would be grateful for a more respectful reply
in the future.
Martin
