Alan Maguire <alan.maguire@xxxxxxxxxx> writes:
> On 12/02/2025 00:49, Stephen Brennan wrote:
>> btf__dedup() does not deduplicate variables, nor the contents of the
>> DATASEC. This is reasonable, because there could very well be multiple
>> variables of the same name & type from different CUs, but with different
>> tags or DATASEC offsets. So it is up to us to ensure we don't provide
>> duplicate variables to libbpf.
>>
>> Unfortunately, there are some cases where variables are declared
>> "__weak" in the source code, resulting in the same variable, with the
>> same name & type, at the same offset, being encoded twice. The DWARF
>> information does not encode any difference between the "__weak" symbol
>> and the overriding symbol. And even if it did, we couldn't filter out
>> "__weak" symbols, because they aren't always overridden. So, we must
>> deduplicate them.
>>
>> We can't guarantee that the types are identical prior to deduplication,
>> but we can at least verify the offsets & names are identical. If this
>> happens, simply skip the duplicate copy (or copies) of the variable.
>>
>
> hi Stephen, quick question; could we do without patch 2, and if we get
> an offset match, then just look up the names of the offset-matching
> variables in the code directly to see if they refer to the same
> variable? that way we would only incur costs for the rare cases where we
> get an offset match. You could use the names__match() function to help
> with that I think too. Would that be simpler maybe?
Without patch 2, we would have already emitted the VAR and DECL_TAG
types. We can still skip emitting the var_secinfo which avoids the
validation error, but it feels a bit messy to leave around the extra
VARs that aren't covered by any var_secinfo. You could probably say with
more confidence whether that's an actual problem.
But as far as the costs incurred by this & patch 2, I _think_
there's not really much extra: yes, we have to store a bit more data
into the buffer for each ELF section, so there is that. (I can probably
reduce some of that in v2, some of the stuff I did is a bit silly in
hindsight.) Mostly we just rearrange the order of when we add the VAR
and DATASEC, and we only check for name matches when we have an offset
match anyway. I do concede that prior to patch 2, VARs may be created in
parallel, and now, they are created in serial, so for parallel BTF that
may be a small regression. Ultimately, I haven't benchmarked and can try
that as well if you'd like.
Let me know how you feel about the extra VARs & the performance!
Thanks,
Stephen
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@xxxxxxxxxx>
>> ---
>> btf_encoder.c | 29 +++++++++++++++++++++++++++++
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index d989fbd..29b2054 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -869,8 +869,36 @@ static int32_t btf_encoder__encode_stored_vars(struct btf_encoder *encoder,
>> struct saved_var *var, uint16_t nr)
>> {
>> int i, id, annot_idx;
>> + int prev = 0;
>>
>> for (i = 0; i < nr; i++) {
>> + if (i && var[i].offset == var[prev].offset) {
>> + /* The offset of this var is the same as the previous.
>> + * This can happen when a variable is declared in one CU
>> + * as "__weak", and then overridden in another CU. There
>> + * is no indication in the DWARF which symbol is weak,
>> + * so we need to deduplicate. If the duplicate is the
>> + * same name, this is expected, and we can just drop the
>> + * second instance. Otherwise, we need to raise an
>> + * error.
>> + *
>> + * Ideally, we would like to verify that the types of
>> + * the duplicated variables are the same type as well.
>> + * However, there's no way to do this without
>> + * deduplicating the BTF, but at that point, our type
>> + * IDs won't be valid.
>> + */
>> + if (strcmp(var[i].name, var[prev].name) == 0) {
>> + var[i].type = 0; /* Don't encode corresponding secinfo */
>> + continue;
>> + } else {
>> + fprintf(stderr,
>> + "error: encountered variable '%s' (type %x) at offset 0x%x which duplicates variable '%s' at the same offset\n",
>> + var[i].name, var[i].type, var[i].offset, var[prev].name);
>> + return -1;
>> + }
>> + }
>> +
>> id = btf_encoder__add_var(encoder, var[i].type,
>> var[i].name, var[i].linkage);
>> if (id < 0) {
>> @@ -894,6 +922,7 @@ static int32_t btf_encoder__encode_stored_vars(struct btf_encoder *encoder,
>> free(var->annots);
>> var->annots = NULL;
>>
>> + prev = i;
>> var[i].type = id;
>> }
>> return 0;
