Bug 26757 - C++ front-end producing two DECLs with the same UID
Summary: C++ front-end producing two DECLs with the same UID
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.1.0
: P3 normal
Target Milestone: 4.1.2
Assignee: Not yet assigned to anyone
URL:
Keywords: ice-on-valid-code, monitored
: 27056 27143 28841 (view as bug list)
Depends on:
Blocks: 20357 27143 28677
  Show dependency treegraph
 
Reported: 2006-03-19 13:02 UTC by Debian GCC Maintainers
Modified: 2006-08-25 04:31 UTC (History)
14 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2006-03-26 16:17:45


Attachments
preprocessed source (134.61 KB, application/x-bzip)
2006-03-19 13:02 UTC, Debian GCC Maintainers
Details
Possible patch (1.66 KB, patch)
2006-04-26 20:38 UTC, Andrew Macleod
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Debian GCC Maintainers 2006-03-19 13:02:11 UTC
[forwarded from http://bugs.debian.org/356569]

works with 4.0.3, 4.1.0 -O0, 4.1.0 -Ox segfaults.

g++ -O3 -c 3ddeskd.ii
3ddeskd.cpp: In function 'void draw_digit(int, float, float)':
3ddeskd.cpp:634: internal compiler error: Segmentation fault
Please submit a full bug report, with preprocessed source if appropriate.
See <URL:http://gcc.gnu.org/bugs.html> for instructions.
Comment 1 Debian GCC Maintainers 2006-03-19 13:02:34 UTC
Created attachment 11069 [details]
preprocessed source
Comment 2 Andrew Pinski 2006-03-19 16:13:43 UTC
Reducing.
Comment 3 Andrew Pinski 2006-03-19 20:36:01 UTC
DCE is removing some statements:
Marking useful stmt: alarm (D.3630_3);

Marking useful stmt: return;


Processing worklist:
processing: return;

processing: alarm (D.3630_3);

processing: D.3630_3 = (unsigned int) D.3629_2;


Eliminating unnecessary statements:
Deleting : cfg.20_1 = cfg;

Deleting : D.3629_2 = cfg.20_1->autoacquire;

Removed 2 of 5 statements (40%)
Removed 0 of 0 PHI nodes (0%)
void acquire_desktop(int) (rv)
{
  unsigned int D.3630;
  int D.3629;
  struct Config * cfg.20;

<bb 0>:
  D.3630_3 = (unsigned int) D.3629_2;
  #   cfg_4 = V_MAY_DEF <cfg_2>;
  alarm (D.3630_3);
  return;

}

I don't understand why  (this is using a reduced version of the source) because it is obvious it is used.
Comment 4 Andrew Pinski 2006-03-20 07:57:46 UTC
I could not reduce this to a good point (at least today).
Comment 5 Ferdinand 2006-03-26 02:19:25 UTC
Here's a reduced version that gives me an ICE on FC5's gcc 4.1.0 with:
g++ -O -c 3ddeskd.cpp

=====================================

class Config {
public:

    int texture_size;
    int bg_size;
    int autoacquire;

};

class Face {
public:
    
    void load_texture_data (int texture_size) {

        extern Config *cfg;

        cfg->bg_size++;
    }

};


class FaceSet {
private:

    Face faces;

public:

    void load_texture_data (int size) { 
        faces.load_texture_data(size);
    };

};


Config *cfg;
FaceSet *face_set;

void load_background_image(void) 
{
    cfg->bg_size++;
    
    extern Config *cfg;

    cfg->autoacquire++;

}


void 
humbug ()
{
    if (cfg->autoacquire)
    face_set->load_texture_data(cfg->texture_size);

}

=====================================
Comment 6 Ferdinand 2006-03-26 02:35:53 UTC
Sorry for the bugspam, but I noticed it could be reduced a bit more.

=====================================

struct Config {
    int bg_size;
    int autoacquire;

};

Config *cfg;

void load_background_image(void) 
{
    cfg->bg_size++;
    
    extern Config *cfg;

    cfg->autoacquire++;

}

void 
humbug ()
{
    if (cfg->autoacquire)
    cfg->bg_size++;

}

=====================================
Comment 7 Andrew Pinski 2006-03-26 15:45:49 UTC
I am starting to think this is a front-end issue, producing two decls for cfg.
Comment 8 Andrew Pinski 2006-03-26 16:17:45 UTC
Confirmed, this only happens with the C++ Front-end, why I don't know why.
Anyways here is a testcase which should go into the testsuite for also testing correct code:
#include <stdlib.h>
typedef struct Config {
    int bg_size;
    int autoacquire;
}Config;

Config *cfg;

void load_background_image(void)
{
    cfg->bg_size++;

    extern Config *cfg;

    cfg->autoacquire++;

}

void
humbug ()
{
    if (cfg->autoacquire)
      cfg->bg_size++;
}


int main(void)
{
  Config a;
  cfg = &a;
  load_background_image();
  humbug();
  if (cfg->bg_size !=  2)
    abort();
  if (cfg->autoacquire != 1)
    abort();
  return 0;
}

----
And shows that it is only the C++ front-end also.
Comment 9 Volker Reichelt 2006-04-06 16:13:12 UTC
*** Bug 27056 has been marked as a duplicate of this bug. ***
Comment 10 Andrew Macleod 2006-04-06 21:05:25 UTC
using the program from comment #5, I get IL for load_background_image that looks like:

void load_background_image() ()
{
  int D.1767;
  int D.1766;
  struct Config * cfg.2;
  int D.1764;
  int D.1763;
  struct Config * cfg.1;

<bb 0>:
  cfg.1 = cfg;
  D.1763 = cfg.1->bg_size;
  D.1764 = D.1763 + 1;
  cfg.1->bg_size = D.1764;
  cfg.2 = cfg;
  D.1766 = cfg.2->autoacquire;
  D.1767 = D.1766 + 1;
  cfg.2->autoacquire = D.1767;
  return;

}

THe two assignments of cfg into temps,  (cfg.1 and cfg.2) are using DIFFERENT cfg variables (ie, different addresses) on the RHS with the same UID.  very bad.  either they should be the same var_decl (which I presume is what is intended), or they should have different UIDs.  

This patchlet provides an assert the triggers when it happens:

Index: tree-dfa.c
===================================================================
*** tree-dfa.c  (revision 112248)
--- tree-dfa.c  (working copy)
*************** referenced_var_insert (unsigned int uid,
*** 610,615 ****
--- 610,619 ----
    h->uid = uid;
    h->to = to;
    loc = htab_find_slot_with_hash (referenced_vars, h, uid, INSERT);
+   /* This assert can only trigger is a variable with the same UID has been
+      inserted already, but has a different pointer value. ie, we have 2
+      different variables with the same UID.  Bug 26757.  */
+   gcc_assert ((*(struct int_tree_map **)loc) == NULL);
    *(struct int_tree_map **)  loc = h;
  }


what happens is that the referenced_var list hashes based on the UID of a variable. referenced_var_insert is only called when a new variable is discovered, so it presumes that the hash slot is already empty.

add_referenced_var calls referenced_var_insert when it sees a new variable, and it determines a variable is new by hashing on the address of the variable in the walk_state->vars_found table. 

since there are 2 different 'cfg' vars, referenced_var_insert overwrote the first 'cfg''s address when it looked up the UID. 

WHen we go out of ssa, delete_tree_ssa removes all the var annotations of referenced vars. The original 'cfg' no longer occurs in the referenced var list, so it is not cleared.

Along comes the next function, and it is using the original 'cfg', and voila, it still has a var_annotation, complete with current_def and lots of fun things set.


I havent tracked down where the cfg variable is created, but Im willing to bet a C++ front end person knows right off the bat by looking at the function.
Comment 11 Andrew Pinski 2006-04-13 16:37:59 UTC
*** Bug 27143 has been marked as a duplicate of this bug. ***
Comment 12 Andrew Pinski 2006-04-13 16:42:43 UTC
This is a latent bug as show by PR 16792 and PR 20357.  Both show the same problem of two DECLs pointing to the same variable and having the same UID.
Comment 13 Jakub Jelinek 2006-04-20 10:27:11 UTC
This has been introduced in
http://gcc.gnu.org/ml/gcc-patches/2004-06/msg01594.html
Will try to revert the name-lookup.c part and see what exactly breaks and if
we couldn't tell about this to cgraph rather than doing ugly hacks.
Comment 14 Jakub Jelinek 2006-04-20 12:04:37 UTC
The reversal of the DECL_UID (x) = DECL_UID (t); assignment in name-lookup.c
causes
FAIL: g++.dg/opt/inline2.C (test for excess errors)
as well as
// { dg-do link }
// { dg-options "-O1" }

static int g;

void f()
{
  extern int g;
  g++;
}

int main () {
  f ();
}
regression.
For some of the duplicate decls in different binding levels I'm pretty sure we
could just duplicate_decls instead of keeping two decls around, but as the comment explains e.g. for
static int g (int x, int y = 8);

void f ()
{
  int g (int x, int y = 12);
  g (6);
}

int g (int x, int y)
{
  return x + y;
}

int main ()
{
  f ();
  g (6);
}
we need two decls, at least throughout the frontend.  On the other side, the two
decls are really supposed to share all cgraph {,varpool} node properties, except
the decl itself (and uid), so I wonder if we couldn't have indirect nodes in cgraph_hash and cgraph_varpool_hash or something similar for this purpose.
Comment 15 Andrew Pinski 2006-04-20 16:19:57 UTC
(In reply to comment #14)
> The reversal of the DECL_UID (x) = DECL_UID (t); assignment in name-lookup.c
> causes

Yes because they are the same decl, if the C++ only uses one decl instead of creating a seperate one, it would just work.

Zack's patch did not cause but did expose one issue.  Again see PR 20357 for why this was not caused by any of the patches mentioned and why this is a latent bug from before any of the patches mentioned and the correct method is to create one decl with one decl UID and forget about a seperate decl with the copy.


Fortran has the same issue and I was trying to fix some of those issue but that work got stalled by me joining Sony and also not having time to work on any of GCC in the last days of college.
Comment 16 Mark Mitchell 2006-04-23 18:14:02 UTC
The correct fix for this issue is for the C++ front-end to save and restore information about a declaration as scopes are entered and exited.

For example, given:

  extern void f(int);
  void g() {
    extern void f(int = 7);
    f();
  }
  void h() { 
    extern void f(int = 3); 
    f(); 
  }

we should use a single FUNCTION_DECL (and a single DECL_UID), add the default argument when the declaration is encountered in g, and then remove the default argument when we exit that scope.  This example shows why you can't just keep calling duplicate_decls; you can actually change the values of the default arguments in different scopes, and, in scopes with no default argument declared, the user must supply all arguments.

Default arguments are only part of the problem; the same issue applies to things like:

  extern int a[];
  void f() { 
    extern int a[10];
  }

It will not be particularly entertaining to fix this in the C++ front-end, though it is indeed something that should be done.  It's probably doable for 4.2, but I'd think it to be a very risk change for 4.1. 

Is there a work-around in the middle end that can be applied on the 4.1 branch?
Comment 17 Andrew Macleod 2006-04-26 20:38:37 UTC
Created attachment 11336 [details]
Possible patch

Maybe.  there is a work around I have an initial patch for.  It re-covers up the problem I think :-)

The reason it is triggering in the ssa portion is because of the Schizophrenic way we find referenced variables.

find_referenced_vars() uses a hash table of variable addresses (vars_found in the walk_state struct) to determine whether a variable has been seen or not. If it hasn't been seen, it eventually calls add_referenced_var().

add_referenced_var() assumes if it is called that the variable truly hasn't been added, and it creates an entry in the referenced_var hash table, which is hashed by DECL_UID rather than by address.

What happens in this test case is with two addresses with the same DECL_UID, add_referenced_var() gets back the same hash slot for the second variable, and simply overwrites the hash slot info with the second variable's info (so the first one's info is gone). 

