[Bug target/107844] New: error: argument is not a field access for __builtin_preserve_field_info

[Bug target/107844] New: error: argument is not a field access for __builtin_preserve_field_info

[james.hilliard1 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107844

            Bug ID: 107844
           Summary: error: argument is not a field access for
                    __builtin_preserve_field_info
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: james.hilliard1 at gmail dot com
  Target Milestone: ---

I'm seeing this error which does not occur in llvm for a bpf
test(test_core_reloc_existence.c) in bpf-next:

In file included from progs/test_core_reloc_existence.c:7:
progs/test_core_reloc_existence.c: In function 'test_core_existence':
/home/buildroot/bpf-next/tools/bpf/resolve_btfids/libbpf/include/bpf/bpf_core_read.h:132:9:
error: argument is not a field access
  132 |         __builtin_preserve_field_info(___bpf_field_ref(field),
BPF_FIELD_EXISTS)
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
progs/test_core_reloc_existence.c:66:13: note: in expansion of macro
'bpf_core_field_exists'
   66 |         if (bpf_core_field_exists(struct core_reloc_existence, arr))
      |             ^~~~~~~~~~~~~~~~~~~~~

[Bug target/107844] error: argument is not a field access for __builtin_preserve_field_info

[david.faust at oracle dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107844

--- Comment #1 from David Faust <david.faust at oracle dot com> ---
Looks like this is a result of the combination of how the bpf_core_field_exists
macro is defined and some sort of optimization(?) happening in the C frontend.

Consider:

  struct S {
    unsigned short x;
    char c[12];
  };

  int foo () {
    int has_c = bpf_core_field_exists (struct S, c);
    return has_c;
  }

The bpf_core_field_exists macro expands to:

  int has_c =
    __builtin_preserve_field_info((((typeof(struct S) *)0)->c),
BPF_RELO_FIELD_EXISTS);

For some reason, for array members specifically, this construction results in
the tree for the parameter when resolving the builtin being simply:

  (gdb) pt param
   <integer_cst 0x7ffff7531b88 type <pointer_type 0x7ffff741bc78> constant 2>

i.e. a pointer to 0x2, the offset of 'c' in a 'struct S' mapped at 0x0. For
non-array members, 'param' is some kind of <component_ref> as I'd expect.

This gives us two problems:
 a) We cannot distinguish an array member from a non-member constant-value
    pointer.
 b) We cannot correctly compute information for the CO-RE relocation.
    For example, in this case BPF_RELO_FIELD_BYTE_SIZE will be calculated as 8
    (size of pointer), not 12 (size of the array).

I'm not sure how to resolve this and support the existing helper macros in the
kernel/libbpf. Might need some change in the C frontend, or to somehow pass
more
information to the target_resolve_overloaded_builtin hook...

[Bug target/107844] error: argument is not a field access for __builtin_preserve_field_info

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107844

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The reason why is folded is because some folks use the null pointer for
offsetof (previously before GCC added __builtin_offsetof).

I wonder if you could use __builtin_offsetof here.
I also curious how this is implemented in clang ...

[Bug target/107844] error: argument is not a field access for __builtin_preserve_field_info

