Patchwork Fix memory corruption in LTO

login
register
mail settings
Submitter Jan Hubicka
Date July 26, 2010, 7:24 p.m.
Message ID <20100726192415.GC20984@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/59935/
State New
Headers show

Comments

Jan Hubicka - July 26, 2010, 7:24 p.m.
Hi,
Andi's changes introduced heap allocated object pointed from lto_file_data, so
we now ICE on everything big enough to trigger garbage collector at WPA stage.
Andi: this solves the cgraph hash table ICE you sent me

Bootstrapping/regtesting x86_64-linux, OK?
Honza

	* lto-streamer.h (struct lto_file_decl_data): Mark resolutions with GTY((skip))
Diego Novillo - July 26, 2010, 8:14 p.m.
On 10-07-26 12:24 , Jan Hubicka wrote:
> Hi,
> Andi's changes introduced heap allocated object pointed from lto_file_data, so
> we now ICE on everything big enough to trigger garbage collector at WPA stage.
> Andi: this solves the cgraph hash table ICE you sent me
>
> Bootstrapping/regtesting x86_64-linux, OK?
> Honza
>
> 	* lto-streamer.h (struct lto_file_decl_data): Mark resolutions with GTY((skip))

Would it make more sense to change the allocation on the resolutions field?


Diego.
Jan Hubicka - July 26, 2010, 8:40 p.m.
> On 10-07-26 12:24 , Jan Hubicka wrote:
>> Hi,
>> Andi's changes introduced heap allocated object pointed from lto_file_data, so
>> we now ICE on everything big enough to trigger garbage collector at WPA stage.
>> Andi: this solves the cgraph hash table ICE you sent me
>>
>> Bootstrapping/regtesting x86_64-linux, OK?
>> Honza
>>
>> 	* lto-streamer.h (struct lto_file_decl_data): Mark resolutions with GTY((skip))
>
> Would it make more sense to change the allocation on the resolutions field?

Well, lto_file_decl_data has most of fields skipped anyway and I think it is
good idea as I don't like moving random stuff into garbage collector.  It is in
GGC because it points to global_decl_state that points to decls, but the
other streamer data are better to stay in explicitely allocated memory IMO.

Honza
>
>
> Diego.
Diego Novillo - July 26, 2010, 8:41 p.m.
On Mon, Jul 26, 2010 at 16:40, Jan Hubicka <hubicka@ucw.cz> wrote:

> Well, lto_file_decl_data has most of fields skipped anyway and I think it is
> good idea as I don't like moving random stuff into garbage collector.  It is in
> GGC because it points to global_decl_state that points to decls, but the
> other streamer data are better to stay in explicitely allocated memory IMO.

Ah, yes, you're right.  I had forgotten that.  Patch is OK.


Diego.
Andi Kleen - July 26, 2010, 11:43 p.m.
On 7/26/2010 9:24 PM, Jan Hubicka wrote:
> Hi,
> Andi's changes introduced heap allocated object pointed from lto_file_data, so
> we now ICE on everything big enough to trigger garbage collector at WPA stage.
> Andi: this solves the cgraph hash table ICE you sent me

Thanks for the fix!

-Andi
Richard Guenther - July 27, 2010, 8:30 a.m.
On Mon, 26 Jul 2010, Jan Hubicka wrote:

> Hi,
> Andi's changes introduced heap allocated object pointed from lto_file_data, so
> we now ICE on everything big enough to trigger garbage collector at WPA stage.
> Andi: this solves the cgraph hash table ICE you sent me
> 
> Bootstrapping/regtesting x86_64-linux, OK?

Ok.

Thanks,
Richard.

> Honza
> 
> 	* lto-streamer.h (struct lto_file_decl_data): Mark resolutions with GTY((skip))
> Index: lto-streamer.h
> ===================================================================
> --- lto-streamer.h	(revision 162539)
> +++ lto-streamer.h	(working copy)
> @@ -602,13 +602,13 @@
>    htab_t GTY((skip)) renaming_hash_table;
>  
>    /* Linked list used temporarily in reader */
>    struct lto_file_decl_data *next;
>  
>    /* Sub ID for merged objects. */
>    unsigned id;
>  
>    /* Symbol resolutions for this file */
> -  VEC(ld_plugin_symbol_resolution_t,heap) *resolutions;
> +  VEC(ld_plugin_symbol_resolution_t,heap) * GTY((skip)) resolutions;
>  };
>  
>  typedef struct lto_file_decl_data *lto_file_decl_data_ptr;
> 
>

Patch

Index: lto-streamer.h
===================================================================
--- lto-streamer.h	(revision 162539)
+++ lto-streamer.h	(working copy)
@@ -602,13 +602,13 @@ 
   htab_t GTY((skip)) renaming_hash_table;
 
   /* Linked list used temporarily in reader */
   struct lto_file_decl_data *next;
 
   /* Sub ID for merged objects. */
   unsigned id;
 
   /* Symbol resolutions for this file */
-  VEC(ld_plugin_symbol_resolution_t,heap) *resolutions;
+  VEC(ld_plugin_symbol_resolution_t,heap) * GTY((skip)) resolutions;
 };
 
 typedef struct lto_file_decl_data *lto_file_decl_data_ptr;