>> Jose E. Marchesi writes:
>>
>>> Hi Cuper.
>>> Thanks for the patch.
>>>
>>>> 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 core referenced errors.
>>>>
>>>> gcc/Changelog:
>>>> * 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.
>>>> (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.
>>>
>>> Is it actually necessary to emit a label (plus .BTF.ext entry) for every
>>> instruction?
>> Maybe BPF would be Ok (not complaining of missing line_info) with just a
>> single entry per function pointing to the entry instruction. That
>> is not what clang does. Don't know if it emits any labels either.
>>
>> It is probably possible to add some logic to compute the offset from
>> the function label and emit with an offset to the instruction
>> location. In case of inline assembly we could add a symbol after, and
>> restart offset computation. It will need to add code to compute the
>> instruction encoding size, and increment function label offset each
>> time we emit an instruction.
>>
>> Still, my personal preference is to create those labels to properly
>> compute instruction location then some rather intricate solution that
>> would lead to future complications. I know BPF is not like all the
>> other targets, but I am thinking of assembly/linker relaxation.
>>
>> WDYT ?
>
> What I meant is: if it is not required to emit a line_info entry for
> _every_ BPF instruction, but only for the instructions that "change" the
> current location, then we better do so?
>
> Then, regarding the labels, I assume their purpose is to get the
> assembler to fill in the `insn_off' field of the bpf_line_info in the
> .BTF.ext section:
>
> struct bpf_line_info {
> __u32 insn_off; /* [0, insn_cnt - 1] */
> __u32 file_name_off; /* offset to string table for the filename */
> __u32 line_off; /* offset to string table for the source line */
> __u32 line_col; /* line number and column number */
> };
>
> Which makes sense, since "instruction offset" is really the business of
> the assembler, not the compiler. I agree with you making it the
> compiler's business would be overcomplicated, given inline assembly and
> variable-sized BPF instructions...
>
> So, what about moving the task of creating these line_info entries
> entirely to the assembler? GCC already knows how to emit .file and .loc
> directives to track location info in DWARF.
>
> The BPF assembler could then process these and create entries in
> .BTF.ext for line_info, all the fields above: insn_off, file_name_off,
> line_off and line_col.
Regarding file_name_off, hopefully it will be possible to make the
assembler to simply expand the string table in .BTF (with the strings
read from .file directives) without having to understand/redo the whole
BTF section...
>>>> * 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/config/bpf/bpf-protos.h | 2 +-
>>>> gcc/config/bpf/bpf.cc | 103 ++++++++++++++---
>>>> 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, 203 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/gcc/config/bpf/bpf-protos.h b/gcc/config/bpf/bpf-protos.h
>>>> index b4866d34209..ddaca50af69 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 d9141dd625a..f1a8eb8d62c 100644
>>>> --- a/gcc/config/bpf/bpf.cc
>>>> +++ b/gcc/config/bpf/bpf.cc
>>>> @@ -754,14 +754,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:
>>>> {
>>>> @@ -774,26 +772,20 @@ 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);
>>>> - }
>>>> - else
>>>> - output_asm_insn ("call\t%0", &target);
>>>> + operands[target_index] = GEN_INT (TREE_INT_CST_LOW (TREE_VALUE
>>>> (attr_args)));
>>>>
>>>> + }
>>>> 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 *
>>>> @@ -1144,6 +1136,87 @@ 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;
>>>> + const char *label = NULL;
>>>> + unsigned int loc = 0;
>>>> + const char *filename = NULL;
>>>> + unsigned int line = 0;
>>>> + unsigned int column = 0;
>>>> +
>>>> + if(!btf_debuginfo_p ())
>>>> + return;
>>>
>>> I think it would be better to put this guard in bpf_asm_out_unwind_emit
>>> instead of bpf_output_line_info.
>>>
>>>> +
>>>> + 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);
>>>> + filename = LOCATION_FILE (loc);
>>>> + line = LOCATION_LINE (loc);
>>>> + column = LOCATION_COLUMN (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. */
>>>> + if (func_start_added == true)
>>>> + {
>>>> + func_start_added = false;
>>>> + return;
>>>> + }
>>>> +
>>>> + loc = INSN_LOCATION (insn);
>>>> + filename = LOCATION_FILE (loc);
>>>> + line = LOCATION_LINE (loc);
>>>> + column = LOCATION_COLUMN (loc);
>>>> +
>>>> + if (filename == NULL || 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 (filename != NULL && line != 0)
>>>> + btf_add_line_info_for (label, filename, line, 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.
>>>> + * This approach expects TARGET_EMIT_BEFORE_INSN to be returing TRUE as
>>>> + * 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 95859328d25..3fdf81b86a6 100644
>>>> --- a/gcc/config/bpf/bpf.md
>>>> +++ b/gcc/config/bpf/bpf.md
>>>> @@ -546,7 +546,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"
>>>> @@ -569,7 +569,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 ff1fd0739f1..42ec48e394e 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).
>>>> */
>>>> @@ -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 *
>>>> @@ -429,6 +451,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);
>>>> +
>>>> + 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);
>>>> +
>>>> + 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. */
>>>>
>>>> @@ -537,6 +600,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);
>>>> + elem = elem->next;
>>>> + }
>>>> + }
>>>> +
>>>> + gcc_assert (count == sec->line_info.num_info);
>>>> + sec = sec->next;
>>>> + }
>>>> +}
>>>> +
>>>> /* Output all CO-RE relocation sections. */
>>>>
>>>> static void
>>>> @@ -609,6 +714,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 b36309475c9..9c6848324e7 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 6fdd14574ec..0f1e0ad1e89 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 } } */
