Bug 32328 - [4.2 Regression] -fstrict-aliasing causes skipped code
Summary: [4.2 Regression] -fstrict-aliasing causes skipped code
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.2.0
: P1 normal
Target Milestone: 4.3.0
Assignee: Not yet assigned to anyone
URL:
Keywords: alias, wrong-code
Depends on:
Blocks:
 
Reported: 2007-06-13 21:18 UTC by John E. / TDM
Modified: 2009-03-30 22:07 UTC (History)
7 users (show)

See Also:
Host: i386-pc-mingw32
Target: i386-pc-mingw32
Build: i386-pc-mingw32
Known to work: 4.1.2 4.3.0
Known to fail: 4.2.0 4.2.1 4.2.2 4.2.5
Last reconfirmed: 2007-06-20 15:12:52


Attachments
Preprocessed example source (1.08 KB, text/plain)
2007-06-13 21:19 UTC, John E. / TDM
Details
Better testcase; compile with -O2, use with alias_main.c (149 bytes, text/plain)
2007-06-20 14:44 UTC, John E. / TDM
Details
Better testcase pt.2; use with alias1.c (256 bytes, text/plain)
2007-06-20 14:46 UTC, John E. / TDM
Details
32328.diff (1.81 KB, patch)
2007-07-16 13:58 UTC, Daniel Berlin
Details | Diff
patch for 4.2 (2.22 KB, patch)
2007-07-16 14:27 UTC, Richard Biener
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John E. / TDM 2007-06-13 21:18:40 UTC
When the attached file is compiled with a mingw32 build of GCC 4.2.0 using the -O2 option (gcc -O2 -c timestamp2.i), lines 159 and 160 are never executed when running the program that uses this file. Since the hashitem function modifies the variable b which is passed to it by-address, leaving those statements out is definitely not a valid optimization. Compiling without -O2 (gcc -c timestamp2.i) gives the correct result. Sorry I can't read assembler, or I'd try to narrow things down more.

This is a regression from 4.1.2, which works correctly with or without -O2. If it helps, the GCC binaries I use (and sources) can be downloaded at http://hosted.filefront.com/tldragon7.
Comment 1 John E. / TDM 2007-06-13 21:19:53 UTC
Created attachment 13701 [details]
Preprocessed example source
Comment 2 Andrew Pinski 2007-06-13 22:21:23 UTC
Do you mean the last two stores to b:
 b->time = time;
 b->progress = found ? 4 : 2;

