Re: [PATCH 3/3] bpf: add line_info support to BTF.ext section.

["Jose E. Marchesi" <jose.marchesi@oracle.com>]

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?

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