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 ?
>> * 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 } } */
