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

[Ihor Solodrai <ihor.solodrai@xxxxxxxxx>]

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