Though we could have an alias violation if you don't cast back in hashitem to the correct type of the argument.  This is still a bug.
Comment 3 John E. / TDM 2007-06-13 22:27:39 UTC
(In reply to comment #2)
> Do you mean the last two stores to b:
>  b->time = time;
>  b->progress = found ? 4 : 2;
Yes, those two lines are the ones that are wrongly skipped.
Comment 4 John E. / TDM 2007-06-13 22:36:03 UTC
(In reply to comment #2)
> Though we could have an alias violation if you don't cast back in hashitem to
> the correct type of the argument.
hashitem() uses the type as punned to HASHDATA* (where HASHDATA is a struct with a single member of type char*). However, by implementation hashitem() will never assign a new address that doesn't point to the same type as what was passed -- in this case, it will only ever assign another BINDING*.
Comment 5 John E. / TDM 2007-06-13 22:47:47 UTC
Is it possible that an optimization enabled by -O2 *assumes* that hashitem() will conform with strict aliasing by not dereferencing that argument, and thus optimizes those lines away? (Not the case.) If this is what is happening and is the correct behavior, then this is in fact user error and I'm sorry to have wasted your time.
Comment 6 John E. / TDM 2007-06-20 14:44:55 UTC
Created attachment 13743 [details]
Better testcase; compile with -O2, use with alias_main.c

Compiling this file with -O2 and linking with the object file from alias_main.c creates a program that demonstrates the miscompilation. Curiously, compiling with -O2 and -fno-strict-aliasing produces a correct compilation.
Comment 7 John E. / TDM 2007-06-20 14:46:54 UTC
Created attachment 13744 [details]
Better testcase pt.2; use with alias1.c

Compile this file with any or no additional options as desired; linking with the object file from alias1.c to create a program that demonstrates the miscompilation, if alias1.c was compiled with -O2.
Comment 8 John E. / TDM 2007-06-20 14:52:45 UTC
Not seeing any further action or confirmation on this yet, I've gone ahead and created a simpler testcase. It's plain that, when using -O2, line 14 of alias1.c is skipped.
Comment 9 Richard Biener 2007-06-20 15:12:52 UTC
Confirmed.

struct barstruct { char const* some_string; };

void changethepointer(struct barstruct**);

void baz()
{
        struct barstruct bar1;
        struct barstruct* barptr = &bar1;
        changethepointer(&barptr);
        barptr->some_string = "Everything's OK!";
}


We end up with

baz ()
{
  struct barstruct * barptr;
  struct barstruct bar1;
  struct barstruct * barptr.0;

<bb 2>:
  #   barptr_2 = V_MUST_DEF <barptr_1>;
  barptr = &bar1;
  #   barptr_6 = V_MAY_DEF <barptr_2>;
  #   SFT.1_7 = V_MAY_DEF <SFT.1_4>;
  #   NONLOCAL.7_8 = V_MAY_DEF <NONLOCAL.7_5>;
  changethepointer (&barptr);
  #   VUSE <barptr_6>;
  barptr.0_3 = barptr;
  #   SFT.1_9 = V_MAY_DEF <SFT.1_7>;
  barptr.0_3->some_string = &"Everything\'s OK!"[0];
  return;

}

because of a wrong points-to set again:

barptr.0_3 = { bar1 }

The constraints are

Constraints: 

ANYTHING = &ANYTHING
READONLY = &ANYTHING
INTEGER = &ANYTHING
ESCAPED_VARS = *ESCAPED_VARS
NONLOCAL.7 = ESCAPED_VARS
ESCAPED_VARS = &NONLOCAL.7
ESCAPED_VARS = &NONLOCAL.7
barptr = &bar1
ESCAPED_VARS = &bar1
ESCAPED_VARS = &barptr
barptr.0_3 = barptr

where ESCAPED_VARS = &barptr looks wrong?  I'd say it needs to be
ESCAPED_VARS = barptr instead.

Danny?
Comment 10 Richard Biener 2007-06-20 15:16:36 UTC
trunk has the same problem, but different constraints:

Constraints:

ANYTHING = &ANYTHING
READONLY = &ANYTHING
INTEGER = &ANYTHING
barptr = &bar1
barptr.0_1 = barptr

Points-to sets

NULL = { }
ANYTHING = { ANYTHING }
READONLY = { ANYTHING }
INTEGER = { ANYTHING }
barptr = { bar1 }
bar1 = { }
barptr.0_1 = same as barptr
Comment 11 John E. / TDM 2007-06-20 15:35:58 UTC
Not sure if this is relevant or just GCC working differently on my target system, but I don't encounter the bug using -fstrict-aliasing, or in fact using individually the entire set of options under -O and -O2 on http://gcc.gnu.org/onlinedocs/gcc-4.2.0/gcc/Optimize-Options.html -- only when I specifically use the option "-O2".

i.e. "gcc -fstrict-aliasing -c alias1.c" DOESN'T, for me, cause the bug
but  "gcc -O2 -c alias1.c" DOES.
Comment 12 Daniel Berlin 2007-06-20 20:03:51 UTC
Subject: Re:  [4.2 Regression] -fstrict-aliasing causes skipped code

On 20 Jun 2007 15:12:53 -0000, rguenth at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #9 from rguenth at gcc dot gnu dot org  2007-06-20 15:12 -------
> Confirmed.
>
> struct barstruct { char const* some_string; };
>
> void changethepointer(struct barstruct**);
>
> void baz()
> {
>         struct barstruct bar1;
>         struct barstruct* barptr = &bar1;
>         changethepointer(&barptr);
>         barptr->some_string = "Everything's OK!";
> }
>
>
> We end up with
>
> baz ()
> {
>   struct barstruct * barptr;
>   struct barstruct bar1;
>   struct barstruct * barptr.0;
>
> <bb 2>:
>   #   barptr_2 = V_MUST_DEF <barptr_1>;
>   barptr = &bar1;
>   #   barptr_6 = V_MAY_DEF <barptr_2>;
>   #   SFT.1_7 = V_MAY_DEF <SFT.1_4>;
>   #   NONLOCAL.7_8 = V_MAY_DEF <NONLOCAL.7_5>;
>   changethepointer (&barptr);
>   #   VUSE <barptr_6>;
>   barptr.0_3 = barptr;
>   #   SFT.1_9 = V_MAY_DEF <SFT.1_7>;
>   barptr.0_3->some_string = &"Everything\'s OK!"[0];
>   return;
>
> }
>
> because of a wrong points-to set again:

Why do you believe this is wrong?
It looks exactly right

>
> barptr.0_3 = { bar1 }
>
> The constraints are
>
> Constraints:
>
> ANYTHING = &ANYTHING
> READONLY = &ANYTHING
> INTEGER = &ANYTHING
> ESCAPED_VARS = *ESCAPED_VARS
> NONLOCAL.7 = ESCAPED_VARS
> ESCAPED_VARS = &NONLOCAL.7
> ESCAPED_VARS = &NONLOCAL.7
> barptr = &bar1
> ESCAPED_VARS = &bar1
> ESCAPED_VARS = &barptr
> barptr.0_3 = barptr
>
> where ESCAPED_VARS = &barptr looks wrong?  I'd say it needs to be
> ESCAPED_VARS = barptr instead.

No, it's right on that part.  It says ESCAPED_VARS points to barptr.
ESCAPED_VARS = *ESCAPED_VARS will take care of making it point what
barptr points to.

What is missing here is that we should be doing barptr = ESCAPED_VARS
when we see &barptr passed to the call.
Comment 13 Daniel Berlin 2007-06-20 20:09:32 UTC
(In reply to comment #10)
> trunk has the same problem, but different constraints:
> 
> Constraints:
> 
> ANYTHING = &ANYTHING
> READONLY = &ANYTHING
> INTEGER = &ANYTHING
> barptr = &bar1
> barptr.0_1 = barptr
> 
> Points-to sets
> 
> NULL = { }
> ANYTHING = { ANYTHING }
> READONLY = { ANYTHING }
> INTEGER = { ANYTHING }
> barptr = { bar1 }
> bar1 = { }
> barptr.0_1 = same as barptr
> 


Same thing, when we see call(&barptr), we should say barptr = ANYTHING.
.

Comment 14 Daniel Berlin 2007-07-04 14:16:49 UTC
Subject: Re:  [4.2/4.3 Regression] -fstrict-aliasing causes skipped code

On 4 Jul 2007 03:29:25 -0000, mmitchel at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> --

Just as an update:
I have been working with richi (I code, he tests :P) diligently on a
patch for mainline, and have one that fixes the dealii regression (and
thus, should fix this as well).
Comment 15 Giovanni Bajo 2007-07-16 13:39:19 UTC
ping: anything that can be done for 4.2.1? This is a really serious regression.
Comment 16 Daniel Berlin 2007-07-16 13:58:40 UTC
Subject: Re:  [4.2/4.3 Regression] -fstrict-aliasing causes skipped code

I've attached a patch you should be able to quickly backport to 4.2.1.
 I'm still testing it against mainline right now.


On 16 Jul 2007 13:39:20 -0000, giovannibajo at libero dot it
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #15 from giovannibajo at libero dot it  2007-07-16 13:39 -------
> ping: anything that can be done for 4.2.1? This is a really serious regression.
>
Comment 17 Daniel Berlin 2007-07-16 13:58:40 UTC
Created attachment 13923 [details]
32328.diff
Comment 18 Richard Biener 2007-07-16 14:27:01 UTC
Created attachment 13924 [details]
patch for 4.2

I'll bootstrap/regtest it.
Comment 19 Richard Biener 2007-07-16 15:51:43 UTC
The backported patch causes 

FAIL: gcc.c-torture/execute/pr28778.c execution,  -O2 
FAIL: gcc.c-torture/execute/pr28778.c execution,  -Os 

DCE removes

-
-<L1>:;
-  list[3] = 42;

we have one added constraint:

 blist.0_7 = blist_1
+*blist.0_7 = &ANYTHING
 ESCAPED_VARS = blist.0_7

IL differences after alias1 are

@@ -251,12 +247,11 @@
   list[3] = 42;
   blist_6 = &list;
 
-  # list_8 = PHI <list_4(3), list_5(4)>;
   # blist_1 = PHI <blist_3(3), blist_6(4)>;
 <L2>:;
   blist.0_7 = (const GLint *) blist_1;
-  #   list_10 = V_MAY_DEF <list_8>;
-  #   NONLOCAL.44_11 = V_MAY_DEF <NONLOCAL.44_9>;
+  #   NONLOCAL.44_10 = V_MAY_DEF <NONLOCAL.44_8>;
+  #   SMT.45_11 = V_MAY_DEF <SMT.45_9>;
   aglChoosePixelFormat (blist.0_7);
   return;
 
we also have

+NULL = &ANYTHING

but this doesn't result in IL differences.
Comment 20 Daniel Berlin 2007-07-16 22:29:29 UTC
Subject: Re:  [4.2/4.3 Regression] -fstrict-aliasing causes skipped code

Oh, for 4.2 you need to add make_constraint_to_escaped_var

On 16 Jul 2007 15:51:44 -0000, rguenth at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #19 from rguenth at gcc dot gnu dot org  2007-07-16 15:51 -------
> The backported patch causes
>
> FAIL: gcc.c-torture/execute/pr28778.c execution,  -O2
> FAIL: gcc.c-torture/execute/pr28778.c execution,  -Os
>
> DCE removes
>
> -
> -<L1>:;
> -  list[3] = 42;
>
> we have one added constraint:
>
>  blist.0_7 = blist_1
> +*blist.0_7 = &ANYTHING
>  ESCAPED_VARS = blist.0_7
>
> IL differences after alias1 are
>
> @@ -251,12 +247,11 @@
>    list[3] = 42;
>    blist_6 = &list;
>
> -  # list_8 = PHI <list_4(3), list_5(4)>;
>    # blist_1 = PHI <blist_3(3), blist_6(4)>;
>  <L2>:;
>    blist.0_7 = (const GLint *) blist_1;
> -  #   list_10 = V_MAY_DEF <list_8>;
> -  #   NONLOCAL.44_11 = V_MAY_DEF <NONLOCAL.44_9>;
> +  #   NONLOCAL.44_10 = V_MAY_DEF <NONLOCAL.44_8>;
> +  #   SMT.45_11 = V_MAY_DEF <SMT.45_9>;
>    aglChoosePixelFormat (blist.0_7);
>    return;

This is something that was fixed on 4.3 by proper SMT pruning.
What is happening is it decides to prune the list access even though
it's illegal to do so.
Comment 21 Daniel Berlin 2007-08-19 23:23:54 UTC
Subject: Bug 32328

Author: dberlin
Date: Sun Aug 19 23:23:29 2007
New Revision: 127629

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=127629
Log:
2007-08-19  Daniel Berlin  <dberlin@dberlin.org>

	Fix PR 32772
	Fix PR 32716
	Fix PR 32328
	Fix PR 32303

	* tree-flow.h (struct stmt_ann_d): Remove makes_clobbering_call.
	* tree-ssa-alias.c (init_transitive_clobber_worklist): Add
	on_worklist argument and avoid adding things to worklist multiple
	times.
	(add_to_worklist): Ditto.
	(mark_aliases_call_clobbered): Mark entire structure clobbered if
	single SFT is clobbered.
	(set_initial_properties): Ditto.
	(compute_call_clobbered): Update for changes to function
	arguments.
	(create_overlap_variables_for): Always create SFT for offset 0.
	(create_structure_vars): Handle PHI's, since we are in SSA form at
	this point.
	* tree-ssa-loop-ivopts.c (get_ref_tag): Don't return subvars.
	* tree-ssa-operands.c (access_can_touch_variable): Don't handle
	TARGET_MEM_REF.
	(add_vars_for_offset): Figure out aliases from access + points-to.
	(add_virtual_operand): Use add_vars-for_offset.
	(get_tmr_operands): Update for NMT changes, rewrite to be correct.
	(add_call_clobber_ops): Remove makes_clobbering_call set.
	(get_expr_operands): Always pass through the INDIRECT_REF
	reference.
	* tree-ssa-structalias.c (struct constraint_graph): Remove
	variables member.
	Add pe, pe_rep, pointer_label, loc_label, pointed_by, points_to,
	address_taken, pt_used, number_incoming.
	(FIRST_ADDR_NODE): Removed.
	(merge_graph_nodes): Remove broken code for the moment.
	(init_graph): New function.
	(build_pred_graph): Remove code to init_graph.
	Add location equivalence support.
	(struct scc_info): Rename roots to deleted.
	(scc_visit): Ditto.
	(init_scc_info): Ditto
	(init_topo_info): Use graph->size.
	(compute_topo_order): Ditto.
	(do_da_constraint): Removed.
	(do_sd_constraint): Remove calls to find().
	set_union_with_increment should always get 0 as last arg here.
	(do_complex_constraint): Replace do_da_constraint with assert.
	Stop calling find.
	(struct equiv_class_label): New.
	(pointer_equiv_class_table): Ditto.
	(location_equiv_class_table): Ditto.
	(equiv_class_label_hash): Ditto.
	(equiv_class_label_eq): Ditto
	(equiv_class_lookup): Ditto.
	(equiv_class_ladd): Ditto.
	(pointer_equiv_class): Ditto.
	(location_equiv_class): Ditto.
	(condense_visit): Rename and rewrite from label_visit to do only
	SCC related stuff for HU.
	(label_visit): Do HU work for HU.
	(perform_var_substitution): Update to do HU and location
	equivalence.
	(free_var_substitution_info): Update to free HU and location
	equivalence structures.  */
	(find_equivalent_node): Update for pointer but not location
	equivalence.
	(unite_pointer_equivalences): New function.
	(move_complex_constraints): Rewrite to only do moving.
	(rewrite_constraints): Split out of move_complex_constraints.
	(solve_graph): Use graph->size.
	(process_constraint_1): Add from_call argument, use it.
	Split *a = &b into two constraints.
	(process_constraint): Use new process_constraint_1.
	(get_constraint_for_component_ref): Handle bitmaxsize == -1 case.
	(get_constraint_for): Handle non-pointer integers properly.
	Remove code that used to handle structures.
	(handle_ptr_arith): Fix a few bugs in pointer arithmetic handling
	with unknown addends.
	(handle_rhs_call): New function.
	(find_func_aliases): Use handle_rhs_call.
	(set_uids_in_ptset): Add an assert.
	(set_used_smts): Fix bug in not considering unified vars.
	(compute_tbaa_pruning): Stop initing useless iteration_obstack.
	(compute_points_to_sets): Update for other function changes.
	(delete_points_to_sets): Ditto.
	(ipa_pta_execute): Ditto.
	(pass_ipa_pta): We need to update SSA after ipa_pta.
	


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-flow.h
    trunk/gcc/tree-ssa-alias.c
    trunk/gcc/tree-ssa-loop-ivopts.c
    trunk/gcc/tree-ssa-operands.c
    trunk/gcc/tree-ssa-structalias.c

Comment 22 Daniel Berlin 2007-08-20 01:54:33 UTC
All should be fixed
Comment 23 Richard Biener 2007-08-22 15:52:29 UTC
But not with 4.2.
Comment 24 Richard Biener 2007-08-23 12:42:17 UTC
Subject: Bug 32328

Author: rguenth
Date: Thu Aug 23 12:41:59 2007
New Revision: 127736

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=127736
Log:
2007-08-23  Richard Guenther  <rguenther@suse.de>

        PR tree-optimization/32328
        * testsuite/gcc.dg/pr32328.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.dg/pr32328.c
Modified:
    trunk/gcc/testsuite/ChangeLog

Comment 25 Giovanni Bajo 2007-09-05 06:47:17 UTC
Daniel, can we backport this patch to 4.2, please? It's a P1 regression!
Comment 26 Richard Biener 2007-09-05 08:29:49 UTC
Not really.
Comment 27 Daniel Berlin 2007-09-05 11:51:24 UTC
Subject: Re:  [4.2 Regression] -fstrict-aliasing causes skipped code

On 5 Sep 2007 06:47:19 -0000, giovannibajo at libero dot it
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #25 from giovannibajo at libero dot it  2007-09-05 06:47 -------
> Daniel, can we backport this patch to 4.2, please? It's a P1 regression!
>

Not really.
We've tried before, and it causes other regressions in 4.2 due to
missing infrastructure.
Comment 28 Mark Mitchell 2007-10-09 19:20:29 UTC
Change target milestone to 4.2.3, as 4.2.2 has been released.
Comment 29 Joseph S. Myers 2008-02-01 16:54:22 UTC
4.2.3 is being released now, changing milestones of open bugs to 4.2.4.
Comment 30 John E. / TDM 2008-02-22 18:14:53 UTC
Are there sufficient motivation and resources to fix this bug in 4.2? Since 4.3 is soon-to-be, an update on this would be sincerely appreciated -- even if it's just forgetting about 4.2 and resolving as FIXED.
Comment 31 Joseph S. Myers 2008-05-19 20:23:18 UTC
4.2.4 is being released, changing milestones to 4.2.5.
Comment 32 Joseph S. Myers 2009-03-30 22:07:16 UTC
Closing 4.2 branch, fixed in 4.3.