On 01/02/2023 17:19, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 30, 2023 at 02:29:42PM +0000, Alan Maguire escreveu:
>> This will be useful for postponing local function addition later on.
>> As part of this, store the type id offset and unspecified type in
>> the encoder, as this will simplify late addition of local functions.
>>
>> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>> ---
>> btf_encoder.c | 101 +++++++++++++++++++++++++++++++++-------------------------
>> 1 file changed, 57 insertions(+), 44 deletions(-)
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index a5fa04a..44f1905 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -54,6 +54,8 @@ struct btf_encoder {
>> struct gobuffer percpu_secinfo;
>> const char *filename;
>> struct elf_symtab *symtab;
>> + uint32_t type_id_off;
>> + uint32_t unspecified_type;
>> bool has_index_type,
>> need_index_type,
>> skip_encoding_vars,
>> @@ -593,20 +595,20 @@ static int32_t btf_encoder__add_func_param(struct btf_encoder *encoder, const ch
>> }
>> }
>>
>> -static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t type_id_off, uint32_t tag_type)
>> +static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_type)
>> {
>> if (tag_type == 0)
>> return 0;
>>
>> - if (encoder->cu->unspecified_type.tag && tag_type == encoder->cu->unspecified_type.type) {
>> + if (tag_type == encoder->unspecified_type) {
>> // No provision for encoding this, turn it into void.
>> return 0;
>> }
>
> Humm, are those two lines (above) really equivalent? IIRC I read that as
> encoder->cu->unspecified_type.tag being zero means we still didn't set
> it, not that it is void (zero), right?
>
> So if we're passing a tag_type zero, void, we'll return 0, i.e. turn
> into a void, so seems equivalent, try not to combine patches like this
> in the future, i.e. I would expect, from a quick glance, to have:
>
> - if (encoder->cu->unspecified_type.tag && tag_type == encoder->cu->unspecified_type.type) {
> + if (encoder->unspecified_type && tag_type == encoder->unspecified_type) {
>
> I.e. just the removal of the indirection thru encoder->cu. Or am I
> missing something here?
>
No, I don't think you're missing anything. I should have separated
out the changes that record encoder info such that we don't need to
rely on the current CU; we need those because now we interact with
functions potentially much later on, and the current CU can be
different. Ideally that would have come before this patch
refactoring function addition.
I can rework the series to do that if you like? Patch 5 will
need a bit of work too so that we can continue to support the
legacy behaviour, and we'll need an additional patch to support
switching the inconsistent prototype handling on also.
> - Arnaldo
>
>>
>> - return type_id_off + tag_type;
>> + return encoder->type_id_off + tag_type;
>> }
>>
>> -static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype, uint32_t type_id_off)
>> +static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype)
>> {
>> struct btf *btf = encoder->btf;
>> const struct btf_type *t;
>> @@ -616,7 +618,7 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
>>
>> /* add btf_type for func_proto */
>> nr_params = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
>> - type_id = btf_encoder__tag_type(encoder, type_id_off, ftype->tag.type);
>> + type_id = btf_encoder__tag_type(encoder, ftype->tag.type);
>>
>> id = btf__add_func_proto(btf, type_id);
>> if (id > 0) {
>> @@ -634,7 +636,7 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
>> ftype__for_each_parameter(ftype, param) {
>> const char *name = parameter__name(param);
>>
>> - type_id = param->tag.type == 0 ? 0 : type_id_off + param->tag.type;
>> + type_id = param->tag.type == 0 ? 0 : encoder->type_id_off + param->tag.type;
>> ++param_idx;
>> if (btf_encoder__add_func_param(encoder, name, type_id, param_idx == nr_params))
>> return -1;
>> @@ -762,6 +764,31 @@ static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char
>> return id;
>> }
>>
>> +static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn)
>> +{
>> + int btf_fnproto_id, btf_fn_id, tag_type_id;
>> + struct llvm_annotation *annot;
>> + const char *name;
>> +
>> + btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto);
>> + name = function__name(fn);
>> + btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
>> + if (btf_fnproto_id < 0 || btf_fn_id < 0) {
>> + printf("error: failed to encode function '%s'\n", function__name(fn));
>> + return -1;
>> + }
>> + list_for_each_entry(annot, &fn->annots, node) {
>> + tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_fn_id,
>> + annot->component_idx);
>> + if (tag_type_id < 0) {
>> + fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
>> + annot->value, name, annot->component_idx);
>> + return -1;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> /*
>> * This corresponds to the same macro defined in
>> * include/linux/kallsyms.h
>> @@ -859,22 +886,21 @@ static void dump_invalid_symbol(const char *msg, const char *sym,
>> fprintf(stderr, "PAHOLE: Error: Use '--btf_encode_force' to ignore such symbols and force emit the btf.\n");
>> }
>>
>> -static int tag__check_id_drift(const struct tag *tag,
>> - uint32_t core_id, uint32_t btf_type_id,
>> - uint32_t type_id_off)
>> +static int tag__check_id_drift(struct btf_encoder *encoder, const struct tag *tag,
>> + uint32_t core_id, uint32_t btf_type_id)
>> {
>> - if (btf_type_id != (core_id + type_id_off)) {
>> + if (btf_type_id != (core_id + encoder->type_id_off)) {
>> fprintf(stderr,
>> "%s: %s id drift, core_id: %u, btf_type_id: %u, type_id_off: %u\n",
>> __func__, dwarf_tag_name(tag->tag),
>> - core_id, btf_type_id, type_id_off);
>> + core_id, btf_type_id, encoder->type_id_off);
>> return -1;
>> }
>>
>> return 0;
>> }
>>
>> -static int32_t btf_encoder__add_struct_type(struct btf_encoder *encoder, struct tag *tag, uint32_t type_id_off)
>> +static int32_t btf_encoder__add_struct_type(struct btf_encoder *encoder, struct tag *tag)
>> {
>> struct type *type = tag__type(tag);
>> struct class_member *pos;
>> @@ -896,7 +922,8 @@ static int32_t btf_encoder__add_struct_type(struct btf_encoder *encoder, struct
>> * is required.
>> */
>> name = class_member__name(pos);
>> - if (btf_encoder__add_field(encoder, name, type_id_off + pos->tag.type, pos->bitfield_size, pos->bit_offset))
>> + if (btf_encoder__add_field(encoder, name, encoder->type_id_off + pos->tag.type,
>> + pos->bitfield_size, pos->bit_offset))
>> return -1;
>> }
>>
>> @@ -936,11 +963,11 @@ static int32_t btf_encoder__add_enum_type(struct btf_encoder *encoder, struct ta
>> return type_id;
>> }
>>
>> -static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag, uint32_t type_id_off,
>> +static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag,
>> struct conf_load *conf_load)
>> {
>> /* single out type 0 as it represents special type "void" */
>> - uint32_t ref_type_id = tag->type == 0 ? 0 : type_id_off + tag->type;
>> + uint32_t ref_type_id = tag->type == 0 ? 0 : encoder->type_id_off + tag->type;
>> struct base_type *bt;
>> const char *name;
>>
>> @@ -970,7 +997,7 @@ static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag,
>> if (tag__type(tag)->declaration)
>> return btf_encoder__add_ref_type(encoder, BTF_KIND_FWD, 0, name, tag->tag == DW_TAG_union_type);
>> else
>> - return btf_encoder__add_struct_type(encoder, tag, type_id_off);
>> + return btf_encoder__add_struct_type(encoder, tag);
>> case DW_TAG_array_type:
>> /* TODO: Encode one dimension at a time. */
>> encoder->need_index_type = true;
>> @@ -978,7 +1005,7 @@ static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag,
>> case DW_TAG_enumeration_type:
>> return btf_encoder__add_enum_type(encoder, tag, conf_load);
>> case DW_TAG_subroutine_type:
>> - return btf_encoder__add_func_proto(encoder, tag__ftype(tag), type_id_off);
>> + return btf_encoder__add_func_proto(encoder, tag__ftype(tag));
>> case DW_TAG_unspecified_type:
>> /* Just don't encode this for now, converting anything with this type to void (0) instead.
>> *
>> @@ -1281,7 +1308,7 @@ static bool ftype__has_arg_names(const struct ftype *ftype)
>> return true;
>> }
>>
>> -static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder, uint32_t type_id_off)
>> +static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>> {
>> struct cu *cu = encoder->cu;
>> uint32_t core_id;
>> @@ -1366,7 +1393,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder, uint32_
>> continue;
>> }
>>
>> - type = var->ip.tag.type + type_id_off;
>> + type = var->ip.tag.type + encoder->type_id_off;
>> linkage = var->external ? BTF_VAR_GLOBAL_ALLOCATED : BTF_VAR_STATIC;
>>
>> if (encoder->verbose) {
>> @@ -1507,7 +1534,6 @@ void btf_encoder__delete(struct btf_encoder *encoder)
>>
>> int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct conf_load *conf_load)
>> {
>> - uint32_t type_id_off = btf__type_cnt(encoder->btf) - 1;
>> struct llvm_annotation *annot;
>> int btf_type_id, tag_type_id, skipped_types = 0;
>> uint32_t core_id;
>> @@ -1516,21 +1542,24 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>> int err = 0;
>>
>> encoder->cu = cu;
>> + encoder->type_id_off = btf__type_cnt(encoder->btf) - 1;
>> + if (encoder->cu->unspecified_type.tag)
>> + encoder->unspecified_type = encoder->cu->unspecified_type.type;
>>
>> if (!encoder->has_index_type) {
>> /* cu__find_base_type_by_name() takes "type_id_t *id" */
>> type_id_t id;
>> if (cu__find_base_type_by_name(cu, "int", &id)) {
>> encoder->has_index_type = true;
>> - encoder->array_index_id = type_id_off + id;
>> + encoder->array_index_id = encoder->type_id_off + id;
>> } else {
>> encoder->has_index_type = false;
>> - encoder->array_index_id = type_id_off + cu->types_table.nr_entries;
>> + encoder->array_index_id = encoder->type_id_off + cu->types_table.nr_entries;
>> }
>> }
>>
>> cu__for_each_type(cu, core_id, pos) {
>> - btf_type_id = btf_encoder__encode_tag(encoder, pos, type_id_off, conf_load);
>> + btf_type_id = btf_encoder__encode_tag(encoder, pos, conf_load);
>>
>> if (btf_type_id == 0) {
>> ++skipped_types;
>> @@ -1538,7 +1567,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>> }
>>
>> if (btf_type_id < 0 ||
>> - tag__check_id_drift(pos, core_id, btf_type_id + skipped_types, type_id_off)) {
>> + tag__check_id_drift(encoder, pos, core_id, btf_type_id + skipped_types)) {
>> err = -1;
>> goto out;
>> }
>> @@ -1572,7 +1601,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>> continue;
>> }
>>
>> - btf_type_id = type_id_off + core_id;
>> + btf_type_id = encoder->type_id_off + core_id;
>> ns = tag__namespace(pos);
>> list_for_each_entry(annot, &ns->annots, node) {
>> tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_type_id, annot->component_idx);
>> @@ -1585,8 +1614,6 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>> }
>>
>> cu__for_each_function(cu, core_id, fn) {
>> - int btf_fnproto_id, btf_fn_id;
>> - const char *name;
>>
>> /*
>> * Skip functions that:
>> @@ -1616,27 +1643,13 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>> continue;
>> }
>>
>> - btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto, type_id_off);
>> - name = function__name(fn);
>> - btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
>> - if (btf_fnproto_id < 0 || btf_fn_id < 0) {
>> - err = -1;
>> - printf("error: failed to encode function '%s'\n", function__name(fn));
>> + err = btf_encoder__add_func(encoder, fn);
>> + if (err)
>> goto out;
>> - }
>> -
>> - list_for_each_entry(annot, &fn->annots, node) {
>> - tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_fn_id, annot->component_idx);
>> - if (tag_type_id < 0) {
>> - fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
>> - annot->value, name, annot->component_idx);
>> - goto out;
>> - }
>> - }
>> }
>>
>> if (!encoder->skip_encoding_vars)
>> - err = btf_encoder__encode_cu_variables(encoder, type_id_off);
>> + err = btf_encoder__encode_cu_variables(encoder);
>> out:
>> encoder->cu = NULL;
>> return err;
>> --
>> 1.8.3.1
>>
>