[david.faust at oracle dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107844

--- Comment #3 from David Faust <david.faust at oracle dot com> ---
Thanks for the info Andrew. I'll look at __builtin_offsetof.

As for the implementation in clang, I can point to some bits relevant to
the builtin itself:
llvm-project/clang/lib/CodeGen/CGBuiltin.cpp
  CodeGenFunction::EmitBPFBuiltinExpr ()

llvm-project/llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
  BPFAbstractMemberAccess::GetFieldInfo ()

But I am less familiar with the surrounding machinery such as their
parsing and type systems..

[Bug target/107844] error: argument is not a field access for __builtin_preserve_field_info

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107844

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-01-18

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to David Faust from comment #3)
> Thanks for the info Andrew. I'll look at __builtin_offsetof.
> 
> As for the implementation in clang, I can point to some bits relevant to
> the builtin itself:
> llvm-project/clang/lib/CodeGen/CGBuiltin.cpp
>   CodeGenFunction::EmitBPFBuiltinExpr ()
> 
> llvm-project/llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
>   BPFAbstractMemberAccess::GetFieldInfo ()
> 
> But I am less familiar with the surrounding machinery such as their
> parsing and type systems..

So I looked (First off I am shocked they don't have target functions to handle
the builtins and every target builtin is handled in that file seems wrong), and
you are handed the AST before folding. This is different from GCC where it is
you are handed it after folding.

So I think we need some special handling in the c (and C++) parser to handle
this. I suspect we want to do the full handling of the builtin
(bpf_core_field_exists) in the parser rather than the macro expanded view of it
too. Similar to how offsetof is handled ...
Of course this will need some modifications to the bpf headers too. And that
solves some other issues too.

[Bug target/107844] error: argument is not a field access for __builtin_preserve_field_info

[david.faust at oracle dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107844

--- Comment #5 from David Faust <david.faust at oracle dot com> ---
(In reply to Andrew Pinski from comment #4)
> (In reply to David Faust from comment #3)
> > Thanks for the info Andrew. I'll look at __builtin_offsetof.
> > 
> > As for the implementation in clang, I can point to some bits relevant to
> > the builtin itself:
> > llvm-project/clang/lib/CodeGen/CGBuiltin.cpp
> >   CodeGenFunction::EmitBPFBuiltinExpr ()
> > 
> > llvm-project/llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
> >   BPFAbstractMemberAccess::GetFieldInfo ()
> > 
> > But I am less familiar with the surrounding machinery such as their
> > parsing and type systems..
> 
> So I looked (First off I am shocked they don't have target functions to
> handle the builtins and every target builtin is handled in that file seems
> wrong), and you are handed the AST before folding. This is different from
> GCC where it is you are handed it after folding.

Aha! I had never realized this difference until now. Thanks for pointing
that out!

> 
> So I think we need some special handling in the c (and C++) parser to handle
> this. I suspect we want to do the full handling of the builtin
> (bpf_core_field_exists) in the parser rather than the macro expanded view of
> it too. Similar to how offsetof is handled ...
> Of course this will need some modifications to the bpf headers too. And that
> solves some other issues too.

Yes, I see. I'll have to study the parser a little since I have not touched
it before, but the approach makes sense.

I wonder if it would be feasible and/or worthwhile to add some sort of
TARGET_PARSE_BUILTIN hook to enable this sort of handling for any other
targets which may want it..? Maybe that can wait.

In any case, thank you very much for the suggestions.

[Bug target/107844] error: argument is not a field access for __builtin_preserve_field_info

[jemarch at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107844

Jose E. Marchesi <jemarch at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |cupertino.miranda at oracle dot co
                   |                            |m

[Bug target/107844] error: argument is not a field access for __builtin_preserve_field_info

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107844

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Cupertino Miranda <cupermir@gcc.gnu.org>:

https://gcc.gnu.org/g:e0a81559c198153923f0a1a3be7c25df545f3964

commit r14-2962-ge0a81559c198153923f0a1a3be7c25df545f3964
Author: Cupertino Miranda <cupertino.miranda@oracle.com>
Date:   Thu Apr 6 15:22:48 2023 +0100

    bpf: Implementation of BPF CO-RE builtins

    This patch updates the support for the BPF CO-RE builtins
    __builtin_preserve_access_index and __builtin_preserve_field_info,
    and adds support for the CO-RE builtins __builtin_btf_type_id,
    __builtin_preserve_type_info and __builtin_preserve_enum_value.

    These CO-RE relocations are now converted to __builtin_core_reloc which
    abstracts all of the original builtins in a polymorphic relocation
    specific builtin.

    The builtin processing is now split in 2 stages, the first (pack) is
    executed right after the front-end and the second (process) right before
    the asm output.

    In expand pass the __builtin_core_reloc is converted to a
    unspec:UNSPEC_CORE_RELOC rtx entry.

    The data required to process the builtin is now collected in the packing
    stage (after front-end), not allowing the compiler to optimize any of
    the relevant information required to compose the relocation when
    necessary.
    At expansion, that information is recovered and CTF/BTF is queried to
    construct the information that will be used in the relocation.
    At this point the relocation is added to specific section and the
    builtin is expanded to the expected default value for the builtin.

    In order to process __builtin_preserve_enum_value, it was necessary to
    hook the front-end to collect the original enum value reference.
    This is needed since the parser folds all the enum values to its
    integer_cst representation.

    More details can be found within the core-builtins.cc.

    Regtested in host x86_64-linux-gnu and target bpf-unknown-none.

    gcc/ChangeLog:

            PR target/107844
            PR target/107479
            PR target/107480
            PR target/107481
            * config.gcc: Added core-builtins.cc and .o files.
            * config/bpf/bpf-passes.def: Removed file.
            * config/bpf/bpf-protos.h (bpf_add_core_reloc,
            bpf_replace_core_move_operands): New prototypes.
            * config/bpf/bpf.cc (enum bpf_builtins, is_attr_preserve_access,
            maybe_make_core_relo, bpf_core_field_info, bpf_core_compute,
            bpf_core_get_index, bpf_core_new_decl, bpf_core_walk,
            bpf_is_valid_preserve_field_info_arg, is_attr_preserve_access,
            handle_attr_preserve, pass_data_bpf_core_attr, pass_bpf_core_attr):
            Removed.
            (def_builtin, bpf_expand_builtin, bpf_resolve_overloaded_builtin):
Changed.
            * config/bpf/bpf.md (define_expand mov<MM:mode>): Changed.
            (mov_reloc_core<mode>): Added.
            * config/bpf/core-builtins.cc (struct cr_builtin, enum
            cr_decision struct cr_local, struct cr_final, struct
            core_builtin_helpers, enum bpf_plugin_states): Added types.
            (builtins_data, core_builtin_helpers, core_builtin_type_defs):
            Added variables.
            (allocate_builtin_data, get_builtin-data, search_builtin_data,
            remove_parser_plugin, compare_same_kind, compare_same_ptr_expr,
            compare_same_ptr_type, is_attr_preserve_access, core_field_info,
            bpf_core_get_index, compute_field_expr,
            pack_field_expr_for_access_index,
pack_field_expr_for_preserve_field,
            process_field_expr, pack_enum_value, process_enum_value, pack_type,
            process_type, bpf_require_core_support, make_core_relo, read_kind,
            kind_access_index, kind_preserve_field_info, kind_enum_value,
            kind_type_id, kind_preserve_type_info,
get_core_builtin_fndecl_for_type,
            bpf_handle_plugin_finish_type, bpf_init_core_builtins,
            construct_builtin_core_reloc, bpf_resolve_overloaded_core_builtin,
            bpf_expand_core_builtin, bpf_add_core_reloc,
            bpf_replace_core_move_operands): Added functions.
            * config/bpf/core-builtins.h (enum bpf_builtins): Added.
            (bpf_init_core_builtins, bpf_expand_core_builtin,
            bpf_resolve_overloaded_core_builtin): Added functions.
            * config/bpf/coreout.cc (struct bpf_core_extra): Added.
            (bpf_core_reloc_add, output_asm_btfext_core_reloc): Changed.
            * config/bpf/coreout.h (bpf_core_reloc_add) Changed prototype.
            * config/bpf/t-bpf: Added core-builtins.o.
            * doc/extend.texi: Added documentation for new BPF builtins.

[Bug target/107844] error: argument is not a field access for __builtin_preserve_field_info

[jemarch at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107844

Jose E. Marchesi <jemarch at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #7 from Jose E. Marchesi <jemarch at gcc dot gnu.org> ---
This is fixed by the commit above.