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. 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. jeff > > Martin
