Re: [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value on stack

Ihor Solodrai
2025-07-04T19:59:14Z

On 7/4/25 2:01 AM, Alexis Lothoré wrote:
> Hello Ihor,
>
> thanks for the prompt feedback and testing !
>
> On Thu Jul 3, 2025 at 8:17 PM CEST, Ihor Solodrai wrote:
>> On 7/3/25 2:02 AM, Alexis LothorÃ© (eBPF Foundation) wrote:
>
> [...]
>
>>> /* do not exclude functions with optimized-out parameters; they
>>> * may still be _called_ with the right parameter values, they
>>> * just do not _use_ them. Only exclude functions with
>>> - * unexpected register use or multiple inconsistent prototypes.
>>> + * unexpected register use, multiple inconsistent prototypes or
>>> + * uncertain parameters location
>>> */
>>> - add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto;
>>> + add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto && !state->uncertain_parm_loc;
>>
>>
>> Is it possible for a function to have uncertain_parm_loc in one CU,
>> but not in another?
>>
>> If yes, we still don't want the function in BTF, right?
>
> TBH, my understanding about those discrepancies between CUs about the same
> functions and how pahole handle them is still a bit fragile. Have you got
> any example about how it could be the case ?

I would hope stuff like this doesn't happen often in the real
world, but in principle you could have the following situation:

#ifdef ENABLE_PACKING
struct some_data {
char header;
int payload;
short footer;
} __attribute__((packed));
#else
struct some_data {
char header;
int payload;
short footer;
};
#endif

void read_data(/* lots of args */, struct some_data data) { ... }

And then one user has #define ENABLE_PACKING and the other doesn't,
for example:

#define ENABLE_PACKING // or not
#include "some_data.h"

void user() {
struct some_data = get_some_data();
...
read_data(/* args */, some_data);
}

And then you compile a binary with both users:

$ gcc -g -O0 user1.c user2.c

DWARF will contain both packed and not packed instances of struct
some_data and two corresponding read_data() funcs.

>
> If it _can_ happen, I guess you are suggesting to make sure that copies are
> compared in saved_functions_combine and their uncertain_loc_parm flag are
> aligned. Something like this:
>
> uncertain_parm_loc = a->uncertain_parm_loc | b->uncertain_parm_loc;
> [...]
> a->uncertain_parm_loc = b->uncertain_parm_loc = uncertain_parm_loc;

I asked out of curiosity, but you're right that this piece is needed.

>
>>> @@ -2693,6 +2736,9 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>>> if (!func)
>>> continue;
>>>
>>> + if (ftype__has_uncertain_arg_loc(cu, &fn->proto))
>>> + fn->proto.uncertain_parm_loc = 1;
>>> +
>>> err = btf_encoder__save_func(encoder, fn, func);
>>
>> I think checking and setting uncertain_parm_loc flag should be done
>> inside btf_encoder__save_func(), because that's where we inspect DWARF
>> function prototype and add a new btf_encoder_func_state.
>
> ACK, it can be moved there
>
> Thanks,
>
> Alexis
>