When SSA is deleted, the second variables annotation is deleted because it is in the referenced_var hash table, but the first one's is never seen. 

Next function comes along, and boom, the first var already has ann annotation and hellfire begins to spread.

The fix that I have removes the first hashtable. Diego thinks it may have been there for historical reasons, but there is certainly no need for it now.  Instead of maintaining and checking thats vars_found table, we can simple check the referenced_var table to see if we have seen a given DECL_UID yet.  

This should be fixed anyway since it is wasteful, and as a bonus it appears to cover the problem back up again.  I ran this a couple of weeks ago on 4.1 but never got back to it.  Someone want to try it out and see if it causes any other problems?  It bootstrapped and caused no new testsuite regressions on x86-linux then.  I'll re-run it tonight.
Comment 18 Mark Mitchell 2006-04-30 17:56:19 UTC
Andrew --

Thanks for investigating this, and for attempting to tolerate this bit of weirdness from the front end.

-- Mark
Comment 19 Martin Michlmayr 2006-05-11 11:03:27 UTC
(In reply to comment #17)
> Created an attachment (id=11336) [edit]
> Possible patch
> 
> Maybe.  there is a work around I have an initial patch for.  It re-covers up
> the problem I think :-)

Andrew, what's the status of this patch?
Comment 20 Mark Mitchell 2006-05-16 14:57:00 UTC
Andrew --

