[RFC dwarves 0/3] dwarves: improvements/fixes to BTF function skip logic

[RFC dwarves 0/3] dwarves: improvements/fixes to BTF function skip logic

[Alan Maguire]

As discussed in [1], there are a few issues with how we determine
whether to skip functions for BTF encoding:

- when detecting unexpected registers, functions which have
  struct parameters need to be skipped as they can use
  multiple registers to pass the struct, and as a result
  later parameters use unexpected registers.  However,
  struct detection does not always work; it needs to be fixed for
  const struct parameters and cases where a parameter references
  the original parameter (which has the type info) via abstract
  origin (patch 1)
- when looking for unexpected registers, location lists are not
  supported.  Fix that by using dwarf_getlocations() (patch 2).
- when marking parameters as using unexpected registers, we should
  stick to the case where we expect register x and register y is
  used; other cases such as optimized-out parameters are no
  guarantee that we were not _passed_ the correct parameters
  (patch 3).

This series can be applied on top of the dwarves "next" branch,
as a follow-on to [2]

[1] https://lore.kernel.org/bpf/20230220190335.bk6jzayfqivsh7rv@macbook-pro-6.dhcp.thefacebook.com/
[2] https://lore.kernel.org/bpf/1676675433-10583-1-git-send-email-alan.maguire@oracle.com/

Alan Maguire (3):
  dwarf_loader: fix detection of struct parameters
  dwarf_loader: fix parameter location retrieval for location lists
  dwarf_loader: only mark parameter as using an unexpected register when
    it does

 dwarf_loader.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

-- 
2.31.1

[RFC dwarves 1/3] dwarf_loader: fix detection of struct parameters

[Alan Maguire]

In some cases, param__is_struct() was failing to notice that
a parameter was a struct.  The first was where a parameter
was a const struct; the second was where the type information
was in the original subroutine information, and additional
parameters that referred to it via abstract origin did not
also specify type information.  We combine information
about type, name etc in ftype__recode_dwarf_types(), but
since we share the tag->type (rather than the dwarf tag),
param__is_struct() was failing to handle this case as
it represents parameters like this:

    <7e0f7d4>   DW_AT_sibling     : <0x7e0f924>
 <2><7e0f7d8>: Abbrev Number: 7 (DW_TAG_formal_parameter)
    <7e0f7d9>   DW_AT_abstract_origin: <0x7e0dc80>
    <7e0f7dd>   DW_AT_location    : 0x2797488 (location list)
    <7e0f7e1>   DW_AT_GNU_locviews: 0x2797484
 <2><7e0f7e5>: Abbrev Number: 7 (DW_TAG_formal_parameter)
    <7e0f7e6>   DW_AT_abstract_origin: <0x7e0dc8b>
    <7e0f7ea>   DW_AT_location    : 0x27974ca (location list)
    <7e0f7ee>   DW_AT_GNU_locviews: 0x27974c8

...which do not specify a type and did not use the tag->type
information.

