On Mon, Aug 7, 2017 at 1:08 AM, Martin Sebor <mse...@gmail.com> wrote: > 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.)
I am not worried about false positive warnings. I am worried about an API used by optimization passes that advertises an ability (must-alias query, whoo!) that may end up producing wrong-code. I believe must-alias is so fundamentally different from may-alias that re-using the same API (with a mere change in one argument) is wrong. We _do_ have a must-alias API as I said. _Any_ change to the alias API needs to give conservatively correct answers. > 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. Sorry, but the Subject of the mails with the patches say "enhance -Wrestrict" which causes me to look away quickly. But most of the patch was actually changes to the alias API and implementation. You should have sumbitted those separately or more prominetly advertise them. I took an offense on this tactic, if that wasn't your intention then take this reaction as a reason to better separate changes to separate parts of the compiler. Thanks, Richard. > Martin