Is there any news on this patch?  

Is there anyone else available to review/test Andrew's patch?

Thanks,

-- Mark
Comment 21 Andrew Macleod 2006-05-16 15:16:47 UTC
Actually, no, no new news :-). I had forgotten about this one.  I'll run it again today and check it in if no problems show up. 

You want it against 4.1 and mainline?  or do you want to try it against mainline for a few days first?  Risk is minor, but always present. :-P

Andrew
Comment 22 Andrew Macleod 2006-05-16 20:48:00 UTC
Bootstraps on the 4.1 branch on i686-pc-linux-gnu and produces no new testsuite failures. It also allows the original testcase to compile.

The mainline version is now running.

Since it seems to be blocking other cases and there is some interest, I'm going to check it into 4.1, and then mainline later.  If any issues pop up, we'll deal with them then.

Andrew
Comment 23 Andrew Macleod 2006-05-16 20:51:42 UTC
Subject: Bug 26757

Author: amacleod
Date: Tue May 16 20:51:14 2006
New Revision: 113829

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=113829
Log:
Remove redundant hash table lookup when finding referenced vars.

2006-05-16  Andrew MacLeod  <amacleod@redhat.com>

	PR c++/26757
	* tree-dfa.c (struct walk_state): Remove.
	(add_referenced_var): Change Parameters.
	(find_referenced_vars): Done use a walk_state.
	(find_vars_r): Unused parameter and change parms to add_referenced_var.
	(referenced_var_insert): Assert same UID has not been inserted.
	(add_referenced_var): Check if var exists via referenced_var table.
	(get_virtual_var): Call add_referenced_var with new parameter.