Fix param__is_struct() to use cu__type(cu, tag->type)
to look up type information, and to handle the const case.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 dwarf_loader.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 014e130..73e3670 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -2645,19 +2645,17 @@ out:
 
 static bool param__is_struct(struct cu *cu, struct tag *tag)
 {
-	const struct dwarf_tag *dtag = tag->priv;
-	struct dwarf_tag *dtype = dwarf_cu__find_type_by_ref(cu->priv, &dtag->type);
-	struct tag *type;
+	struct tag *type = cu__type(cu, tag->type);
 
-	if (!dtype)
+	if (!type)
 		return false;
-	type = dtype->tag;
 
 	switch (type->tag) {
 	case DW_TAG_structure_type:
 		return true;
+	case DW_TAG_const_type:
 	case DW_TAG_typedef:
-		/* handle "typedef struct" */
+		/* handle "typedef struct", const parameter */
 		return param__is_struct(cu, type);
 	default:
 		return false;
-- 
2.31.1

[RFC dwarves 2/3] dwarf_loader: fix parameter location retrieval for location lists

[Alan Maguire]

dwarf_getlocation() does not work for location lists; use
dwarf_getlocations() instead.  For parameters we are
only interested in the first expr - the location on function
entry.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 dwarf_loader.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 73e3670..6ae0954 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -8,6 +8,7 @@
 #include <dirent.h>
 #include <dwarf.h>
 #include <elfutils/libdwfl.h>
+#include <elfutils/version.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <fnmatch.h>
@@ -1073,6 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
 	struct parameter *parm = tag__alloc(cu, sizeof(*parm));
 
 	if (parm != NULL) {
+		Dwarf_Addr base, start, end;
 		bool has_const_value;
 		Dwarf_Attribute attr;
 		struct location loc;
@@ -1115,10 +1117,18 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
 		 * between these parameter representations.  See
 		 * ftype__recode_dwarf_types() below for how this is handled.
 		 */
-		parm->has_loc = dwarf_attr(die, DW_AT_location, &attr) != NULL;
 		has_const_value = dwarf_attr(die, DW_AT_const_value, &attr) != NULL;
+		parm->has_loc = dwarf_attr(die, DW_AT_location, &attr) != NULL;
+		/* dwarf_getlocations() handles location lists; here we are
+		 * only interested in the first expr.
+		 */
 		if (parm->has_loc &&
-		    attr_location(die, &loc.expr, &loc.exprlen) == 0 &&
+#if _ELFUTILS_PREREQ(0, 157)
+		    dwarf_getlocations(&attr, 0, &base, &start, &end,
+				       &loc.expr, &loc.exprlen) > 0 &&
+#else
+		    dwarf_getlocation(&attr, &loc.expr, &loc.exprlen) == 0 &&
+#endif
 			loc.exprlen != 0) {
 			int expected_reg = cu->register_params[param_idx];
 			Dwarf_Op *expr = loc.expr;
-- 
2.31.1

[RFC dwarves 3/3] dwarf_loader: only mark parameter as using an unexpected register when it does

[Alan Maguire]

It is important to distinguish between cases where a parameter uses
an unexpected register versus cases where it is optimized out;
the function may still be called with the right parameter values
in the latter case, they just are not used.

Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 dwarf_loader.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 6ae0954..791845a 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -1144,14 +1144,12 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
 				if (expected_reg >= 0 && expected_reg != expr->atom)
 					parm->unexpected_reg = 1;
 				break;
-			case DW_OP_breg0 ... DW_OP_breg31:
-				break;
 			default:
-				parm->unexpected_reg = 1;
+				parm->optimized = 1;
 				break;
 			}
 		} else if (has_const_value) {
-			parm->unexpected_reg = 1;
+			parm->optimized = 1;
 		}
 	}
 
-- 
2.31.1

Re: [RFC dwarves 0/3] dwarves: improvements/fixes to BTF function skip logic

[Jiri Olsa]

On Tue, Feb 21, 2023 at 03:48:39PM +0000, Alan Maguire wrote:
> As discussed in [1], there are a few issues with how we determine
> whether to skip functions for BTF encoding:
> 
> - when detecting unexpected registers, functions which have
>   struct parameters need to be skipped as they can use
>   multiple registers to pass the struct, and as a result
>   later parameters use unexpected registers.  However,
>   struct detection does not always work; it needs to be fixed for
>   const struct parameters and cases where a parameter references
>   the original parameter (which has the type info) via abstract
>   origin (patch 1)
> - when looking for unexpected registers, location lists are not
>   supported.  Fix that by using dwarf_getlocations() (patch 2).
> - when marking parameters as using unexpected registers, we should
>   stick to the case where we expect register x and register y is
>   used; other cases such as optimized-out parameters are no
>   guarantee that we were not _passed_ the correct parameters
>   (patch 3).
> 
> This series can be applied on top of the dwarves "next" branch,
> as a follow-on to [2]
> 
> [1] https://lore.kernel.org/bpf/20230220190335.bk6jzayfqivsh7rv@macbook-pro-6.dhcp.thefacebook.com/
> [2] https://lore.kernel.org/bpf/1676675433-10583-1-git-send-email-alan.maguire@oracle.com/
> 
> Alan Maguire (3):
>   dwarf_loader: fix detection of struct parameters
>   dwarf_loader: fix parameter location retrieval for location lists
>   dwarf_loader: only mark parameter as using an unexpected register when
>     it does

I'm getting more functions in with this patchset

  1666 for gcc   (with 61678, without 60012)
  9390 for clang (with 62128, without 52738)

but no duplicates and selftests are passing

Tested-by: Jiri Olsa <jolsa@kernel.org>

jirka

> 
>  dwarf_loader.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [RFC dwarves 0/3] dwarves: improvements/fixes to BTF function skip logic

[Arnaldo Carvalho de Melo]

Em Thu, Feb 23, 2023 at 11:10:07PM +0100, Jiri Olsa escreveu:
> On Tue, Feb 21, 2023 at 03:48:39PM +0000, Alan Maguire wrote:
> > As discussed in [1], there are a few issues with how we determine
> > whether to skip functions for BTF encoding:
> > 
> > - when detecting unexpected registers, functions which have
> >   struct parameters need to be skipped as they can use
> >   multiple registers to pass the struct, and as a result
> >   later parameters use unexpected registers.  However,
> >   struct detection does not always work; it needs to be fixed for
> >   const struct parameters and cases where a parameter references
> >   the original parameter (which has the type info) via abstract
> >   origin (patch 1)
> > - when looking for unexpected registers, location lists are not
> >   supported.  Fix that by using dwarf_getlocations() (patch 2).
> > - when marking parameters as using unexpected registers, we should
> >   stick to the case where we expect register x and register y is
> >   used; other cases such as optimized-out parameters are no
> >   guarantee that we were not _passed_ the correct parameters
> >   (patch 3).
> > 
> > This series can be applied on top of the dwarves "next" branch,
> > as a follow-on to [2]
> > 
> > [1] https://lore.kernel.org/bpf/20230220190335.bk6jzayfqivsh7rv@macbook-pro-6.dhcp.thefacebook.com/
> > [2] https://lore.kernel.org/bpf/1676675433-10583-1-git-send-email-alan.maguire@oracle.com/
> > 
> > Alan Maguire (3):
> >   dwarf_loader: fix detection of struct parameters
> >   dwarf_loader: fix parameter location retrieval for location lists
> >   dwarf_loader: only mark parameter as using an unexpected register when
> >     it does
> 
> I'm getting more functions in with this patchset
> 
>   1666 for gcc   (with 61678, without 60012)
>   9390 for clang (with 62128, without 52738)
> 
> but no duplicates and selftests are passing
> 
> Tested-by: Jiri Olsa <jolsa@kernel.org>

Oh, I saw this and your comment about not using the branch 'next' and
thought these were already there :-\

I reviewed the patches now and I'm testing them, soon will push to the
'next' branch for some testing on the libbpf CI.

- Arnaldo