Message ID | dcc20aec-ce49-4497-b329-4bab4a6a05dc@in.tum.de |
---|---|
State | New |
Headers | show |
Series | handle unwind tables that are embedded within unwinding code, [PR111731] | expand |
On Fri, Mar 15, 2024 at 11:31 AM Thomas Neumann <thomas.neumann@in.tum.de> wrote: > > Original bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111731 > Given that this is a regression, is this okay for gcc 13 and mainline? It does look straightforward but I hope Jason or Florian can provide the ACK. Thanks, Richard. > The unwinding mechanism registers both the code range and the unwind > table itself within a b-tree lookup structure. That data structure > assumes that is consists of non-overlappping intervals. This > becomes a problem if the unwinding table is embedded within the > code itself, as now the intervals do overlap. > > To fix this problem we now keep the unwind tables in a separate > b-tree, which prevents the overlap. > > libgcc/ChangeLog: > PR libgcc/111731 > * unwind-dw2-fde.c: Split unwind ranges if they contain the > unwind table. > --- > libgcc/unwind-dw2-fde.c | 37 +++++++++++++++++++++---------------- > 1 file changed, 21 insertions(+), 16 deletions(-) > > diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c > index 61a578d097e..9d503545677 100644 > --- a/libgcc/unwind-dw2-fde.c > +++ b/libgcc/unwind-dw2-fde.c > @@ -48,6 +48,7 @@ typedef __UINTPTR_TYPE__ uintptr_type; > #include "unwind-dw2-btree.h" > > static struct btree registered_frames; > +static struct btree registered_objects; > static bool in_shutdown; > > static void > @@ -58,6 +59,7 @@ release_registered_frames (void) > /* Release the b-tree and all frames. Frame releases that happen later are > * silently ignored */ > btree_destroy (®istered_frames); > + btree_destroy (®istered_objects); > in_shutdown = true; > } > > @@ -103,6 +105,21 @@ static __gthread_mutex_t object_mutex; > #endif > #endif > > +#ifdef ATOMIC_FDE_FAST_PATH > +// Register the pc range for a given object in the lookup structure. > +static void > +register_pc_range_for_object (uintptr_type begin, struct object *ob) > +{ > + // Register the object itself to know the base pointer on deregistration. > + btree_insert (®istered_objects, begin, 1, ob); > + > + // Register the frame in the b-tree > + uintptr_type range[2]; > + get_pc_range (ob, range); > + btree_insert (®istered_frames, range[0], range[1] - range[0], ob); > +} > +#endif > + > /* Called from crtbegin.o to register the unwind info for an object. */ > > void > @@ -124,13 +141,7 @@ __register_frame_info_bases (const void *begin, struct object *ob, > #endif > > #ifdef ATOMIC_FDE_FAST_PATH > - // Register the object itself to know the base pointer on deregistration. > - btree_insert (®istered_frames, (uintptr_type) begin, 1, ob); > - > - // Register the frame in the b-tree > - uintptr_type range[2]; > - get_pc_range (ob, range); > - btree_insert (®istered_frames, range[0], range[1] - range[0], ob); > + register_pc_range_for_object ((uintptr_type) begin, ob); > #else > init_object_mutex_once (); > __gthread_mutex_lock (&object_mutex); > @@ -178,13 +189,7 @@ __register_frame_info_table_bases (void *begin, struct object *ob, > ob->s.b.encoding = DW_EH_PE_omit; > > #ifdef ATOMIC_FDE_FAST_PATH > - // Register the object itself to know the base pointer on deregistration. > - btree_insert (®istered_frames, (uintptr_type) begin, 1, ob); > - > - // Register the frame in the b-tree > - uintptr_type range[2]; > - get_pc_range (ob, range); > - btree_insert (®istered_frames, range[0], range[1] - range[0], ob); > + register_pc_range_for_object ((uintptr_type) begin, ob); > #else > init_object_mutex_once (); > __gthread_mutex_lock (&object_mutex); > @@ -232,7 +237,7 @@ __deregister_frame_info_bases (const void *begin) > > #ifdef ATOMIC_FDE_FAST_PATH > // Find the originally registered object to get the base pointer. > - ob = btree_remove (®istered_frames, (uintptr_type) begin); > + ob = btree_remove (®istered_objects, (uintptr_type) begin); > > // Remove the corresponding PC range. > if (ob) > @@ -240,7 +245,7 @@ __deregister_frame_info_bases (const void *begin) > uintptr_type range[2]; > get_pc_range (ob, range); > if (range[0] != range[1]) > - btree_remove (®istered_frames, range[0]); > + btree_remove (®istered_frames, range[0]); > } > > // Deallocate the sort array if any. > -- > 2.43.0 >
On 3/15/24 4:29 AM, Thomas Neumann wrote: > Original bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111731 > Given that this is a regression, is this okay for gcc 13 and mainline? > > The unwinding mechanism registers both the code range and the unwind > table itself within a b-tree lookup structure. That data structure > assumes that is consists of non-overlappping intervals. This > becomes a problem if the unwinding table is embedded within the > code itself, as now the intervals do overlap. > > To fix this problem we now keep the unwind tables in a separate > b-tree, which prevents the overlap. > > libgcc/ChangeLog: > PR libgcc/111731 > * unwind-dw2-fde.c: Split unwind ranges if they contain the > unwind table. I'll go ahead and give the final OK here :-) Jeff
On 3/15/24 4:29 AM, Thomas Neumann wrote: > Original bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111731 > Given that this is a regression, is this okay for gcc 13 and mainline? > > The unwinding mechanism registers both the code range and the unwind > table itself within a b-tree lookup structure. That data structure > assumes that is consists of non-overlappping intervals. This > becomes a problem if the unwinding table is embedded within the > code itself, as now the intervals do overlap. > > To fix this problem we now keep the unwind tables in a separate > b-tree, which prevents the overlap. > > libgcc/ChangeLog: > PR libgcc/111731 > * unwind-dw2-fde.c: Split unwind ranges if they contain the > unwind table. And what I'd suggest is committing to the trunk now, then waiting a week or two before backporting to gcc-13. jeff
>> libgcc/ChangeLog: >> PR libgcc/111731 >> * unwind-dw2-fde.c: Split unwind ranges if they contain the >> unwind table. > And what I'd suggest is committing to the trunk now, then waiting a week > or two before backporting to gcc-13. I will do that, thanks for looking at the patch. Best Thomas
diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c index 61a578d097e..9d503545677 100644 --- a/libgcc/unwind-dw2-fde.c +++ b/libgcc/unwind-dw2-fde.c @@ -48,6 +48,7 @@ typedef __UINTPTR_TYPE__ uintptr_type; #include "unwind-dw2-btree.h" static struct btree registered_frames; +static struct btree registered_objects; static bool in_shutdown; static void @@ -58,6 +59,7 @@ release_registered_frames (void) /* Release the b-tree and all frames. Frame releases that happen later are * silently ignored */ btree_destroy (®istered_frames); + btree_destroy (®istered_objects); in_shutdown = true; } @@ -103,6 +105,21 @@ static __gthread_mutex_t object_mutex; #endif #endif +#ifdef ATOMIC_FDE_FAST_PATH +// Register the pc range for a given object in the lookup structure. +static void +register_pc_range_for_object (uintptr_type begin, struct object *ob) +{ + // Register the object itself to know the base pointer on deregistration. + btree_insert (®istered_objects, begin, 1, ob); + + // Register the frame in the b-tree + uintptr_type range[2]; + get_pc_range (ob, range); + btree_insert (®istered_frames, range[0], range[1] - range[0], ob); +} +#endif + /* Called from crtbegin.o to register the unwind info for an object. */ void @@ -124,13 +141,7 @@ __register_frame_info_bases (const void *begin, struct object *ob, #endif #ifdef ATOMIC_FDE_FAST_PATH - // Register the object itself to know the base pointer on deregistration. - btree_insert (®istered_frames, (uintptr_type) begin, 1, ob); - - // Register the frame in the b-tree - uintptr_type range[2]; - get_pc_range (ob, range); - btree_insert (®istered_frames, range[0], range[1] - range[0], ob); + register_pc_range_for_object ((uintptr_type) begin, ob); #else init_object_mutex_once (); __gthread_mutex_lock (&object_mutex); @@ -178,13 +189,7 @@ __register_frame_info_table_bases (void *begin, struct object *ob, ob->s.b.encoding = DW_EH_PE_omit; #ifdef ATOMIC_FDE_FAST_PATH - // Register the object itself to know the base pointer on deregistration. - btree_insert (®istered_frames, (uintptr_type) begin, 1, ob); - - // Register the frame in the b-tree - uintptr_type range[2]; - get_pc_range (ob, range); - btree_insert (®istered_frames, range[0], range[1] - range[0], ob); + register_pc_range_for_object ((uintptr_type) begin, ob); #else init_object_mutex_once (); __gthread_mutex_lock (&object_mutex); @@ -232,7 +237,7 @@ __deregister_frame_info_bases (const void *begin) #ifdef ATOMIC_FDE_FAST_PATH // Find the originally registered object to get the base pointer. - ob = btree_remove (®istered_frames, (uintptr_type) begin); + ob = btree_remove (®istered_objects, (uintptr_type) begin); // Remove the corresponding PC range. if (ob) @@ -240,7 +245,7 @@ __deregister_frame_info_bases (const void *begin) uintptr_type range[2]; get_pc_range (ob, range); if (range[0] != range[1]) - btree_remove (®istered_frames, range[0]); + btree_remove (®istered_frames, range[0]); } // Deallocate the sort array if any.