| Message ID | 20170914080147.969-1-derodat@adacore.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add comments to struct cgraph_thunk_info | expand |
On 09/14/2017 02:01 AM, Pierre-Marie de Rodat wrote: > Hello, > > This commit adds comments to fields in the cgraph_thunk_info structure > declaration from cgraph.h. They will hopefully answer questions that > people like myself can ask while discovering the thunk machinery. I > also made an assertion stricter in cgraph_node::create_thunk. > > I'm adding Nathan in copy as we discussed this thunk matter at this > year's Cauldron. :-) > > Bootsrapped and regtested on x86_64-linux. Ok to commit? Thank you in > advance! > > gcc/ > > * cgraph.h (cgraph_thunk_info): Add comments, reorder fields. > * cgraph.c (cgraph_node::create_thunk): Adjust comment, make > assert for VIRTUAL_* arguments stricter. The comment additions are fine. What's the rationale behind the ordering of the fields? In general we want the opposite order from what you did -- going from most strictly aligned to least strictly aligned minimizes the amount of unused padding. jeff
On 09/14/2017 07:16 PM, Jeff Law wrote: > The comment additions are fine. What's the rationale behind the > ordering of the fields? In general we want the opposite order from what > you did -- going from most strictly aligned to least strictly aligned > minimizes the amount of unused padding. Thank you for the review! I moved them because I thought it would make more sense to readers: flag that describe the “shape” of the thunk, and then the associated data, whose validity depends on the flags. I also thought that in practice, cgraph_thunk_info is embedded only in the cgraph_node class, and the field after “thunk” (“count”, a profile_count class) must be aligned at least on 8 bytes (because of n_bits/m_quality), so this change would not matter structure-size-wise. I’m not super confident about this though, so I’ll resubmit a patch without the reordering: I’ve added more comments anyway as I’ve learned more about this since yesterday. ;-)
On 09/15/2017 12:54 PM, Pierre-Marie de Rodat wrote: > I’m not super confident about this though, so I’ll resubmit a patch > without the reordering: I’ve added more comments anyway as I’ve learned > more about this since yesterday. ;-) Here it is!
On 09/15/2017 08:48 AM, Pierre-Marie de Rodat wrote: > On 09/15/2017 12:54 PM, Pierre-Marie de Rodat wrote: >> I’m not super confident about this though, so I’ll resubmit a patch >> without the reordering: I’ve added more comments anyway as I’ve >> learned more about this since yesterday. ;-) > > Here it is! > > -- > Pierre-Marie de Rodat > > 0001-Add-comments-to-struct-cgraph_thunk_info.patch > > > From 601dd0e949f4af456a11036918da9dbadbb3aa0c Mon Sep 17 00:00:00 2001 > From: Pierre-Marie de Rodat <derodat@adacore.com> > Date: Wed, 13 Sep 2017 16:21:04 +0200 > Subject: [PATCH] Add comments to struct cgraph_thunk_info > > This commit adds comments to fields in the cgraph_thunk_info structure > declaration from cgraph.h. They will hopefully answer questions that > people like myself can ask while discovering the thunk machinery. I > also made an assertion stricter in cgraph_node::create_thunk. > > Bootsrapped and regtested on x86_64-linux. > > gcc/ > > * cgraph.h (cgraph_thunk_info): Add comments. > * cgraph.c (cgraph_node::create_thunk): Adjust comment, make > assert for VIRTUAL_* arguments stricter. OK. jeff
On 09/15/2017 06:10 PM, Jeff Law wrote: > OK. > jeff Committed. Thanks!
On 15 September 2017 18:20:33 CEST, Pierre-Marie de Rodat <derodat@adacore.com> wrote: >On 09/15/2017 06:10 PM, Jeff Law wrote: >> OK. >> jeff > >Committed. Thanks! + * for result-adjusting thinks, the FIXED_OFFSET adjustment is done after s/think/thunk/ TIA
On 09/16/2017 09:35 AM, Bernhard Reutner-Fischer wrote: > + * for result-adjusting thinks, the FIXED_OFFSET adjustment is done after > > s/think/thunk/ > TIA Good catch, thank you! I just pushed the following obvious change, as r252904: Fix a typo in a comment (cgraph.c:cgraph_thunk_info) gcc/ * cgraph.h (cgraph_thunk_info): Fix a typo in a comment. diff --git a/gcc/cgraph.h b/gcc/cgraph.h index c668b37ef82..7daca1e40cc 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -662,7 +662,7 @@ struct GTY(()) cgraph_thunk_info { * for this-adjusting thunks, after the FIXED_OFFSET based adjustment is done, add to the result the offset found in the vtable at: vptr + VIRTUAL_VALUE - * for result-adjusting thinks, the FIXED_OFFSET adjustment is done after + * for result-adjusting thunks, the FIXED_OFFSET adjustment is done after the virtual one. */ bool virtual_offset_p;
diff --git a/gcc/cgraph.c b/gcc/cgraph.c index 69aa6c5bce2..20ab418d410 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -603,7 +603,7 @@ cgraph_node::create_same_body_alias (tree alias, tree decl) /* Add thunk alias into callgraph. The alias declaration is ALIAS and it aliases DECL with an adjustments made into the first parameter. - See comments in thunk_adjust for detail on the parameters. */ + See comments in struct cgraph_thunk_info for detail on the parameters. */ cgraph_node * cgraph_node::create_thunk (tree alias, tree, bool this_adjusting, @@ -619,8 +619,12 @@ cgraph_node::create_thunk (tree alias, tree, bool this_adjusting, node->reset (); else node = cgraph_node::create (alias); - gcc_checking_assert (!virtual_offset - || wi::eq_p (virtual_offset, virtual_value)); + + /* Make sure that if VIRTUAL_OFFSET is in sync with VIRTUAL_VALUE. */ + gcc_checking_assert (virtual_offset + ? wi::eq_p (virtual_offset, virtual_value) + : virtual_value == 0); + node->thunk.fixed_offset = fixed_offset; node->thunk.this_adjusting = this_adjusting; node->thunk.virtual_value = virtual_value; diff --git a/gcc/cgraph.h b/gcc/cgraph.h index 57cdaa45681..372ac9f01aa 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -629,18 +629,40 @@ extern const char * const cgraph_availability_names[]; extern const char * const ld_plugin_symbol_resolution_names[]; extern const char * const tls_model_names[]; -/* Information about thunk, used only for same body aliases. */ +/* Sub-structure of cgraph_node. Holds information about thunk, used only for + same body aliases. + + Thunks are basically wrappers around methods which are introduced in case + of multiple inheritance in order to adjust the value of the "this" pointer + or of the returned value. */ struct GTY(()) cgraph_thunk_info { - /* Information about the thunk. */ - HOST_WIDE_INT fixed_offset; - HOST_WIDE_INT virtual_value; - tree alias; + /* Set to true when alias node (the cgraph_node to which this struct belong) + is a thunk. Access to any other fields is invalid if this is false. */ + bool thunk_p; + + /* Nonzero for a "this" adjusting thunk and zero for a result adjusting + thunk. */ bool this_adjusting; + + /* If true, this thunk is what we call a virtual thunk. In this case, after + the FIXED_OFFSET based adjustment is done, add to the result the offset + found in the vtable at: vptr + VIRTUAL_VALUE. */ bool virtual_offset_p; + + /* ??? True for special kind of thunks, seems related to instrumentation. */ bool add_pointer_bounds_args; - /* Set to true when alias node is thunk. */ - bool thunk_p; + + /* Offset used to adjust "this". */ + HOST_WIDE_INT fixed_offset; + + /* Offset in the virtual table to get the offset to adjust "this". Valid iff + VIRTUAL_OFFSET_P is true. */ + HOST_WIDE_INT virtual_value; + + /* Thunk target, i.e. the method that this thunk wraps. Depending on the + TARGET_USE_LOCAL_THUNK_ALIAS_P macro, this may have to be a new alias. */ + tree alias; }; /* Information about the function collected locally.