Re: [PATCH dwarves v4 01/11] dwarf_loader: Reduce parameter checking with clang DW_AT_calling_convention attr
Yonghong Song <[email protected]> Sat, 23 May 2026 09:22:46 -0700
| Newsgroups | org.kernel.vger.dwarves,org.kernel.vger.bpf |
|---|---|
| Message-ID | <[email protected]> |
On 3/30/26 1:31 AM, Alan Maguire wrote:
> On 26/03/2026 01:31, Yonghong Song wrote:
>> Currently every function is checked for its parameters to identify whether
>> the signature changed or not. If signature indeed changed, pahole may do
>> some adjustment for parameters for true signatures.
>>
>> In clang, any function with the following attribute
>> DW_AT_calling_convention (DW_CC_nocall)
>> indicates this function having signature changed.
>> pahole can take advantage of this to avoid parameter checking if
>> DW_AT_calling_convention is not DW_CC_nocall.
>>
>> But more importantly, DW_CC_nocall can identify signature-changed functions
>> and parameters can be checked one-after-another to create the true
>> signatures. Otherwise, it takes more effort to identify whether a
>> function has signature changed or not. For example, for funciton
>> __bpf_kfunc static void bbr_main(struct sock *sk, u32 ack, int flag,
>> const struct rate_sample *rs) { ... }
>> and bbr_main() is a callback function in
>> .cong_control = bbr_main
>> in 'struct tcp_congestion_ops tcp_bbr_cong_ops'.
>> In the above bbr_main(...), parameter 'ack' and 'flag' are not used.
>> The following are some details:
>>
>> 0x0a713b8d: DW_TAG_formal_parameter
>> DW_AT_location (indexed (0x28) loclist = 0x0166d452:
>> [0xffffffff83e77fd9, 0xffffffff83e78016): DW_OP_reg5 RDI
>> ...
>> DW_AT_name ("sk")
>> DW_AT_type (0x0a6f5b2b "sock *")
>> ...
>>
>> 0x0a713b98: DW_TAG_formal_parameter
>> DW_AT_name ("ack")
>> DW_AT_type (0x0a6f58fd "u32")
>> ...
>>
>> 0x0a713ba2: DW_TAG_formal_parameter
>> DW_AT_name ("flag")
>> DW_AT_type (0x0a6f57d1 "int")
>> ...
>>
>> 0x0a713bac: DW_TAG_formal_parameter
>> DW_AT_location (indexed (0x29) loclist = 0x0166d4a8:
>> [0xffffffff83e77fd9, 0xffffffff83e78016): DW_OP_reg2 RCX
>> ...
>> DW_AT_name ("rs")
>> DW_AT_type (0x0a710da5 "const rate_sample *")
>>
>> Some analysis for the above dwarf can conclude that the 'ark' and 'flag'
>> may be related to RSI and RDX, considering the last one is RCX. Basically this
>> requires all parameters are available to collectively decide whether the
>> true signature can be found or not. In such case, DW_CC_nocall can make things
>> easier as parameter can be checked one after another.
>>
>> For a clang built bpf-next kernel with x86_64, in non-LTO setup,
>> the number of kernel functions is 69103 and the number of signature changed
>> functions is 875, based on
>> DW_AT_calling_convention (DW_CC_nocall)
>> indication.
>>
>> Among 875 signature changed functions, after this patch, 343 functions
>> can have proper true signatures, mostly due to simple dead argument
>> elimination. The number of remaining functions, which cannot get the
>> true signature, is 532 due to dead or additional-checked parameters.
>>
>> They will be addressed in the subsequent commits.
>>
>> In llvm23, I implemented [1] which added DW_CC_nocall for ArgumentPromotion pass.
>> This compiler pass can add additional DW_CC_nocall cases for the following
>> compilation:
>> - Flag -O3 or FullLTO
>> So once llvm23 available, we may have more DW_CC_nocall cases, hence more
>> potential true signatures if the kernel is built with -O3 or
>> with FullLTO (CONFIG_LTO_CLANG_FULL).
>>
>> [1] https://github.com/llvm/llvm-project/pull/178973
>>
>> Signed-off-by: Yonghong Song <[email protected]>
>> ---
>> dwarf_loader.c | 82 +++++++++++++++++++++++++++++++++++++++++---------
>> dwarves.h | 1 +
>> 2 files changed, 69 insertions(+), 14 deletions(-)
>>
>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>> index 16fb7be..c569002 100644
>> --- a/dwarf_loader.c
>> +++ b/dwarf_loader.c
>> @@ -1190,6 +1190,10 @@ static ptrdiff_t __dwarf_getlocations(Dwarf_Attribute *attr,
>> return ret;
>> }
>>
>> +struct func_info {
>> + bool signature_changed;
>> +};
>> +
>> /* For DW_AT_location 'attr':
>> * - if first location is DW_OP_regXX with expected number, return the register;
>> * otherwise save the register for later return
>> @@ -1252,7 +1256,8 @@ out:
>> }
>>
>> static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>> - struct conf_load *conf, int param_idx)
>> + struct conf_load *conf, int param_idx,
>> + struct func_info *info)
>> {
>> struct parameter *parm = tag__alloc(cu, sizeof(*parm));
>>
>> @@ -1263,8 +1268,15 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>> tag__init(&parm->tag, cu, die);
>> parm->name = attr_string(die, DW_AT_name, conf);
>> parm->idx = param_idx;
>> - if (param_idx >= cu->nr_register_params || param_idx < 0)
>> + if (param_idx < 0)
>> + return parm;
>> + if (!info->signature_changed) {
>> + if (cu->producer_clang || param_idx >= cu->nr_register_params)
>> + return parm;
>> + } else if (param_idx >= cu->nr_register_params) {
>> return parm;
>> + }
>> +
> logic here is confusing. why not just retain
>
>
> if (param_idx >= cu->nr_register_params || param_idx < 0)
> return parm;
>
> and add
> if (cu->producer_clang && !info->signature_changed)
> return parm;
The code
+ if (param_idx < 0)
+ return parm;
+ if (!info->signature_changed) {
+ if (cu->producer_clang || param_idx >= cu->nr_register_params)
+ return parm;
+ } else if (param_idx >= cu->nr_register_params) {
return parm;
+ }
is written in a way to have minimum change later on.
For example,
if (param_idx >= cu->nr_register_params || param_idx < 0)
return parm;
works for this patch.
But consider parameters after cu->nr_register_params, e.g., 7th parameter
for x86_64. We cannot just return parm since it is possible the 7th
parameter may be dead or may have some other changes. We will go back
to
if (param_idx < 0)
return parm;
So I prefer to keep the current implementation.
>
> but given that info can be NULL, isn't it safer to have
>
> if (cu->producer_clang && info && !info->signature_changed)
> return parm;
'info' will not be NULL as this pointer. 'info' could be NULL if param_idx < 0.
But for param_idx >= 0 case, info is not NULL. I have audited it.
>
> Technically we could drop the cu->producer_clang clause since info->signature changed
> will only be true for clang functions with DW_CC_nocall.
cu->producer_clang is needed. Otherwise, for gcc, info->signature is false and
the original parm will be returned which is incorrect.
>
>
>> /* Parameters which use DW_AT_abstract_origin to point at
>> * the original parameter definition (with no name in the DIE)
>> * are the result of later DWARF generation during compilation
>> @@ -1337,7 +1349,7 @@ static int formal_parameter_pack__load_params(struct formal_parameter_pack *pack
>> continue;
>> }
>>
>> - struct parameter *param = parameter__new(die, cu, conf, -1);
>> + struct parameter *param = parameter__new(die, cu, conf, -1, NULL);
>>
>> if (param == NULL)
>> return -1;
>> @@ -1502,6 +1514,29 @@ static struct ftype *ftype__new(Dwarf_Die *die, struct cu *cu)
>> return ftype;
>> }
>>
>> +static bool function__signature_changed(struct function *func, Dwarf_Die *die)
>> +{
>
> given that AFAIK DW_CC_nocall is only used in clang currently, why not short-circuit
> the logic here and return false if (cu->producer_clang) ? Logical place to do this
> would be in die__create_new_function().
Yes, I can do this. I see your suggested code below with cu->producer_clang
as part of condition. It does make sense.
>
>> + /* The inlined DW_TAG_subprogram typically has the original source type for
>> + * abstract origin of a concrete function with address range, inlined subroutine,
>> + * or call site.
>> + */
>> + if (func->inlined)
>> + return false;
>> +
>> + if (!func->abstract_origin)
>> + return attr_numeric(die, DW_AT_calling_convention) == DW_CC_nocall;
>> +
>> + Dwarf_Attribute attr;
>> + if (dwarf_attr(die, DW_AT_abstract_origin, &attr)) {
>> + Dwarf_Die origin;
>> + if (dwarf_formref_die(&attr, &origin))
>> + return attr_numeric(&origin, DW_AT_calling_convention) == DW_CC_nocall;
>> + }
>> +
>> + /* This should not happen */
>> + return false;
>> +}
>> +
>> static struct function *function__new(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
>> {
>> struct function *func = tag__alloc(cu, sizeof(*func));
>> @@ -1800,9 +1835,9 @@ static struct tag *die__create_new_parameter(Dwarf_Die *die,
>> struct ftype *ftype,
>> struct lexblock *lexblock,
>> struct cu *cu, struct conf_load *conf,
>> - int param_idx)
>> + int param_idx, struct func_info *info)
>> {
>> - struct parameter *parm = parameter__new(die, cu, conf, param_idx);
>> + struct parameter *parm = parameter__new(die, cu, conf, param_idx, info);
>>
>> if (parm == NULL)
>> return NULL;
>> @@ -1889,7 +1924,7 @@ static struct tag *die__create_new_subroutine_type(Dwarf_Die *die,
>> tag__print_not_supported(die);
>> continue;
>> case DW_TAG_formal_parameter:
>> - tag = die__create_new_parameter(die, ftype, NULL, cu, conf, -1);
>> + tag = die__create_new_parameter(die, ftype, NULL, cu, conf, -1, NULL);
>> break;
>> case DW_TAG_unspecified_parameters:
>> ftype->unspec_parms = 1;
>> @@ -2118,7 +2153,8 @@ out_enomem:
>> }
>>
>> static int die__process_function(Dwarf_Die *die, struct ftype *ftype,
>> - struct lexblock *lexblock, struct cu *cu, struct conf_load *conf);
>> + struct lexblock *lexblock, struct cu *cu, struct conf_load *conf,
>> + struct func_info *info);
>>
>> static int die__create_new_lexblock(Dwarf_Die *die,
>> struct cu *cu, struct lexblock *father, struct conf_load *conf)
>> @@ -2126,7 +2162,7 @@ static int die__create_new_lexblock(Dwarf_Die *die,
>> struct lexblock *lexblock = lexblock__new(die, cu);
>>
>> if (lexblock != NULL) {
>> - if (die__process_function(die, NULL, lexblock, cu, conf) != 0)
>> + if (die__process_function(die, NULL, lexblock, cu, conf, NULL) != 0)
>> goto out_delete;
>> }
>> if (father != NULL)
>> @@ -2246,7 +2282,8 @@ static struct tag *die__create_new_inline_expansion(Dwarf_Die *die,
>> }
>>
>> static int die__process_function(Dwarf_Die *die, struct ftype *ftype,
>> - struct lexblock *lexblock, struct cu *cu, struct conf_load *conf)
>> + struct lexblock *lexblock, struct cu *cu, struct conf_load *conf,
>> + struct func_info *info)
>> {
>> int param_idx = 0;
>> Dwarf_Die child;
>> @@ -2320,7 +2357,7 @@ static int die__process_function(Dwarf_Die *die, struct ftype *ftype,
>> continue;
>> }
>> case DW_TAG_formal_parameter:
>> - tag = die__create_new_parameter(die, ftype, lexblock, cu, conf, param_idx++);
>> + tag = die__create_new_parameter(die, ftype, lexblock, cu, conf, param_idx++, info);
>> break;
>> case DW_TAG_variable:
>> tag = die__create_new_variable(die, cu, conf, 0);
>> @@ -2391,11 +2428,15 @@ out_enomem:
>> static struct tag *die__create_new_function(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
>> {
>> struct function *function = function__new(die, cu, conf);
>> + struct func_info info = {};
>> +
>> + if (function != NULL) {
>> + info.signature_changed = function__signature_changed(function, die);
>>
> i'd suggest adding a coment and a predicate here, something like
>
> /* For clang, we determine if function signature changes via DW_AT_calling_convention
> * set to DW_CC_nocall.
> */
> if (cu->producer_clang)
> info.signature_changed = function__signature_changed(function, die);
I will do
if (cu->pruducer_clang && funciton != NULL)
info.signature_changed = function__signature_changed(function, die);
>
>> - if (function != NULL &&
>> - die__process_function(die, &function->proto, &function->lexblock, cu, conf) != 0) {
>> - function__delete(function, cu);
>> - function = NULL;
>> + if (die__process_function(die, &function->proto, &function->lexblock, cu, conf, &info) != 0) {
>> + function__delete(function, cu);
>> + function = NULL;
>> + }
>> }
>>
>> return function ? &function->proto.tag : NULL;
>> @@ -3045,6 +3086,17 @@ static unsigned long long dwarf_tag__orig_id(const struct tag *tag,
>> return cu->extra_dbg_info ? dtag->id : 0;
>> }
>>
>> +static bool attr_producer_clang(Dwarf_Die *die)
>> +{
>> + const char *producer;
>> +
>> + producer = attr_string(die, DW_AT_producer, NULL);
>> + if (!producer)
>> + return false;
>> +
>> + return !!strstr(producer, "clang");
>> +}
>> +
>> struct debug_fmt_ops dwarf__ops;
>>
>> static int die__process(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
>> @@ -3082,6 +3134,7 @@ static int die__process(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
>> }
>>
>> cu->language = attr_numeric(die, DW_AT_language);
>> + cu->producer_clang = attr_producer_clang(die);
>>
>> if (conf->early_cu_filter)
>> cu = conf->early_cu_filter(cu);
>> @@ -3841,6 +3894,7 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
>> cu->priv = dcu;
>> cu->dfops = &dwarf__ops;
>> cu->language = attr_numeric(cu_die, DW_AT_language);
>> + cu->producer_clang = attr_producer_clang(cu_die);
>> cus__add(cus, cu);
>> }
>>
>> diff --git a/dwarves.h b/dwarves.h
>> index d7c6474..4cabab0 100644
>> --- a/dwarves.h
>> +++ b/dwarves.h
>> @@ -302,6 +302,7 @@ struct cu {
>> uint8_t has_addr_info:1;
>> uint8_t uses_global_strings:1;
>> uint8_t little_endian:1;
>> + uint8_t producer_clang:1;
>> uint8_t nr_register_params;
>> int register_params[ARCH_MAX_REGISTER_PARAMS];
>> int functions_saved;