> -----Original Message----- > From: Martin Sebor <mse...@gmail.com> > Sent: Thursday, July 25, 2019 2:08 AM > To: Jeff Law <l...@redhat.com>; JiangNing OS > <jiangn...@os.amperecomputing.com>; gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] PR91195: fix -Wmaybe-uninitialized warning for > conditional store optimization > > On 7/24/19 11:12 AM, Jeff Law wrote: > > On 7/24/19 10:09 AM, Martin Sebor wrote: > >> On 7/24/19 9:25 AM, Jeff Law wrote: > >>> On 7/23/19 10:20 AM, Martin Sebor wrote: > >>>> On 7/22/19 10:26 PM, JiangNing OS wrote: > >>>>> This patch is to fix PR91195. Is it OK for trunk? > >>>>> > >>>>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog index > >>>>> 711a31ea597..4db36644160 100644 > >>>>> --- a/gcc/ChangeLog > >>>>> +++ b/gcc/ChangeLog > >>>>> @@ -1,3 +1,9 @@ > >>>>> +2019-07-22 Jiangning Liu <jiangning....@amperecomputing.com> > >>>>> + > >>>>> + PR middle-end/91195 > >>>>> + * tree-ssa-phiopt.c (cond_store_replacement): Work around > >>>>> + -Wmaybe-uninitialized warning. > >>>>> + > >>>>> 2019-07-22 Stafford Horne <sho...@gmail.com> > >>>>> * config/or1k/or1k.c (or1k_expand_compare): Check for > >>>>> int before diff --git a/gcc/tree-ssa-phiopt.c > >>>>> b/gcc/tree-ssa-phiopt.c index b64bde695f4..7a86007d087 100644 > >>>>> --- a/gcc/tree-ssa-phiopt.c > >>>>> +++ b/gcc/tree-ssa-phiopt.c > >>>>> @@ -2240,6 +2240,14 @@ cond_store_replacement (basic_block > >>>>> middle_bb, basic_block join_bb, > >>>>> tree base = get_base_address (lhs); > >>>>> if (!auto_var_p (base) || TREE_ADDRESSABLE (base)) > >>>>> return false; > >>>>> + > >>>>> + /* The transformation below will inherently introduce a > >>>>> +memory > >>>>> load, > >>>>> + for which LHS may not be initialized yet if it is not in > >>>>> +NOTRAP, > >>>>> + so a -Wmaybe-uninitialized warning message could be triggered. > >>>>> + Since it's a bit hard to have a very accurate > >>>>> +uninitialization > >>>>> + analysis to memory reference, we disable the warning here to > >>>>> avoid > >>>>> + confusion. */ > >>>>> + TREE_NO_WARNING (lhs) = 1; > >>>> > >>>> The no-warning bit is sometimes (typically?) set by the middle-end > >>>> after a warning has been issued, to avoid triggering other warnings > >>>> down the line for an already diagnosed expression. Unless it's > >>>> done just for the purposes of a single pass and the bit is cleared > >>>> afterwards, using it to avoid potential false positives doesn't > >>>> seem like a robust solution. It will mask warnings for constructs > >>>> that have been determined to be invalid. > >>>> > >>>> FWIW, the middle-end is susceptible to quite a few false positives > >>>> that would nice to avoid. We have discussed various approaches to > >>>> the problem but setting the no-warning bit seems like too blunt of > >>>> an instrument. > >>> All true. > >>> > >>> But in the case JiangNing is working with the transformation > >>> inherently can introduce an uninitialized read. It seems reasonable > >>> to mark those loads he generates that don't have a dominating read. > >>> > >>> His code takes something like > >>> > >>> if (x) > >>> *p = <someval> > >>> > >>> And turns it into > >>> > >>> t1 = *p; > >>> t2 = x ? <someval> : t1; > >>> *p = t2; > >>> > >>> In the past we required there be a dominating read from *p (among > >>> other restrictions). That requirement was meant to ensure *p isn't > >>> going to fault. Jiang's work relaxes that requirement somewhat for > >>> objects that we can prove aren't going to fault via other means. > >>> > >>> Can setting TREE_NO_WARNING on the inserted loads inhibit warnings? > >>> Certainly. However, I believe we use it in other places where we > >>> know the code we're emitting is safe, but can cause a warning. I > >>> think Jiang's work falls into that category. > >>> > >>> I do think the bit should only be set if we don't have a dominating > >>> load to minimize cases where we might inhibit a valid warning. > >> > >> I was thinking of a few cases where setting the no-warning bit might > >> interfere with detecting bugs unrelated to uninitialized reads: > >> > >> 1) -Warray-bounds in gimple-ssa-warn-restrict and tree-vrp > >> 2) -Wstringop-overflow in tree-ssa-strlen (other than for calls > >> to built-ins) > >> > >> I couldn't come up with a test case that shows how it might happen > >> with this patch but I didn't spend too much time on it. > > I bet we could find one and it's more likely to show up on aarch64 > > than > > x86 due to costing issues. Either way we're making a bit of a > > judgment call -- an extra false positive here due to a load the > > compiler inserted on a path that didn't have one before, or > > potentially missing a warning on that load because of the > TREE_NO_WARNING. > > > > I believe the false positive here is worse than the potential missed > > warning. > > > > > >> > >> There are a number of existing instances of setting TREE_NO_WARNING > >> to suppress -Wuninitialized, and some are the cause of known problems. > >> Bugs 51545, 58950, 74762, 74765 or 89697 are examples. They all boil > >> down to the fact that there's just one bit for all warnings. Jakub > >> mentioned adding a hash-map for this. That seems like a simple and > >> good solution. > > I'm not sure how that really helps here. We marking the MEM on the > > LHS and that's not a shared object. I don't see how it's going to be > > significantly different using a hash map vs the bit in this circumstance. > > I don't know what Jakub had in mind but the mapping I envision is one like > hash_map<tree, bitmap> that would make it possible to set a bit for each > distinct warning for every tree node. It would let us set a bit for - > Wuninitialized while leaving the bit for -Warray-bounds (and all other > warnings) clear. > > > > > If the bit were on an SSA_NAME, or a _DECL node, then the flag bit is > > shared and would be a much larger concern. > > For shared objects the mapping would have to be more involved but I > haven't thought about it in any detail to have an idea what it might look > like. > > Anyway, if/when someone does come up with a solution for this we will have > to go through all the places where the no-warning bit is set and replace them > with whatever replacement we come up with. > One instance more or less won't make a difference. I just wanted to point > out that setting the bit is not a robust solution.
Hi Martin, I see "TREE_NO_WARNING (repl) = 1;" is still in generate_subtree_copies, and it seems PR89697 is unresolved, so we don't have the new hash_map mechanism to make difference for -Wunintialized and all others yet, correct? It sounds we should avoid using "TREE_NO_WARNING (xxx) = 1" if only xxx is not a newly build expr, but I can see there are still a lot of code in trunk using it this way. Would it be OK to fix the issue this way first and then handle all cases together later? After all, as Jeff pointed out false positive of raising uninitialization warning is worse than missing the warning. The bugzilla examples you give are all for missing warnings. Thanks, -Jiangning > > Martin