Modified:
    branches/gcc-4_1-branch/gcc/ChangeLog
    branches/gcc-4_1-branch/gcc/tree-dfa.c

Comment 24 Mark Mitchell 2006-05-17 20:38:11 UTC
Removing the 4.1 marker, as this is now fixed on the 4.1 branch.
Comment 25 Andrew Macleod 2006-05-23 14:07:29 UTC
Subject: Bug 26757

Author: amacleod
Date: Tue May 23 14:07:21 2006
New Revision: 114018

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=114018
Log:

2006-05-23  Andrew MacLeod  <amacleod@redhat.com>
	
	PR c++/26757
	* tree-ssa-loop-im.c (determine_invariantness_stmt): Use 
	add_referenced_var instead of add_referenced_tmp_var.
	* tree-complex.c (create_one_component_var): Use add_referenced_var.
	* tree-ssa-loop-manip.c (create_iv, tree_unroll_loop): Use
	add_referenced_var.
	* tree-tailcall.c (adjust_accumulator_values, adjust_return_value,
	tree_optimize_tail_calls_1): Use add_referenced_var.
	* tree-ssa-loop-ivopts.c (create_new_iv): Use add_referenced_var.
	* tree-ssa-alias.c (create_memory_tag, create_global_var, create_sft):
	Use add_referenced_var.
	* tree-if-conv.c (ifc_temp_var): Use add_referenced_var.
	* gimplify.c (force_gimple_operand): Use add_referenced_var.
	* tree-ssa-phiopt.c (conditional_replacement, abs_replacement):
	Use add_referenced_var.
	* tree-dfa.c (struct walk_state): Remove.
	(find_referenced_vars): Remove walk state and vars_found hash table.
	(make_rename_temp): Use add_referenced_var.
	(find_vars_r): Pass less parameters to add_referenced_var.
	(referenced_var_p): New.  Is var in referenced_var hash table.
	(referenced_var_insert): Assert var isn't already in hash table.
	(add_referenced_var): Don't need walk_state parameter.  Add var if it
	isn't already in the hash table.
	(add_referenced_tmp_var): Remove.
	(find_new_referenced_vars_1): Use add_referenced_var.
	* tree-ssa-pre.c (create_expression_by_pieces, 
	insert_into_preds_of_block, insert_extra_phis, realify_fake_stores):
	Use add_referenced_var.
	* tree-vect-patterns.c (vect_pattern_recog_1): Use add_referenced_var.
	* lambda-code.c (lbv_to_gcc_expression, lle_to_gcc_expression,
	lambda_loopnest_to_gcc_loopnest, perfect_nestify): Use 
	add_referenced_var.
	* tree-vect-transform.c (vect_create_addr_base_for_vector_ref,
	vect_create_data_ref_ptr, vect_create_destination_var,
	vect_init_vector, vect_build_loop_niters, 
	vect_generate_tmps_on_preheader, vect_update_ivs_after_vectorizer,
	vect_gen_niters_for_prolog_loop, vect_create_cond_for_align_checks):
	Use add_referenced_var.
	* tree-outof-ssa.c (create_temp): Use add_referenced_var.
	* tree-flow.h (add_referenced_tmp_var): Remove prototype
	(add_referenced_var): Add prototype.
	* tree-ssa-structalias.c (get_constraint_for, 
	intra_create_variable_infos): Use add_referenced_var.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/gimplify.c
    trunk/gcc/lambda-code.c
    trunk/gcc/tree-complex.c
    trunk/gcc/tree-dfa.c
    trunk/gcc/tree-flow.h
    trunk/gcc/tree-if-conv.c
    trunk/gcc/tree-outof-ssa.c
    trunk/gcc/tree-ssa-alias.c
    trunk/gcc/tree-ssa-loop-im.c
    trunk/gcc/tree-ssa-loop-ivopts.c
    trunk/gcc/tree-ssa-loop-manip.c
    trunk/gcc/tree-ssa-phiopt.c
    trunk/gcc/tree-ssa-pre.c
    trunk/gcc/tree-ssa-structalias.c
    trunk/gcc/tree-tailcall.c
    trunk/gcc/tree-vect-patterns.c
    trunk/gcc/tree-vect-transform.c

