From: "Jose E. Marchesi" <jose.marchesi@oracle.com>
To: Cupertino Miranda <cupertino.miranda@oracle.com>
Cc: gcc-patches@gcc.gnu.org, david.faust@oracle.com,
elena.zannoni@oracle.com
Subject: Re: [PATCH 3/3] bpf: add line_info support to BTF.ext section.
Date: Wed, 17 Apr 2024 19:48:53 +0200 [thread overview]
Message-ID: <87mspr6fsq.fsf@oracle.com> (raw)
In-Reply-To: <87mspsj8ja.fsf@oracle.com> (Cupertino Miranda's message of "Wed, 17 Apr 2024 16:47:21 +0100")
> 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.
>>> * 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 } } */
next prev parent reply other threads:[~2024-04-17 17:49 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 11:11 [PATCH 1/3] bpf: support more instructions to match CO-RE relocations Cupertino Miranda
2024-04-11 11:11 ` [PATCH 2/3] bpf: remove huge memory waste with string allocation Cupertino Miranda
2024-04-11 16:34 ` David Faust
2024-04-11 17:23 ` Cupertino Miranda
2024-04-11 11:11 ` [PATCH 3/3] bpf: add line_info support to BTF.ext section Cupertino Miranda
2024-04-17 15:14 ` Jose E. Marchesi
2024-04-17 15:47 ` Cupertino Miranda
2024-04-17 17:48 ` Jose E. Marchesi [this message]
2024-04-17 17:55 ` Jose E. Marchesi
2024-04-17 15:07 ` [PATCH 1/3] bpf: support more instructions to match CO-RE relocations Jose E. Marchesi
2024-04-19 10:17 ` Cupertino Miranda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87mspr6fsq.fsf@oracle.com \
--to=jose.marchesi@oracle.com \
--cc=cupertino.miranda@oracle.com \
--cc=david.faust@oracle.com \
--cc=elena.zannoni@oracle.com \
--cc=gcc-patches@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).