Hi Cupertino,
On 2/25/26 12:30, Cupertino Miranda wrote:
> Hi everyone,
>
> Long awaited patch for review.
> Looking forward to your comments.
Comments inline below. I think the direction is good and it applies
cleanly, but there may be a bug with the encoding of the line_col field
in the lineinfo records (details below).
Thanks for working on this, it should be useful to help track down
remaining verifier issues :)
>
> Cheers,
> Cupertino
>
> ------
>
> This patch adds line_info debug information support to .BTF.ext
> sections.
>
> Line info information is used by the BPF verifier to improve error
> reporting and give more precise source code referenced errors.
>
> gcc/ChangeLog:
> PR target/113453
> * config/bpf/bpf-protos.h (bpf_output_call): Change prototype.
> * config/bpf/bpf.cc (bpf_output_call): Change to adapt operands
> and return
> the instruction template instead of emmidiatelly emit asm and
> not allow proper final expected execution flow.
nit: some weird wrapping here, also typo: emmidiatelly -> immediately
> (bpf_output_line_info): Add function to introduce line info
> entries in respective structures
> (bpf_asm_out_unwind_emit): Add function as hook to
> TARGET_ASM_UNWIND_EMIT. This hook is called before any
> instruction is emitted.
> * config/bpf/bpf.md: Change calls to bpf_output_call.
> * config/bpf/btfext-out.cc (struct btf_ext_lineinfo): Add fields
> to struct.
> (bpf_create_lineinfo, btf_add_line_info_for): Add support
> function to insert line_info data in respective structures.
> (output_btfext_line_info): Function to emit line_info data in
> .BTF.ext section.
> (btf_ext_output): Call output_btfext_line_info.
> * config/bpf/btfext-out.h: Add prototype for
> btf_add_line_info_for.
>
> gcc/testsuite/ChangeLog:
> PR target/113453
> * gcc.target/bpf/btfext-funcinfo.c: Adapt test.
Let's also add new test(s) for the lineinfo please.
> ---
> gcc/config/bpf/bpf-protos.h | 2 +-
> gcc/config/bpf/bpf.cc | 104 ++++++++++++++---
> gcc/config/bpf/bpf.md | 4 +-
> gcc/config/bpf/btfext-out.cc | 108 +++++++++++++++++-
> gcc/config/bpf/btfext-out.h | 4 +
> .../gcc.target/bpf/btfext-funcinfo.c | 3 +-
> 6 files changed, 205 insertions(+), 20 deletions(-)
>
> diff --git a/gcc/config/bpf/bpf-protos.h b/gcc/config/bpf/bpf-protos.h
> index 4a4799e9bf22..1113cc3e6d17 100644
> --- a/gcc/config/bpf/bpf-protos.h
> +++ b/gcc/config/bpf/bpf-protos.h
> @@ -23,7 +23,7 @@ along with GCC; see the file COPYING3. If not see
> /* Routines implemented in bpf.cc. */
>
> extern HOST_WIDE_INT bpf_initial_elimination_offset (int, int);
> -extern const char *bpf_output_call (rtx);
> +extern const char *bpf_output_call (const char *templ, rtx *, int
> target_index);
> extern void bpf_target_macros (cpp_reader *);
> extern void bpf_print_operand (FILE *, rtx, int);
> extern void bpf_print_operand_address (FILE *, rtx);
> diff --git a/gcc/config/bpf/bpf.cc b/gcc/config/bpf/bpf.cc
> index d91013ad140b..aa85b6888e70 100644
> --- a/gcc/config/bpf/bpf.cc
> +++ b/gcc/config/bpf/bpf.cc
> @@ -745,14 +745,12 @@ bpf_output_destructor (rtx symbol, int priority
> ATTRIBUTE_UNUSED)
> bpf.md. */
>
> const char *
> -bpf_output_call (rtx target)
> +bpf_output_call (const char *templ, rtx *operands, int target_index)
> {
> - rtx xops[1];
> -
> + rtx target = operands[target_index];
> switch (GET_CODE (target))
> {
> case CONST_INT:
> - output_asm_insn ("call\t%0", &target);
> break;
> case SYMBOL_REF:
> {
> @@ -765,26 +763,21 @@ bpf_output_call (rtx target)
> {
> tree attr_args = TREE_VALUE (attr);
>
> - xops[0] = GEN_INT (TREE_INT_CST_LOW (TREE_VALUE (attr_args)));
> - output_asm_insn ("call\t%0", xops);
> + operands[target_index] =
> + GEN_INT (TREE_INT_CST_LOW (TREE_VALUE (attr_args)));
> }
> - else
> - output_asm_insn ("call\t%0", &target);
> -
> break;
> }
> default:
> - if (TARGET_XBPF)
> - output_asm_insn ("call\t%0", &target);
> - else
> + if (!TARGET_XBPF)
> {
> error ("indirect call in function, which are not supported by eBPF");
> - output_asm_insn ("call 0", NULL);
> + operands[target_index] = GEN_INT (0);
> }
> break;
> }
>
> - return "";
> + return templ;
> }
>
> const char *
> @@ -1145,6 +1138,89 @@ bpf_debug_unwind_info ()
> #undef TARGET_DEBUG_UNWIND_INFO
> #define TARGET_DEBUG_UNWIND_INFO bpf_debug_unwind_info
>
> +/* Create a BTF.ext line_info entry. */
> +
> +static void
> +bpf_output_line_info (FILE *asm_out_file, rtx_insn *insn)
> +{
> + static unsigned int line_info_label = 1;
> + static tree cfun_decl = NULL_TREE;
> + static bool func_start_added = false;
> + static location_t prev_location = 0;
> + const char *label = NULL;
> + unsigned int loc = 0;
> + expanded_location exploc = {NULL, 0, 0, NULL, false};
> +
> + if(!btf_debuginfo_p () || flag_building_libgcc)
> + return;
> +
> + gcc_assert (insn != NULL_RTX);
> +
> + if (current_function_decl != cfun_decl
> + && GET_CODE (insn) == NOTE)
> + {
> + label = current_function_func_begin_label;
> + loc = DECL_SOURCE_LOCATION (current_function_decl);
> + exploc = expand_location (loc);
> + func_start_added = true;
> + }
> + else
> + {
> + if (GET_CODE (insn) == NOTE)
> + return;
> +
> + /* Already added a label for this location. This might not be fully
> + acurate but it is better then adding 2 entries on the same location,
> + which is imcompatible with the verifier expectations. */
nit: typos:
accurate
incompatible
> + if (func_start_added == true)
> + {
> + func_start_added = false;
> + return;
> + }
> +
> +
> + if (!INSN_HAS_LOCATION (insn)
> + || prev_location == INSN_LOCATION (insn))
> + return;
> +
> + prev_location = INSN_LOCATION (insn);
> +
> + exploc = insn_location (insn);
> +
> + if (exploc.file == NULL || exploc.line == 0)
> + return;
> +
> + char tmp_label[25];
> + sprintf (tmp_label, "LI%u", line_info_label);
> + ASM_OUTPUT_LABEL (asm_out_file, tmp_label);
> + line_info_label += 1;
> + label = const_cast<char *> (ggc_strdup (tmp_label));
> +
> + }
> +
> + cfun_decl = current_function_decl;
> +
> + if (exploc.file != NULL && exploc.line != 0)
> + btf_add_line_info_for (label, exploc.file, exploc.line, exploc.column);
> +}
> +
> +
> +/* This hook is defined as a way for BPF target to create a label before each
> + emitted instruction and emit line_info information. This data is later
> + output in .BTF.ext section.
Aha! Clever :D
> + This approach expects TARGET_EMIT_BEFORE_INSN to be returing TRUE as
nit: typo: returning
> + this function needs to be called before the instruction is emitted.
> Current
> + default behaviour returns TRUE and the hook is left undefined. */
> +
> +static void
> +bpf_asm_out_unwind_emit (FILE *asm_out_file, rtx_insn *insn)
> +{
> + bpf_output_line_info (asm_out_file, insn);
> +}
> +
> +#undef TARGET_ASM_UNWIND_EMIT
> +#define TARGET_ASM_UNWIND_EMIT bpf_asm_out_unwind_emit
> +
> /* Output assembly directives to assemble data of various sized and
> alignments. */
>
> diff --git a/gcc/config/bpf/bpf.md b/gcc/config/bpf/bpf.md
> index 0eec006d3cfe..bc739f2746c0 100644
> --- a/gcc/config/bpf/bpf.md
> +++ b/gcc/config/bpf/bpf.md
> @@ -545,7 +545,7 @@
> ;; operands[2] is next_arg_register
> ;; operands[3] is struct_value_size_rtx.
> ""
> - { return bpf_output_call (operands[0]); }
> + { return bpf_output_call ("call\t%0", operands, 0); }
> [(set_attr "type" "jmp")])
>
> (define_expand "call_value"
> @@ -568,7 +568,7 @@
> ;; operands[3] is next_arg_register
> ;; operands[4] is struct_value_size_rtx.
> ""
> - { return bpf_output_call (operands[1]); }
> + { return bpf_output_call ("call\t%1", operands, 1); }
> [(set_attr "type" "jmp")])
>
> (define_insn "sibcall"
> diff --git a/gcc/config/bpf/btfext-out.cc b/gcc/config/bpf/btfext-out.cc
> index 04bc60d69b0e..4f267e4059b6 100644
> --- a/gcc/config/bpf/btfext-out.cc
> +++ b/gcc/config/bpf/btfext-out.cc
> @@ -32,6 +32,7 @@
> #include "rtl.h"
> #include "tree-pretty-print.h"
> #include "cgraph.h"
> +#include "toplev.h" /* get_src_pwd */
>
> #include "btfext-out.h"
>
> @@ -124,7 +125,8 @@ struct GTY ((chain_next ("%h.next"))) btf_ext_funcinfo
> /* A lineinfo record, in the .BTF.ext lineinfo section. */
> struct GTY ((chain_next ("%h.next"))) btf_ext_lineinfo
> {
> - uint32_t insn_off; /* Offset of the instruction. */
> + const char *insn_label; /* Instruction label. */
> + const char *file_name; /* Source-code file name. */
> uint32_t file_name_off; /* Offset of file name in BTF string table. */
> uint32_t line_off; /* Offset of source line in BTF string table. */
> uint32_t line_col; /* Line number (bits 31-11) and column (11-0). */
ah, I think the comment for line_col here is wrong; see below.
Basically the line_col is line (upper 22 bits) and column is lower 10.
> @@ -235,6 +237,26 @@ bpf_create_or_find_funcinfo (const char *fnname, const
> char *sec_name,
> return *head;
> }
>
> +/* Function to create a lineinfo node in info. */
> +
> +static struct btf_ext_lineinfo *
> +bpf_create_lineinfo (const char *sec_name, btf_ext_info_sec **in_sec = NULL)
> +{
> + struct btf_ext_info_sec *sec_elem =
> + btfext_info_sec_find_or_add (sec_name, true);
> +
> + if (in_sec != NULL)
> + *in_sec = sec_elem;
> +
> + struct btf_ext_lineinfo **head =
> + SEARCH_NODE_AND_RETURN(struct btf_ext_lineinfo,
> + sec_elem->line_info.head,
> + false);
> + *head = ggc_cleared_alloc<struct btf_ext_lineinfo> ();
> +
> + return *head;
> +}
> +
> /* Function to create a core_reloc node in info. */
>
> static struct btf_ext_core_reloc *
> @@ -438,6 +460,47 @@ btf_validate_funcinfo (btf_ext_info_sec *sec)
> }
> }
>
> +struct btf_ext_lineinfo *
> +btf_add_line_info_for (const char *label, const char *filename,
> + unsigned int line, unsigned int column)
> +{
> + const char *sec_name = decl_section_name (current_function_decl);
> +
> + if (sec_name == NULL)
> + sec_name = ".text";
> +
> + struct btf_ext_info_sec *sec = NULL;
> + struct btf_ext_lineinfo *info =
> + bpf_create_lineinfo (sec_name, &sec);
> +
> + unsigned int line_column = ((0x000fffff & line) << 12)
> + | (0x00000fff & column);
Let's define macros for this in btfext-out.h, along the lines
of the ones in gcc/include/btf.h for packing/extracting bits from
the BTF info records.
But... I don't think these masks/shifts are correct.
I applied the patch locally and compiled a small test program which the
verifier rejects, and the source line number reported by the verifier is
way off from where the offending instruction corresponds to.
The source file is ~100 lines total, but the 'bad' line is reported as
coming from line number 372. I checked .BTF.ext section using poke, and
the records are consistent with what the verifier reports.
BUT the lines recorded in the -dA strings do not match the numbers
in the lineinfo records, and they actually look more correct.
My understanding is the line number and column are packed
like (using some poke syntax which I hope is clear):
struct uint<32>
{
uint<22> line_num;
uint<10> column;
}
i.e. the line number is the upper 22 bits, and the column is
the lower 10.
I see also in the kernel's Documentation/bpf/btf.rst:
For line_info, the line number and column number are defined as below:
::
#define BPF_LINE_INFO_LINE_NUM(line_col) ((line_col) >> 10)
#define BPF_LINE_INFO_LINE_COL(line_col) ((line_col) & 0x3ff)
> +
> + info->insn_label = label;
> +
> + if (!IS_DIR_SEPARATOR (filename[0]))
> + {
> + char full_filename[256];
> +
> + /* Filename is a relative path. */
> + const char * cu_pwd = get_src_pwd ();
> + gcc_assert (strlen (cu_pwd) + strlen (filename) + 2 < 256);
comparing to sizeof(full_filename) or using a #define for the size
of the buffer would be preferable so we only have to change it in
one spot.
> +
> + sprintf (full_filename, "%s%c%s", cu_pwd, DIR_SEPARATOR, filename);
> + info->file_name = ggc_strdup (full_filename);
> + }
> + else
> + /* Filename is an absolute path. */
> + info->file_name = ggc_strdup (filename);
> +
> + info->file_name_off = btf_ext_add_string (info->file_name);
> + info->line_off = 0;
> + info->line_col = line_column;
> +
> + sec->line_info.num_info += 1;
> + return info;
> +}
> +
> /* Compute the section size in section for func_info, line_info and core_info
> regions of .BTF.ext. */
>
> @@ -545,6 +608,48 @@ output_btfext_func_info (struct btf_ext_info_sec *sec)
> }
> }
>
> +/* Outputs line_info region on .BTF.ext. */
> +
> +static void
> +output_btfext_line_info (struct btf_ext_info_sec *sec)
> +{
> + unsigned int str_aux_off = ctfc_get_strtab_len (ctf_get_tu_ctfc (),
> + CTF_STRTAB);
> + bool executed = false;
> + while (sec != NULL)
> + {
> + uint32_t count = 0;
> + if (sec->line_info.num_info > 0)
> + {
> + if (executed == false && (executed = true))
> + dw2_asm_output_data (4, 16, "LineInfo entry size");
> + dw2_asm_output_data (4, sec->sec_name_off + str_aux_off,
> + "LineInfo section string for %s",
> + sec->sec_name);
> + dw2_asm_output_data (4, sec->line_info.num_info, "Number of entries");
> +
> + struct btf_ext_lineinfo *elem = sec->line_info.head;
> + while (elem != NULL)
> + {
> + count += 1;
> + dw2_asm_output_offset (4, elem->insn_label, NULL, "insn_label");
> +
> + unsigned int file_name_off = btf_ext_add_string (elem->file_name);
> + dw2_asm_output_data (4, file_name_off + str_aux_off,
> + "file_name_off");
> + dw2_asm_output_data (4, elem->line_off, "line_off");
> + dw2_asm_output_data (4, elem->line_col, "(line, col) (%u, %u)",
> + elem->line_col >> 12,
> + elem->line_col & 0x00000fff);
Similar to above, let's define and use a macro for these bit extractions.
> + elem = elem->next;
> + }
> + }
> +
> + gcc_assert (count == sec->line_info.num_info);
> + sec = sec->next;
> + }
> +}
> +
> /* Output all CO-RE relocation sections. */
>
> static void
> @@ -622,6 +727,7 @@ btf_ext_output (void)
>
> output_btfext_header ();
> output_btfext_func_info (btf_ext);
> + output_btfext_line_info (btf_ext);
> if (TARGET_BPF_CORE)
> output_btfext_core_sections ();
>
> diff --git a/gcc/config/bpf/btfext-out.h b/gcc/config/bpf/btfext-out.h
> index a4c87ffd1f02..fe9b3768643e 100644
> --- a/gcc/config/bpf/btfext-out.h
> +++ b/gcc/config/bpf/btfext-out.h
> @@ -99,6 +99,10 @@ extern int bpf_core_get_sou_member_index
> (ctf_container_ref, const tree);
>
> struct btf_ext_funcinfo *btf_add_func_info_for (tree decl,
> const char *label);
> +struct btf_ext_lineinfo *
> +btf_add_line_info_for (const char *label, const char *filename,
> + unsigned int line, unsigned int column);
> +
> unsigned int btf_ext_add_string (const char *str);
>
> #ifdef __cplusplus
> diff --git a/gcc/testsuite/gcc.target/bpf/btfext-funcinfo.c
> b/gcc/testsuite/gcc.target/bpf/btfext-funcinfo.c
> index fbbefeae68f0..f7f5b44b1ff4 100644
> --- a/gcc/testsuite/gcc.target/bpf/btfext-funcinfo.c
> +++ b/gcc/testsuite/gcc.target/bpf/btfext-funcinfo.c
> @@ -39,6 +39,5 @@ int bar_func (struct T *t)
> /* { dg-final { scan-assembler-times "ascii \"bar_sec.0\"\[\t
> \]+\[^\n\]*btf_aux_string" 1 } } */
> /* { dg-final { scan-assembler-times "FuncInfo entry size" 1 } } */
>
> -/* { dg-final { scan-assembler-times ".4byte\t0x1\t# Number of entries" 3 }
> } */
> -/* { dg-final { scan-assembler-times ".4byte\t0x2\t# Number of entries" 1 }
> } */
> +/* { dg-final { scan-assembler-times "FuncInfo
> section\[^\n\]*\n\[^0\]*0x1\t# Number of entries" 2 } } */
> /* { dg-final { scan-assembler-times "Required padding" 1 } } */
