Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type

[Yonghong Song <yhs@fb.com>]

On 2/7/21 6:18 AM, Mark Wielaard wrote:
> Hi,
> 
> On Sat, 2021-02-06 at 23:17 -0800, Yonghong Song wrote:
>> clang with dwarf5 may generate non-regular int base type,
>> i.e., not a signed/unsigned char/short/int/longlong/__int128.
>> Such base types are often used to describe
>> how an actual parameter or variable is generated. For example,
>>
>> 0x000015cf:   DW_TAG_base_type
>>                  DW_AT_name      ("DW_ATE_unsigned_1")
>>                  DW_AT_encoding  (DW_ATE_unsigned)
>>                  DW_AT_byte_size (0x00)
>>
>> 0x00010ed9:         DW_TAG_formal_parameter
>>                        DW_AT_location    (DW_OP_lit0,
>>                                           DW_OP_not,
>>                                           DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
>>                                           DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
>>                                           DW_OP_stack_value)
>>                        DW_AT_abstract_origin     (0x00013984 "branch")
>>
>> What it does is with a literal "0", did a "not" operation, and the converted to
>> one-bit unsigned int and then 8-bit unsigned int.
> 
> Thanks for tracking this down. Do you have any idea why the clang
> compiler emits this? You might be right that it is intended to do what

No, I don't know.

> you describe it does (but then it would simply encode an unsigned
> constant 1 char in a very inefficient way). But as implemented it
> doesn't seem to make any sense. What would DW_OP_convert of an zero
> sized base type even mean (if it is intended as a 1 bit sized typed,
> then why is there no DW_AT_bit_size)?

We need to report back to llvm community about this instance.
DW_AT_byte_size = 0 only for DW_ATE_unsigned_1 does not sound right.
DW_AT_bit_size might be needed as you suggested.

> 
> So I do think your patch makes sense. clang clearly is emitting
> something bogus. And so some fixup is needed. But maybe we should at
> least give a warning about it, otherwise it might never get fixed.

In pahole, to deal with dwarf weirdness, we have several other places
for workaround without warning. I think it is okay not to have
warning here as users cannot do anything about it. Also we have
this special int type with name __SANITIZED_FAKE_INT__ which will
signal the issue still exists if looking at BTF or generated vmlinux.h.

> 
> BTW. If these bogus base types are only emitted as part of a location
> expression and not as part of an actual function or variable type
> description, then why are we even trying to encode it as a BTF type? It

Let us still encode it as a BTF type as an evidence that we do
changed some types. After deduplication, we will only have ONE 
__SANITIZED_FAKE_INT__ type. I think leave this one type in BTF is okay.

> might be cheaper to just skip/drop it. But maybe the code setup makes
> it hard to know whether or not such a (bogus) type is actually
> referenced from a function or variable description?

I guess it is possible that we do a preprocessing to check whether
any types BTF intends to emit (var/func definition, etc.) referencing
such types or not. This will involves overhead for every CU most of 
which does not have this issue. And this should not really happen given 
a sane dwarf.

With __SANITIZED_FAKE_INT__ as the type name, if something e.g., a 
variable definition use this type, you will have
    __SANITIZED_FAKE_INT__ a;
in skeleton, or have
    struct {
       __SANITIZED_FAKE_INT__ member;
       ...
    };
in vmlinux.h,
we should be able to catch such an error easily.

> 
> Cheers,
> 
> Mark
>