Comment 26 Alexandre Oliva 2006-05-28 05:11:50 UTC
Looks like it's fixed.
Comment 27 Andrew Pinski 2006-05-28 05:13:58 UTC
(In reply to comment #26)
> Looks like it's fixed.

No, just worked around.
Comment 28 Jakub Jelinek 2006-06-13 09:21:43 UTC
Subject: Bug 26757

Author: jakub
Date: Tue Jun 13 09:21:30 2006
New Revision: 114607

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=114607
Log:
	PR middle-end/27793
	* cp-tree.h (cxx_int_tree_map): New struct.
	(struct language_function): Add extern_decl_map field.
	* name-lookup.c (pushdecl_maybe_friend): Add x -> t mapping
	to cp_function_chain->extern_decl_map hash table instead of
	copying over DECL_UID.
	* cp-gimplify.c (cxx_int_tree_map_eq, cxx_int_tree_map_hash): New
	functions.
	(cp_genericize_r): Remap DECL_EXTERN local decls using
	cp_function_chain->extern_decl_map hash table.
	* decl.c (finish_function): Clear extern_decl_map.

	PR c++/26757
	PR c++/27894
	* g++.dg/tree-ssa/pr26757.C: New test.
	* g++.dg/tree-ssa/pr27894.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/tree-ssa/pr26757.C
    trunk/gcc/testsuite/g++.dg/tree-ssa/pr27894.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/cp-gimplify.c
    trunk/gcc/cp/cp-tree.h
    trunk/gcc/cp/decl.c
    trunk/gcc/cp/name-lookup.c
    trunk/gcc/testsuite/ChangeLog

Comment 29 Andrew Pinski 2006-08-10 20:15:51 UTC
This has now been fixed in 4.2.0.
Comment 30 Jakub Jelinek 2006-08-13 19:26:52 UTC
Subject: Bug 26757

Author: jakub
Date: Sun Aug 13 20:26:38 2006
New Revision: 116115

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=116115
Log:
2006-08-13  Andrew MacLeod  <amacleod@redhat.com>

	PR middle-end/27793
	* tree-dfa.c (add_referenced_var): Assert DECL_UID is unique for
	different decls.

2006-08-13  Jakub Jelinek  <jakub@redhat.com>

	PR c++/28677
	PR middle-end/27793
	* cp-tree.h (cxx_int_tree_map): New struct.
	(struct language_function): Add extern_decl_map field.
	* name-lookup.c (pushdecl_maybe_friend): Add x -> t mapping
	to cp_function_chain->extern_decl_map hash table instead of
	copying over DECL_UID.
	* cp-gimplify.c (cxx_int_tree_map_eq, cxx_int_tree_map_hash): New
	functions.
	(cp_genericize_r): Remap DECL_EXTERN local decls using
	cp_function_chain->extern_decl_map hash table.
	* decl.c (finish_function): Clear extern_decl_map.

	Revert:
	2006-06-06  Andrew MacLeod  <amacleod@redhat.com>
	PR middle-end/27793
	* tree-dfa.c (referenced_vars_dup_list): New.  List of duplicate 
	referenced_variables with matching DECL_UID's.
	(find_referenced_vars): Make sure duplicate list is empty to start.
	(add_referenced_var): Add var to duplicate list if required.
	* tree-ssa.c (delete_tree_ssa): Clear var_ann's on duplicates.
	* tree-flow.h (referenced_vars_dup_list): External declaration.

	PR c++/26757
	PR c++/27894
	* g++.dg/tree-ssa/pr26757.C: New test.
	* g++.dg/tree-ssa/pr27894.C: New test.

Added:
    branches/gcc-4_1-branch/gcc/testsuite/g++.dg/tree-ssa/pr26757.C
    branches/gcc-4_1-branch/gcc/testsuite/g++.dg/tree-ssa/pr27894.C
Modified:
    branches/gcc-4_1-branch/gcc/ChangeLog
    branches/gcc-4_1-branch/gcc/cp/ChangeLog
    branches/gcc-4_1-branch/gcc/cp/cp-gimplify.c
    branches/gcc-4_1-branch/gcc/cp/cp-tree.h
    branches/gcc-4_1-branch/gcc/cp/decl.c
    branches/gcc-4_1-branch/gcc/cp/name-lookup.c
    branches/gcc-4_1-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_1-branch/gcc/tree-dfa.c
    branches/gcc-4_1-branch/gcc/tree-flow.h
    branches/gcc-4_1-branch/gcc/tree-ssa.c

Comment 31 Andrew Pinski 2006-08-25 04:31:11 UTC
*** Bug 28841 has been marked as a duplicate of this bug. ***