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

[Ihor Solodrai <ihor.solodrai@linux.dev>]

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
>