diff mbox series

btf_encoder: Add extra checks for symbol names

Message ID 20210112184004.1302879-1-jolsa@kernel.org (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series btf_encoder: Add extra checks for symbol names | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Jiri Olsa Jan. 12, 2021, 6:40 p.m. UTC
When processing kernel image build by clang we can
find some functions without the name, which causes
pahole to segfault.

Adding extra checks to make sure we always have
function's name defined before using it.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Andrii Nakryiko Jan. 12, 2021, 7:20 p.m. UTC | #1
On Tue, Jan 12, 2021 at 10:43 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> When processing kernel image build by clang we can
> find some functions without the name, which causes
> pahole to segfault.
>
> Adding extra checks to make sure we always have
> function's name defined before using it.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  btf_encoder.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 333973054b61..17f7a14f2ef0 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
>
>         if (elf_sym__type(sym) != STT_FUNC)
>                 return 0;
> +       if (!elf_sym__name(sym, btfe->symtab))
> +               return 0;

elf_sym__name() is called below again, so might be better to just use
local variable to store result?

>
>         if (functions_cnt == functions_alloc) {
>                 functions_alloc = max(1000, functions_alloc * 3 / 2);
> @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>                 if (!has_arg_names(cu, &fn->proto))
>                         continue;
>                 if (functions_cnt) {
> -                       struct elf_function *func;
> +                       const char *name = function__name(fn, cu);
> +                       struct elf_function *func = NULL;
>
> -                       func = find_function(btfe, function__name(fn, cu));
> +                       if (name)
> +                               func = find_function(btfe, name);

isn't this a more convoluted way of writing:

name = function__name(fn, cu);
if (!name)
    continue;

func = find_function(btfe, name);
if (!func || func->generated)
    continue

?

>                         if (!func || func->generated)
>                                 continue;
>                         func->generated = true;
> --
> 2.26.2
>
Jiri Olsa Jan. 12, 2021, 7:47 p.m. UTC | #2
On Tue, Jan 12, 2021 at 11:20:44AM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 12, 2021 at 10:43 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > When processing kernel image build by clang we can
> > find some functions without the name, which causes
> > pahole to segfault.
> >
> > Adding extra checks to make sure we always have
> > function's name defined before using it.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  btf_encoder.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 333973054b61..17f7a14f2ef0 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> >
> >         if (elf_sym__type(sym) != STT_FUNC)
> >                 return 0;
> > +       if (!elf_sym__name(sym, btfe->symtab))
> > +               return 0;
> 
> elf_sym__name() is called below again, so might be better to just use
> local variable to store result?

right, will add

> 
> >
> >         if (functions_cnt == functions_alloc) {
> >                 functions_alloc = max(1000, functions_alloc * 3 / 2);
> > @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >                 if (!has_arg_names(cu, &fn->proto))
> >                         continue;
> >                 if (functions_cnt) {
> > -                       struct elf_function *func;
> > +                       const char *name = function__name(fn, cu);
> > +                       struct elf_function *func = NULL;
> >
> > -                       func = find_function(btfe, function__name(fn, cu));
> > +                       if (name)
> > +                               func = find_function(btfe, name);
> 
> isn't this a more convoluted way of writing:
> 
> name = function__name(fn, cu);
> if (!name)
>     continue;
> 
> func = find_function(btfe, name);
> if (!func || func->generated)
>     continue
> 
> ?

convoluted is my middle name ;-) I'll change it

thanks,
jirka

> 
> >                         if (!func || func->generated)
> >                                 continue;
> >                         func->generated = true;
> > --
> > 2.26.2
> >
>
Sedat Dilek Jan. 12, 2021, 8:17 p.m. UTC | #3
On Tue, Jan 12, 2021 at 8:47 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Jan 12, 2021 at 11:20:44AM -0800, Andrii Nakryiko wrote:
> > On Tue, Jan 12, 2021 at 10:43 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > When processing kernel image build by clang we can
> > > find some functions without the name, which causes
> > > pahole to segfault.
> > >
> > > Adding extra checks to make sure we always have
> > > function's name defined before using it.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  btf_encoder.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > index 333973054b61..17f7a14f2ef0 100644
> > > --- a/btf_encoder.c
> > > +++ b/btf_encoder.c
> > > @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > >
> > >         if (elf_sym__type(sym) != STT_FUNC)
> > >                 return 0;
> > > +       if (!elf_sym__name(sym, btfe->symtab))
> > > +               return 0;
> >
> > elf_sym__name() is called below again, so might be better to just use
> > local variable to store result?
>
> right, will add
>
> >
> > >
> > >         if (functions_cnt == functions_alloc) {
> > >                 functions_alloc = max(1000, functions_alloc * 3 / 2);
> > > @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > >                 if (!has_arg_names(cu, &fn->proto))
> > >                         continue;
> > >                 if (functions_cnt) {
> > > -                       struct elf_function *func;
> > > +                       const char *name = function__name(fn, cu);
> > > +                       struct elf_function *func = NULL;
> > >
> > > -                       func = find_function(btfe, function__name(fn, cu));
> > > +                       if (name)
> > > +                               func = find_function(btfe, name);
> >
> > isn't this a more convoluted way of writing:
> >
> > name = function__name(fn, cu);
> > if (!name)
> >     continue;
> >
> > func = find_function(btfe, name);
> > if (!func || func->generated)
> >     continue
> >
> > ?
>
> convoluted is my middle name ;-) I'll change it
>

OK, a v2 will follow.

Thanks JCO.

- sed@ -

> thanks,
> jirka
>
> >
> > >                         if (!func || func->generated)
> > >                                 continue;
> > >                         func->generated = true;
> > > --
> > > 2.26.2
> > >
> >
>
Tom Stellard Jan. 13, 2021, 12:27 a.m. UTC | #4
On 1/12/21 10:40 AM, Jiri Olsa wrote:
> When processing kernel image build by clang we can
> find some functions without the name, which causes
> pahole to segfault.
> 
> Adding extra checks to make sure we always have
> function's name defined before using it.
> 

I backported this patch to pahole 1.19, and I can confirm it fixes the 
segfault for me.

-Tom

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   btf_encoder.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 333973054b61..17f7a14f2ef0 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
>   
>   	if (elf_sym__type(sym) != STT_FUNC)
>   		return 0;
> +	if (!elf_sym__name(sym, btfe->symtab))
> +		return 0;
>   
>   	if (functions_cnt == functions_alloc) {
>   		functions_alloc = max(1000, functions_alloc * 3 / 2);
> @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>   		if (!has_arg_names(cu, &fn->proto))
>   			continue;
>   		if (functions_cnt) {
> -			struct elf_function *func;
> +			const char *name = function__name(fn, cu);
> +			struct elf_function *func = NULL;
>   
> -			func = find_function(btfe, function__name(fn, cu));
> +			if (name)
> +				func = find_function(btfe, name);
>   			if (!func || func->generated)
>   				continue;
>   			func->generated = true;
>
Sedat Dilek Jan. 14, 2021, 7:50 a.m. UTC | #5
On Wed, Jan 13, 2021 at 1:28 AM Tom Stellard <tstellar@redhat.com> wrote:
>
> On 1/12/21 10:40 AM, Jiri Olsa wrote:
> > When processing kernel image build by clang we can
> > find some functions without the name, which causes
> > pahole to segfault.
> >
> > Adding extra checks to make sure we always have
> > function's name defined before using it.
> >
>
> I backported this patch to pahole 1.19, and I can confirm it fixes the
> segfault for me.
>

Thanks for testing.

Can you give me Git commit-id of LLVM-12 you tried?

- Sedat -

> -Tom
>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   btf_encoder.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 333973054b61..17f7a14f2ef0 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> >
> >       if (elf_sym__type(sym) != STT_FUNC)
> >               return 0;
> > +     if (!elf_sym__name(sym, btfe->symtab))
> > +             return 0;
> >
> >       if (functions_cnt == functions_alloc) {
> >               functions_alloc = max(1000, functions_alloc * 3 / 2);
> > @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >               if (!has_arg_names(cu, &fn->proto))
> >                       continue;
> >               if (functions_cnt) {
> > -                     struct elf_function *func;
> > +                     const char *name = function__name(fn, cu);
> > +                     struct elf_function *func = NULL;
> >
> > -                     func = find_function(btfe, function__name(fn, cu));
> > +                     if (name)
> > +                             func = find_function(btfe, name);
> >                       if (!func || func->generated)
> >                               continue;
> >                       func->generated = true;
> >
>
Tom Stellard Jan. 14, 2021, 2:33 p.m. UTC | #6
On 1/13/21 11:50 PM, Sedat Dilek wrote:
> On Wed, Jan 13, 2021 at 1:28 AM Tom Stellard <tstellar@redhat.com> wrote:
>>
>> On 1/12/21 10:40 AM, Jiri Olsa wrote:
>>> When processing kernel image build by clang we can
>>> find some functions without the name, which causes
>>> pahole to segfault.
>>>
>>> Adding extra checks to make sure we always have
>>> function's name defined before using it.
>>>
>>
>> I backported this patch to pahole 1.19, and I can confirm it fixes the
>> segfault for me.
>>
> 
> Thanks for testing.
> 
> Can you give me Git commit-id of LLVM-12 you tried?
> 

I was building with LLVM 11.0.1

-Tom

> - Sedat -
> 
>> -Tom
>>
>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>> ---
>>>    btf_encoder.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/btf_encoder.c b/btf_encoder.c
>>> index 333973054b61..17f7a14f2ef0 100644
>>> --- a/btf_encoder.c
>>> +++ b/btf_encoder.c
>>> @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
>>>
>>>        if (elf_sym__type(sym) != STT_FUNC)
>>>                return 0;
>>> +     if (!elf_sym__name(sym, btfe->symtab))
>>> +             return 0;
>>>
>>>        if (functions_cnt == functions_alloc) {
>>>                functions_alloc = max(1000, functions_alloc * 3 / 2);
>>> @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>>>                if (!has_arg_names(cu, &fn->proto))
>>>                        continue;
>>>                if (functions_cnt) {
>>> -                     struct elf_function *func;
>>> +                     const char *name = function__name(fn, cu);
>>> +                     struct elf_function *func = NULL;
>>>
>>> -                     func = find_function(btfe, function__name(fn, cu));
>>> +                     if (name)
>>> +                             func = find_function(btfe, name);
>>>                        if (!func || func->generated)
>>>                                continue;
>>>                        func->generated = true;
>>>
>>
>
Sedat Dilek Jan. 14, 2021, 2:39 p.m. UTC | #7
On Thu, Jan 14, 2021 at 3:33 PM Tom Stellard <tstellar@redhat.com> wrote:
>
> On 1/13/21 11:50 PM, Sedat Dilek wrote:
> > On Wed, Jan 13, 2021 at 1:28 AM Tom Stellard <tstellar@redhat.com> wrote:
> >>
> >> On 1/12/21 10:40 AM, Jiri Olsa wrote:
> >>> When processing kernel image build by clang we can
> >>> find some functions without the name, which causes
> >>> pahole to segfault.
> >>>
> >>> Adding extra checks to make sure we always have
> >>> function's name defined before using it.
> >>>
> >>
> >> I backported this patch to pahole 1.19, and I can confirm it fixes the
> >> segfault for me.
> >>
> >
> > Thanks for testing.
> >
> > Can you give me Git commit-id of LLVM-12 you tried?
> >
>
> I was building with LLVM 11.0.1
>

Thanks Tom.

- Sedat -

> -Tom
>
> > - Sedat -
> >
> >> -Tom
> >>
> >>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >>> ---
> >>>    btf_encoder.c | 8 ++++++--
> >>>    1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/btf_encoder.c b/btf_encoder.c
> >>> index 333973054b61..17f7a14f2ef0 100644
> >>> --- a/btf_encoder.c
> >>> +++ b/btf_encoder.c
> >>> @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> >>>
> >>>        if (elf_sym__type(sym) != STT_FUNC)
> >>>                return 0;
> >>> +     if (!elf_sym__name(sym, btfe->symtab))
> >>> +             return 0;
> >>>
> >>>        if (functions_cnt == functions_alloc) {
> >>>                functions_alloc = max(1000, functions_alloc * 3 / 2);
> >>> @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >>>                if (!has_arg_names(cu, &fn->proto))
> >>>                        continue;
> >>>                if (functions_cnt) {
> >>> -                     struct elf_function *func;
> >>> +                     const char *name = function__name(fn, cu);
> >>> +                     struct elf_function *func = NULL;
> >>>
> >>> -                     func = find_function(btfe, function__name(fn, cu));
> >>> +                     if (name)
> >>> +                             func = find_function(btfe, name);
> >>>                        if (!func || func->generated)
> >>>                                continue;
> >>>                        func->generated = true;
> >>>
> >>
> >
>
Arnaldo Carvalho de Melo Jan. 21, 2021, 1:38 p.m. UTC | #8
Em Tue, Jan 12, 2021 at 04:27:59PM -0800, Tom Stellard escreveu:
> On 1/12/21 10:40 AM, Jiri Olsa wrote:
> > When processing kernel image build by clang we can
> > find some functions without the name, which causes
> > pahole to segfault.
> > 
> > Adding extra checks to make sure we always have
> > function's name defined before using it.
> > 
> 
> I backported this patch to pahole 1.19, and I can confirm it fixes the
> segfault for me.

I'm applying v2 for this patch and based on your above statement I'm
adding a:

Tested-by: Tom Stellard <tstellar@redhat.com>

Ok?

Who originally reported this?

- Arnaldo
 
> -Tom
> 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   btf_encoder.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 333973054b61..17f7a14f2ef0 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> >   	if (elf_sym__type(sym) != STT_FUNC)
> >   		return 0;
> > +	if (!elf_sym__name(sym, btfe->symtab))
> > +		return 0;
> >   	if (functions_cnt == functions_alloc) {
> >   		functions_alloc = max(1000, functions_alloc * 3 / 2);
> > @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >   		if (!has_arg_names(cu, &fn->proto))
> >   			continue;
> >   		if (functions_cnt) {
> > -			struct elf_function *func;
> > +			const char *name = function__name(fn, cu);
> > +			struct elf_function *func = NULL;
> > -			func = find_function(btfe, function__name(fn, cu));
> > +			if (name)
> > +				func = find_function(btfe, name);
> >   			if (!func || func->generated)
> >   				continue;
> >   			func->generated = true;
> > 
>
Sedat Dilek Jan. 21, 2021, 4:06 p.m. UTC | #9
On Thu, Jan 21, 2021 at 2:38 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Tue, Jan 12, 2021 at 04:27:59PM -0800, Tom Stellard escreveu:
> > On 1/12/21 10:40 AM, Jiri Olsa wrote:
> > > When processing kernel image build by clang we can
> > > find some functions without the name, which causes
> > > pahole to segfault.
> > >
> > > Adding extra checks to make sure we always have
> > > function's name defined before using it.
> > >
> >
> > I backported this patch to pahole 1.19, and I can confirm it fixes the
> > segfault for me.
>
> I'm applying v2 for this patch and based on your above statement I'm
> adding a:
>
> Tested-by: Tom Stellard <tstellar@redhat.com>
>
> Ok?
>
> Who originally reported this?
>

The origin was AFAICS the thread where I asked initially [1].

Tom reported in the same thread in [2] that pahole segfaults.

Later in the thread Jiri offered a draft of this patch after doing some tests.

I have tested all diffs and v1 and v2 of Jiri's patch.
( Anyway, latest pahole ToT plus Jiri's patch did not solve my origin problem. )

So up to you Arnaldo for the credits.

- Sedat -

[1] https://marc.info/?t=161036949500004&r=1&w=2
[2] https://marc.info/?t=161036949500004&r=1&w=2

> - Arnaldo
>
> > -Tom
> >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >   btf_encoder.c | 8 ++++++--
> > >   1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > index 333973054b61..17f7a14f2ef0 100644
> > > --- a/btf_encoder.c
> > > +++ b/btf_encoder.c
> > > @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > >     if (elf_sym__type(sym) != STT_FUNC)
> > >             return 0;
> > > +   if (!elf_sym__name(sym, btfe->symtab))
> > > +           return 0;
> > >     if (functions_cnt == functions_alloc) {
> > >             functions_alloc = max(1000, functions_alloc * 3 / 2);
> > > @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > >             if (!has_arg_names(cu, &fn->proto))
> > >                     continue;
> > >             if (functions_cnt) {
> > > -                   struct elf_function *func;
> > > +                   const char *name = function__name(fn, cu);
> > > +                   struct elf_function *func = NULL;
> > > -                   func = find_function(btfe, function__name(fn, cu));
> > > +                   if (name)
> > > +                           func = find_function(btfe, name);
> > >                     if (!func || func->generated)
> > >                             continue;
> > >                     func->generated = true;
> > >
> >
>
> --
>
> - Arnaldo
Tom Stellard Jan. 21, 2021, 5:37 p.m. UTC | #10
On 1/21/21 5:38 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 12, 2021 at 04:27:59PM -0800, Tom Stellard escreveu:
>> On 1/12/21 10:40 AM, Jiri Olsa wrote:
>>> When processing kernel image build by clang we can
>>> find some functions without the name, which causes
>>> pahole to segfault.
>>>
>>> Adding extra checks to make sure we always have
>>> function's name defined before using it.
>>>
>>
>> I backported this patch to pahole 1.19, and I can confirm it fixes the
>> segfault for me.
> 
> I'm applying v2 for this patch and based on your above statement I'm
> adding a:
> 
> Tested-by: Tom Stellard <tstellar@redhat.com>
> 
> Ok?
> 

Yes, this is fine.  I also backported the v2 patch and tested it and it
fixed the issue.

-Tom

> Who originally reported this?
> 
> - Arnaldo
>   
>> -Tom
>>
>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>> ---
>>>    btf_encoder.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/btf_encoder.c b/btf_encoder.c
>>> index 333973054b61..17f7a14f2ef0 100644
>>> --- a/btf_encoder.c
>>> +++ b/btf_encoder.c
>>> @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
>>>    	if (elf_sym__type(sym) != STT_FUNC)
>>>    		return 0;
>>> +	if (!elf_sym__name(sym, btfe->symtab))
>>> +		return 0;
>>>    	if (functions_cnt == functions_alloc) {
>>>    		functions_alloc = max(1000, functions_alloc * 3 / 2);
>>> @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>>>    		if (!has_arg_names(cu, &fn->proto))
>>>    			continue;
>>>    		if (functions_cnt) {
>>> -			struct elf_function *func;
>>> +			const char *name = function__name(fn, cu);
>>> +			struct elf_function *func = NULL;
>>> -			func = find_function(btfe, function__name(fn, cu));
>>> +			if (name)
>>> +				func = find_function(btfe, name);
>>>    			if (!func || func->generated)
>>>    				continue;
>>>    			func->generated = true;
>>>
>>
>
Arnaldo Carvalho de Melo Jan. 21, 2021, 7:37 p.m. UTC | #11
Em Thu, Jan 21, 2021 at 05:06:25PM +0100, Sedat Dilek escreveu:
> On Thu, Jan 21, 2021 at 2:38 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> > Em Tue, Jan 12, 2021 at 04:27:59PM -0800, Tom Stellard escreveu:
> > > On 1/12/21 10:40 AM, Jiri Olsa wrote:
> > > > When processing kernel image build by clang we can
> > > > find some functions without the name, which causes
> > > > pahole to segfault.
> > > >
> > > > Adding extra checks to make sure we always have
> > > > function's name defined before using it.
> > > >
> > >
> > > I backported this patch to pahole 1.19, and I can confirm it fixes the
> > > segfault for me.
> >
> > I'm applying v2 for this patch and based on your above statement I'm
> > adding a:
> >
> > Tested-by: Tom Stellard <tstellar@redhat.com>
> >
> > Ok?
> >
> > Who originally reported this?
> >
> 
> The origin was AFAICS the thread where I asked initially [1].
> 
> Tom reported in the same thread in [2] that pahole segfaults.
> 
> Later in the thread Jiri offered a draft of this patch after doing some tests.
> 
> I have tested all diffs and v1 and v2 of Jiri's patch.
> ( Anyway, latest pahole ToT plus Jiri's patch did not solve my origin problem. )
> 
> So up to you Arnaldo for the credits.

Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
Tested-by: Tom Stellard <tstellar@redhat.com>

is how I'm going about it.

Thanks for clarifying,

- Arnaldo
 
> - Sedat -
> 
> [1] https://marc.info/?t=161036949500004&r=1&w=2
> [2] https://marc.info/?t=161036949500004&r=1&w=2
> 
> > - Arnaldo
> >
> > > -Tom
> > >
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >   btf_encoder.c | 8 ++++++--
> > > >   1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > > index 333973054b61..17f7a14f2ef0 100644
> > > > --- a/btf_encoder.c
> > > > +++ b/btf_encoder.c
> > > > @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > > >     if (elf_sym__type(sym) != STT_FUNC)
> > > >             return 0;
> > > > +   if (!elf_sym__name(sym, btfe->symtab))
> > > > +           return 0;
> > > >     if (functions_cnt == functions_alloc) {
> > > >             functions_alloc = max(1000, functions_alloc * 3 / 2);
> > > > @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > >             if (!has_arg_names(cu, &fn->proto))
> > > >                     continue;
> > > >             if (functions_cnt) {
> > > > -                   struct elf_function *func;
> > > > +                   const char *name = function__name(fn, cu);
> > > > +                   struct elf_function *func = NULL;
> > > > -                   func = find_function(btfe, function__name(fn, cu));
> > > > +                   if (name)
> > > > +                           func = find_function(btfe, name);
> > > >                     if (!func || func->generated)
> > > >                             continue;
> > > >                     func->generated = true;
> > > >
> > >
> >
> > --
> >
> > - Arnaldo
Andrii Nakryiko Jan. 21, 2021, 8:53 p.m. UTC | #12
On Thu, Jan 21, 2021 at 8:09 AM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> On Thu, Jan 21, 2021 at 2:38 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> > Em Tue, Jan 12, 2021 at 04:27:59PM -0800, Tom Stellard escreveu:
> > > On 1/12/21 10:40 AM, Jiri Olsa wrote:
> > > > When processing kernel image build by clang we can
> > > > find some functions without the name, which causes
> > > > pahole to segfault.
> > > >
> > > > Adding extra checks to make sure we always have
> > > > function's name defined before using it.
> > > >
> > >
> > > I backported this patch to pahole 1.19, and I can confirm it fixes the
> > > segfault for me.
> >
> > I'm applying v2 for this patch and based on your above statement I'm
> > adding a:
> >
> > Tested-by: Tom Stellard <tstellar@redhat.com>
> >
> > Ok?
> >
> > Who originally reported this?
> >
>
> The origin was AFAICS the thread where I asked initially [1].
>
> Tom reported in the same thread in [2] that pahole segfaults.
>
> Later in the thread Jiri offered a draft of this patch after doing some tests.
>
> I have tested all diffs and v1 and v2 of Jiri's patch.
> ( Anyway, latest pahole ToT plus Jiri's patch did not solve my origin problem. )

Your original problem was with DWARF5 or DWARF4? I think you mentioned
both at some point, but I remember I couldn't repro DWARF4 problems.
If you still have problems, can you start a new thread with steps to
repro (including Kconfig, tooling versions, etc). And one for each
problem, no all at the same time, please. I honestly lost track of
what's still not working among those multiple intertwined email
threads, sorry about that.

>
> So up to you Arnaldo for the credits.
>
> - Sedat -
>
> [1] https://marc.info/?t=161036949500004&r=1&w=2
> [2] https://marc.info/?t=161036949500004&r=1&w=2
>
> > - Arnaldo
> >
> > > -Tom
> > >
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >   btf_encoder.c | 8 ++++++--
> > > >   1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > > index 333973054b61..17f7a14f2ef0 100644
> > > > --- a/btf_encoder.c
> > > > +++ b/btf_encoder.c
> > > > @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > > >     if (elf_sym__type(sym) != STT_FUNC)
> > > >             return 0;
> > > > +   if (!elf_sym__name(sym, btfe->symtab))
> > > > +           return 0;
> > > >     if (functions_cnt == functions_alloc) {
> > > >             functions_alloc = max(1000, functions_alloc * 3 / 2);
> > > > @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > >             if (!has_arg_names(cu, &fn->proto))
> > > >                     continue;
> > > >             if (functions_cnt) {
> > > > -                   struct elf_function *func;
> > > > +                   const char *name = function__name(fn, cu);
> > > > +                   struct elf_function *func = NULL;
> > > > -                   func = find_function(btfe, function__name(fn, cu));
> > > > +                   if (name)
> > > > +                           func = find_function(btfe, name);
> > > >                     if (!func || func->generated)
> > > >                             continue;
> > > >                     func->generated = true;
> > > >
> > >
> >
> > --
> >
> > - Arnaldo
Sedat Dilek Jan. 22, 2021, 2:07 a.m. UTC | #13
On Thu, Jan 21, 2021 at 9:53 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jan 21, 2021 at 8:09 AM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> >
> > On Thu, Jan 21, 2021 at 2:38 PM Arnaldo Carvalho de Melo
> > <arnaldo.melo@gmail.com> wrote:
> > >
> > > Em Tue, Jan 12, 2021 at 04:27:59PM -0800, Tom Stellard escreveu:
> > > > On 1/12/21 10:40 AM, Jiri Olsa wrote:
> > > > > When processing kernel image build by clang we can
> > > > > find some functions without the name, which causes
> > > > > pahole to segfault.
> > > > >
> > > > > Adding extra checks to make sure we always have
> > > > > function's name defined before using it.
> > > > >
> > > >
> > > > I backported this patch to pahole 1.19, and I can confirm it fixes the
> > > > segfault for me.
> > >
> > > I'm applying v2 for this patch and based on your above statement I'm
> > > adding a:
> > >
> > > Tested-by: Tom Stellard <tstellar@redhat.com>
> > >
> > > Ok?
> > >
> > > Who originally reported this?
> > >
> >
> > The origin was AFAICS the thread where I asked initially [1].
> >
> > Tom reported in the same thread in [2] that pahole segfaults.
> >
> > Later in the thread Jiri offered a draft of this patch after doing some tests.
> >
> > I have tested all diffs and v1 and v2 of Jiri's patch.
> > ( Anyway, latest pahole ToT plus Jiri's patch did not solve my origin problem. )
>
> Your original problem was with DWARF5 or DWARF4? I think you mentioned
> both at some point, but I remember I couldn't repro DWARF4 problems.
> If you still have problems, can you start a new thread with steps to
> repro (including Kconfig, tooling versions, etc). And one for each
> problem, no all at the same time, please. I honestly lost track of
> what's still not working among those multiple intertwined email
> threads, sorry about that.
>

I love people saying "I have a (one) problem." :-).

The origin was Debian kernel-team enabled BTF-debuginfo Kconfig.

My main focus is to be as close to Debian's kernel-config and if this
works well with (experimental) Linux DWARF v5 support I am a happy
guy.

Do you want Nick's DWARF v5 patch-series as a base?
Thinking of DWARF-v4?
Use Nick's patchset or DWARF-v4 what is in Linux upstream means Linux
v5.11-rc4+?
What Git tree to use - Linus or one of your BPF/BTF folks?

What version of pahole (latest Git) etc.?

- Sedat -

> >
> > So up to you Arnaldo for the credits.
> >
> > - Sedat -
> >
> > [1] https://marc.info/?t=161036949500004&r=1&w=2
> > [2] https://marc.info/?t=161036949500004&r=1&w=2
> >
> > > - Arnaldo
> > >
> > > > -Tom
> > > >
> > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > ---
> > > > >   btf_encoder.c | 8 ++++++--
> > > > >   1 file changed, 6 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > > > index 333973054b61..17f7a14f2ef0 100644
> > > > > --- a/btf_encoder.c
> > > > > +++ b/btf_encoder.c
> > > > > @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > > > >     if (elf_sym__type(sym) != STT_FUNC)
> > > > >             return 0;
> > > > > +   if (!elf_sym__name(sym, btfe->symtab))
> > > > > +           return 0;
> > > > >     if (functions_cnt == functions_alloc) {
> > > > >             functions_alloc = max(1000, functions_alloc * 3 / 2);
> > > > > @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > > >             if (!has_arg_names(cu, &fn->proto))
> > > > >                     continue;
> > > > >             if (functions_cnt) {
> > > > > -                   struct elf_function *func;
> > > > > +                   const char *name = function__name(fn, cu);
> > > > > +                   struct elf_function *func = NULL;
> > > > > -                   func = find_function(btfe, function__name(fn, cu));
> > > > > +                   if (name)
> > > > > +                           func = find_function(btfe, name);
> > > > >                     if (!func || func->generated)
> > > > >                             continue;
> > > > >                     func->generated = true;
> > > > >
> > > >
> > >
> > > --
> > >
> > > - Arnaldo
Andrii Nakryiko Jan. 22, 2021, 4:11 a.m. UTC | #14
On Thu, Jan 21, 2021 at 6:07 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> On Thu, Jan 21, 2021 at 9:53 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Jan 21, 2021 at 8:09 AM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> > >
> > > On Thu, Jan 21, 2021 at 2:38 PM Arnaldo Carvalho de Melo
> > > <arnaldo.melo@gmail.com> wrote:
> > > >
> > > > Em Tue, Jan 12, 2021 at 04:27:59PM -0800, Tom Stellard escreveu:
> > > > > On 1/12/21 10:40 AM, Jiri Olsa wrote:
> > > > > > When processing kernel image build by clang we can
> > > > > > find some functions without the name, which causes
> > > > > > pahole to segfault.
> > > > > >
> > > > > > Adding extra checks to make sure we always have
> > > > > > function's name defined before using it.
> > > > > >
> > > > >
> > > > > I backported this patch to pahole 1.19, and I can confirm it fixes the
> > > > > segfault for me.
> > > >
> > > > I'm applying v2 for this patch and based on your above statement I'm
> > > > adding a:
> > > >
> > > > Tested-by: Tom Stellard <tstellar@redhat.com>
> > > >
> > > > Ok?
> > > >
> > > > Who originally reported this?
> > > >
> > >
> > > The origin was AFAICS the thread where I asked initially [1].
> > >
> > > Tom reported in the same thread in [2] that pahole segfaults.
> > >
> > > Later in the thread Jiri offered a draft of this patch after doing some tests.
> > >
> > > I have tested all diffs and v1 and v2 of Jiri's patch.
> > > ( Anyway, latest pahole ToT plus Jiri's patch did not solve my origin problem. )
> >
> > Your original problem was with DWARF5 or DWARF4? I think you mentioned
> > both at some point, but I remember I couldn't repro DWARF4 problems.
> > If you still have problems, can you start a new thread with steps to
> > repro (including Kconfig, tooling versions, etc). And one for each
> > problem, no all at the same time, please. I honestly lost track of
> > what's still not working among those multiple intertwined email
> > threads, sorry about that.
> >
>
> I love people saying "I have a (one) problem." :-).
>
> The origin was Debian kernel-team enabled BTF-debuginfo Kconfig.
>
> My main focus is to be as close to Debian's kernel-config and if this
> works well with (experimental) Linux DWARF v5 support I am a happy
> guy.

I don't know what kernel config Debian is using, that's why I'm asking
for kernel config that does cause the problem. Because the one I'm
using doesn't. But this problem can be a result of a lot of things,
specific compiler and its version, specific kernel config, who knows
what else.

>
> Do you want Nick's DWARF v5 patch-series as a base?

Arnaldo was going to figure out the DWARF v5 problem, so I'm leaving
it up to him. I'm curious about DWARF v4 problems because no one yet
reported that previously.

> Thinking of DWARF-v4?
> Use Nick's patchset or DWARF-v4 what is in Linux upstream means Linux
> v5.11-rc4+?
> What Git tree to use - Linus or one of your BPF/BTF folks?

I checked both v5.11-rc4 and the latest bpf-next with
CONFIG_DEBUG_INFO_DWARF4=y and CONFIG_DEBUG_INFO_BTF=y. I get no
warnings, everything works.

>
> What version of pahole (latest Git) etc.?

Latest pahole built from Git, yes.

But let's not do it in a backwards manner with me telling you what
works (my environment, my config), rather you telling us what
*doesn't* work (your config, your environment), so that we can try to
reproduce.

>
> - Sedat -
>
> > >
> > > So up to you Arnaldo for the credits.
> > >
> > > - Sedat -
> > >
> > > [1] https://marc.info/?t=161036949500004&r=1&w=2
> > > [2] https://marc.info/?t=161036949500004&r=1&w=2
> > >
> > > > - Arnaldo
> > > >
> > > > > -Tom
> > > > >
> > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > > ---
> > > > > >   btf_encoder.c | 8 ++++++--
> > > > > >   1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > > > > index 333973054b61..17f7a14f2ef0 100644
> > > > > > --- a/btf_encoder.c
> > > > > > +++ b/btf_encoder.c
> > > > > > @@ -72,6 +72,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > > > > >     if (elf_sym__type(sym) != STT_FUNC)
> > > > > >             return 0;
> > > > > > +   if (!elf_sym__name(sym, btfe->symtab))
> > > > > > +           return 0;
> > > > > >     if (functions_cnt == functions_alloc) {
> > > > > >             functions_alloc = max(1000, functions_alloc * 3 / 2);
> > > > > > @@ -730,9 +732,11 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > > > >             if (!has_arg_names(cu, &fn->proto))
> > > > > >                     continue;
> > > > > >             if (functions_cnt) {
> > > > > > -                   struct elf_function *func;
> > > > > > +                   const char *name = function__name(fn, cu);
> > > > > > +                   struct elf_function *func = NULL;
> > > > > > -                   func = find_function(btfe, function__name(fn, cu));
> > > > > > +                   if (name)
> > > > > > +                           func = find_function(btfe, name);
> > > > > >                     if (!func || func->generated)
> > > > > >                             continue;
> > > > > >                     func->generated = true;
> > > > > >
> > > > >
> > > >
> > > > --
> > > >
> > > > - Arnaldo
Arnaldo Carvalho de Melo Jan. 28, 2021, 8 p.m. UTC | #15
Em Thu, Jan 21, 2021 at 08:11:17PM -0800, Andrii Nakryiko escreveu:
> On Thu, Jan 21, 2021 at 6:07 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> > Do you want Nick's DWARF v5 patch-series as a base?
 
> Arnaldo was going to figure out the DWARF v5 problem, so I'm leaving
> it up to him. I'm curious about DWARF v4 problems because no one yet
> reported that previously.

I think I have the reported one fixed, Andrii, can you please do
whatever pre-release tests you can in your environment with what is in:

https://git.kernel.org/pub/scm/devel/pahole/pahole.git/log/?h=DW_AT_data_bit_offset

?

The cset has the tests I performed and the references to the bugzilla
ticket and Daniel has tested as well for his XDR + gcc 11 problem.

Thanks,

- Arnaldo
Sedat Dilek Jan. 28, 2021, 8:57 p.m. UTC | #16
On Thu, Jan 28, 2021 at 9:00 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Thu, Jan 21, 2021 at 08:11:17PM -0800, Andrii Nakryiko escreveu:
> > On Thu, Jan 21, 2021 at 6:07 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> > > Do you want Nick's DWARF v5 patch-series as a base?
>
> > Arnaldo was going to figure out the DWARF v5 problem, so I'm leaving
> > it up to him. I'm curious about DWARF v4 problems because no one yet
> > reported that previously.
>
> I think I have the reported one fixed, Andrii, can you please do
> whatever pre-release tests you can in your environment with what is in:
>
> https://git.kernel.org/pub/scm/devel/pahole/pahole.git/log/?h=DW_AT_data_bit_offset
>
> ?
>
> The cset has the tests I performed and the references to the bugzilla
> ticket and Daniel has tested as well for his XDR + gcc 11 problem.
>
> Thanks,
>

What Git tree should someone use to test this?
Linus Git?
bpf / bpf-next?

- Sedat -
Arnaldo Carvalho de Melo Jan. 28, 2021, 9:11 p.m. UTC | #17
Em Thu, Jan 28, 2021 at 09:57:14PM +0100, Sedat Dilek escreveu:
> On Thu, Jan 28, 2021 at 9:00 PM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote:

> > Em Thu, Jan 21, 2021 at 08:11:17PM -0800, Andrii Nakryiko escreveu:
> > > On Thu, Jan 21, 2021 at 6:07 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> > > > Do you want Nick's DWARF v5 patch-series as a base?

> > > Arnaldo was going to figure out the DWARF v5 problem, so I'm leaving
> > > it up to him. I'm curious about DWARF v4 problems because no one yet
> > > reported that previously.

> > I think I have the reported one fixed, Andrii, can you please do
> > whatever pre-release tests you can in your environment with what is in:

> > https://git.kernel.org/pub/scm/devel/pahole/pahole.git/log/?h=DW_AT_data_bit_offset

> > ?

> > The cset has the tests I performed and the references to the bugzilla
> > ticket and Daniel has tested as well for his XDR + gcc 11 problem.
> 
> What Git tree should someone use to test this?
> Linus Git?
> bpf / bpf-next?

The one you were having problems with :)

This pahole branch should be handling multiple problems, this is the
list of changes since v1.19:

[acme@five pahole]$ git log --oneline v1.19..
b91b19840b0062b8 (HEAD -> master, quaco/master, origin/DW_AT_data_bit_offset) dwarf_loader: Support DW_AT_data_bit_offset
c692e8ac5ccbab99 dwarf_loader: Optimize a bit the reading of DW_AT_data_member_location
65917b24942ce620 dwarf_loader: Fix typo
77205a119c85e396 dwarf_loader: Introduce __attr_offset() to reuse call to dwarf_attr()
8ec231f6b0c8aaef dwarf_loader: Support DW_FORM_implicit_const in attr_numeric()
7453895e01edb535 (origin/master, origin/HEAD) btf_encoder: Improve ELF error reporting
1bb49897dd2b65b0 bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
3f8aad340bf1a188 elf_symtab: Handle SHN_XINDEX index in elf_section_by_name()
e32b9800e650a6eb btf_encoder: Add extra checks for symbol names
82749180b23d3c9c libbpf: allow to use packaged version
452dbcf35f1a7bf9 btf_encoder: Improve error-handling around objcopy
cf381f9a3822d68b btf_encoder: Fix handling of restrict qualifier
b688e35970600c15 btf_encoder: fix skipping per-CPU variables at offset 0
8c009d6ce762dfc9 btf_encoder: fix BTF variable generation for kernel modules
b94e97e015a94e6b dwarves: Fix compilation on 32-bit architectures
17df51c700248f02 btf_encoder: Detect kernel module ftrace addresses
06ca639505fc56c6 btf_encoder: Use address size based on ELF's class
aff60970d16b909e btf_encoder: Factor filter_functions function
1e6a3fed6e52d365 rpm: Fix changelog date
[acme@five pahole]$

Now I just need to do the boilerplate update of the version number,
Changes and .spec file, to release v1.20.

- Arnaldo
Sedat Dilek Jan. 28, 2021, 10:28 p.m. UTC | #18
On Thu, Jan 28, 2021 at 10:11 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Thu, Jan 28, 2021 at 09:57:14PM +0100, Sedat Dilek escreveu:
> > On Thu, Jan 28, 2021 at 9:00 PM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote:
>
> > > Em Thu, Jan 21, 2021 at 08:11:17PM -0800, Andrii Nakryiko escreveu:
> > > > On Thu, Jan 21, 2021 at 6:07 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> > > > > Do you want Nick's DWARF v5 patch-series as a base?
>
> > > > Arnaldo was going to figure out the DWARF v5 problem, so I'm leaving
> > > > it up to him. I'm curious about DWARF v4 problems because no one yet
> > > > reported that previously.
>
> > > I think I have the reported one fixed, Andrii, can you please do
> > > whatever pre-release tests you can in your environment with what is in:
>
> > > https://git.kernel.org/pub/scm/devel/pahole/pahole.git/log/?h=DW_AT_data_bit_offset
>
> > > ?
>
> > > The cset has the tests I performed and the references to the bugzilla
> > > ticket and Daniel has tested as well for his XDR + gcc 11 problem.
> >
> > What Git tree should someone use to test this?
> > Linus Git?
> > bpf / bpf-next?
>
> The one you were having problems with :)
>
> This pahole branch should be handling multiple problems, this is the
> list of changes since v1.19:
>
> [acme@five pahole]$ git log --oneline v1.19..
> b91b19840b0062b8 (HEAD -> master, quaco/master, origin/DW_AT_data_bit_offset) dwarf_loader: Support DW_AT_data_bit_offset
> c692e8ac5ccbab99 dwarf_loader: Optimize a bit the reading of DW_AT_data_member_location
> 65917b24942ce620 dwarf_loader: Fix typo
> 77205a119c85e396 dwarf_loader: Introduce __attr_offset() to reuse call to dwarf_attr()
> 8ec231f6b0c8aaef dwarf_loader: Support DW_FORM_implicit_const in attr_numeric()
> 7453895e01edb535 (origin/master, origin/HEAD) btf_encoder: Improve ELF error reporting
> 1bb49897dd2b65b0 bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
> 3f8aad340bf1a188 elf_symtab: Handle SHN_XINDEX index in elf_section_by_name()
> e32b9800e650a6eb btf_encoder: Add extra checks for symbol names
> 82749180b23d3c9c libbpf: allow to use packaged version
> 452dbcf35f1a7bf9 btf_encoder: Improve error-handling around objcopy
> cf381f9a3822d68b btf_encoder: Fix handling of restrict qualifier
> b688e35970600c15 btf_encoder: fix skipping per-CPU variables at offset 0
> 8c009d6ce762dfc9 btf_encoder: fix BTF variable generation for kernel modules
> b94e97e015a94e6b dwarves: Fix compilation on 32-bit architectures
> 17df51c700248f02 btf_encoder: Detect kernel module ftrace addresses
> 06ca639505fc56c6 btf_encoder: Use address size based on ELF's class
> aff60970d16b909e btf_encoder: Factor filter_functions function
> 1e6a3fed6e52d365 rpm: Fix changelog date
> [acme@five pahole]$
>
> Now I just need to do the boilerplate update of the version number,
> Changes and .spec file, to release v1.20.
>

When do you plan to release v1.20?
I can wait until this is done.

- Sedat -
Andrii Nakryiko Feb. 2, 2021, 7:48 a.m. UTC | #19
On Thu, Jan 28, 2021 at 12:00 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Thu, Jan 21, 2021 at 08:11:17PM -0800, Andrii Nakryiko escreveu:
> > On Thu, Jan 21, 2021 at 6:07 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> > > Do you want Nick's DWARF v5 patch-series as a base?
>
> > Arnaldo was going to figure out the DWARF v5 problem, so I'm leaving
> > it up to him. I'm curious about DWARF v4 problems because no one yet
> > reported that previously.
>
> I think I have the reported one fixed, Andrii, can you please do
> whatever pre-release tests you can in your environment with what is in:
>

Hi Arnaldo,

Sorry for the delay, just back from a short PTO.

It all looks to be working fine on my side. There is a compilation
error in our libbpf CI when building the latest pahole from sources
due to DW_FORM_implicit_const being undefined. I'm updating our VMs to
use Ubuntu Focal 20.04, up from Bionic 18.04, and that should
hopefully solve the issue due to newer versions of libdw. If you worry
about breaking others, though, we might want to add #ifndef guards and
re-define DW_FORM_implicit_const as 0x21 explicitly in pahole source
code.

But otherwise, all good from what I can see in my environment. Looking
forward to 1.20 release! I'll let you know if, after updating to
Ubuntu Focal, any new pahole issues crop up.


> https://git.kernel.org/pub/scm/devel/pahole/pahole.git/log/?h=DW_AT_data_bit_offset
>
> ?
>
> The cset has the tests I performed and the references to the bugzilla
> ticket and Daniel has tested as well for his XDR + gcc 11 problem.
>
> Thanks,
>
> - Arnaldo
Sedat Dilek Feb. 3, 2021, 9:03 a.m. UTC | #20
On Tue, Feb 2, 2021 at 8:48 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jan 28, 2021 at 12:00 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> > Em Thu, Jan 21, 2021 at 08:11:17PM -0800, Andrii Nakryiko escreveu:
> > > On Thu, Jan 21, 2021 at 6:07 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> > > > Do you want Nick's DWARF v5 patch-series as a base?
> >
> > > Arnaldo was going to figure out the DWARF v5 problem, so I'm leaving
> > > it up to him. I'm curious about DWARF v4 problems because no one yet
> > > reported that previously.
> >
> > I think I have the reported one fixed, Andrii, can you please do
> > whatever pre-release tests you can in your environment with what is in:
> >
>
> Hi Arnaldo,
>
> Sorry for the delay, just back from a short PTO.
>
> It all looks to be working fine on my side. There is a compilation
> error in our libbpf CI when building the latest pahole from sources
> due to DW_FORM_implicit_const being undefined. I'm updating our VMs to
> use Ubuntu Focal 20.04, up from Bionic 18.04, and that should
> hopefully solve the issue due to newer versions of libdw. If you worry
> about breaking others, though, we might want to add #ifndef guards and
> re-define DW_FORM_implicit_const as 0x21 explicitly in pahole source
> code.
>
> But otherwise, all good from what I can see in my environment. Looking
> forward to 1.20 release! I'll let you know if, after updating to
> Ubuntu Focal, any new pahole issues crop up.
>

Last weekend I did some testing with
<pahole.git#DW_AT_data_bit_offset> and DWARF-v5 support for the
Linux-kernel.

The good: I was able to compile :-).
The bad: My build-log grew up to 1.2GiB and I could not boot in QEMU.
The ugly: I killed the archive which had all relevant material.

Yesterday, I compiled latest pahole.git:

$ git describe
v1.19-25-g8d6f06f053a0

$ git log -1 --oneline
8d6f06f053a0 (HEAD -> master, origin/master, origin/HEAD)
dwarf_loader: Add conditional DW_FORM_implicit_const definition for
older system

I cannot promise to test it with Nick Desaulniers' DWARF-v5 patchset
but the recent DWARF changes within pahole.git look promising.

- Sedat -

>
> > https://git.kernel.org/pub/scm/devel/pahole/pahole.git/log/?h=DW_AT_data_bit_offset
> >
> > ?
> >
> > The cset has the tests I performed and the references to the bugzilla
> > ticket and Daniel has tested as well for his XDR + gcc 11 problem.
> >
> > Thanks,
> >
> > - Arnaldo
Mark Wielaard Feb. 3, 2021, 10:23 a.m. UTC | #21
Hi,

On Wed, 2021-02-03 at 10:03 +0100, Sedat Dilek wrote:
> > It all looks to be working fine on my side. There is a compilation
> > error in our libbpf CI when building the latest pahole from sources
> > due to DW_FORM_implicit_const being undefined. I'm updating our VMs to
> > use Ubuntu Focal 20.04, up from Bionic 18.04, and that should
> > hopefully solve the issue due to newer versions of libdw. If you worry
> > about breaking others, though, we might want to add #ifndef guards and
> > re-define DW_FORM_implicit_const as 0x21 explicitly in pahole source
> > code.

I think that might be a good idea for older setups. But that also means
that the underlying elfutils libdw doesn't support DWARF5, so pahole
itself also wouldn't work (the define would only fix the compile time
issue, not the runtime issue of not being able to parse
DW_FORM_implicit_const). That might not be a problem because such
systems also wouldn't have GCC11 defaulting to DWARF5.

> > But otherwise, all good from what I can see in my environment.
> > Looking
> > forward to 1.20 release! I'll let you know if, after updating to
> > Ubuntu Focal, any new pahole issues crop up.
> > 
> 
> Last weekend I did some testing with
> <pahole.git#DW_AT_data_bit_offset> and DWARF-v5 support for the
> Linux-kernel.
> 
> The good: I was able to compile :-).
> The bad: My build-log grew up to 1.2GiB and I could not boot in QEMU.
> The ugly: I killed the archive which had all relevant material.

I think the build-log grew so much because of warnings about unknown
tags. At least when using GCC11 you'll get a couple of standardized
DWARF5 tags instead of the GNU extensions to DWARF4. That should be
solved by:

   commit d783117162c0212d4f75f6cea185f493d2f244e1
   Author: Mark Wielaard <mark@klomp.org>
   Date:   Sun Jan 31 01:27:31 2021 +0100

       dwarf_loader: Handle DWARF5 DW_TAG_call_site like DW_TAG_GNU_call_site

Cheers,

Mark
Sedat Dilek Feb. 3, 2021, 9:48 p.m. UTC | #22
On Wed, Feb 3, 2021 at 11:23 AM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi,
>
> On Wed, 2021-02-03 at 10:03 +0100, Sedat Dilek wrote:
> > > It all looks to be working fine on my side. There is a compilation
> > > error in our libbpf CI when building the latest pahole from sources
> > > due to DW_FORM_implicit_const being undefined. I'm updating our VMs to
> > > use Ubuntu Focal 20.04, up from Bionic 18.04, and that should
> > > hopefully solve the issue due to newer versions of libdw. If you worry
> > > about breaking others, though, we might want to add #ifndef guards and
> > > re-define DW_FORM_implicit_const as 0x21 explicitly in pahole source
> > > code.
>
> I think that might be a good idea for older setups. But that also means
> that the underlying elfutils libdw doesn't support DWARF5, so pahole
> itself also wouldn't work (the define would only fix the compile time
> issue, not the runtime issue of not being able to parse
> DW_FORM_implicit_const). That might not be a problem because such
> systems also wouldn't have GCC11 defaulting to DWARF5.
>
> > > But otherwise, all good from what I can see in my environment.
> > > Looking
> > > forward to 1.20 release! I'll let you know if, after updating to
> > > Ubuntu Focal, any new pahole issues crop up.
> > >
> >
> > Last weekend I did some testing with
> > <pahole.git#DW_AT_data_bit_offset> and DWARF-v5 support for the
> > Linux-kernel.
> >
> > The good: I was able to compile :-).
> > The bad: My build-log grew up to 1.2GiB and I could not boot in QEMU.
> > The ugly: I killed the archive which had all relevant material.
>
> I think the build-log grew so much because of warnings about unknown
> tags. At least when using GCC11 you'll get a couple of standardized
> DWARF5 tags instead of the GNU extensions to DWARF4. That should be
> solved by:
>
>    commit d783117162c0212d4f75f6cea185f493d2f244e1
>    Author: Mark Wielaard <mark@klomp.org>
>    Date:   Sun Jan 31 01:27:31 2021 +0100
>
>        dwarf_loader: Handle DWARF5 DW_TAG_call_site like DW_TAG_GNU_call_site
>

I had some conversation with Mark directly as I dropped by accident the CC list.

With latest pahole from Git and CONFIG_DEBUG_INFO_BTF=y I was not able
to build with DWARF-v4 and DWARF-v5.

Hope it is OK for you Mark when I quote you:

> Here I use LLVM/Clang v12.0.0-rc1 with Clang's Integrated Assembler
> (make LLVM_IAS=1).

Note I haven't personally tested llvm with DWARF5. I know some other
tools cannot (yet) handle the DWARF5 produced by llvm (for example
valgrind, rpm debugedit and dwz don't handle all the forms llvm emits
when it produces DWARF5, which aren't emitted by GCC unless requesting
split-dwarf). In theory dwarves/pahole should be able to handle it
because elfutils libdw (at least versions > 0.172) does handle it. But
I don't know if anybody ever tested that. But I believe llvm will by
default emit DWARF4, not 5.

More quotes from Mark:

I would try to avoid using clang producing DWARF5. It clearly has some
incompatibilities with dwarves/pahole. It should work if you don't set
DEBUG_INFO_DWARF5. Try GCC 11 (which defaults to -gdwarf-5) or an
earlier version (probably at least GCC 8 or higher) using -gdwarf-5
explicitly.

What makes me nerves are reports from Red Hat's CKI reporting:

'failed to validate module [something] BTF: -22 '

This is was from ClangBuiltLinux mailing-list.

Looks like CONFIG_DEBUG_INFO_BTF=y makes troubles with LLVM/Clang.
Can we have a fix for Linux v5.11-rc6+ to avoid a selection of it when
CC_IS_CLANG=y?

- Sedat -


- Sedat -
Andrii Nakryiko Feb. 3, 2021, 11:21 p.m. UTC | #23
On Wed, Feb 3, 2021 at 1:48 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> On Wed, Feb 3, 2021 at 11:23 AM Mark Wielaard <mark@klomp.org> wrote:
> >
> > Hi,
> >
> > On Wed, 2021-02-03 at 10:03 +0100, Sedat Dilek wrote:
> > > > It all looks to be working fine on my side. There is a compilation
> > > > error in our libbpf CI when building the latest pahole from sources
> > > > due to DW_FORM_implicit_const being undefined. I'm updating our VMs to
> > > > use Ubuntu Focal 20.04, up from Bionic 18.04, and that should
> > > > hopefully solve the issue due to newer versions of libdw. If you worry
> > > > about breaking others, though, we might want to add #ifndef guards and
> > > > re-define DW_FORM_implicit_const as 0x21 explicitly in pahole source
> > > > code.
> >
> > I think that might be a good idea for older setups. But that also means
> > that the underlying elfutils libdw doesn't support DWARF5, so pahole
> > itself also wouldn't work (the define would only fix the compile time
> > issue, not the runtime issue of not being able to parse
> > DW_FORM_implicit_const). That might not be a problem because such
> > systems also wouldn't have GCC11 defaulting to DWARF5.
> >
> > > > But otherwise, all good from what I can see in my environment.
> > > > Looking
> > > > forward to 1.20 release! I'll let you know if, after updating to
> > > > Ubuntu Focal, any new pahole issues crop up.
> > > >
> > >
> > > Last weekend I did some testing with
> > > <pahole.git#DW_AT_data_bit_offset> and DWARF-v5 support for the
> > > Linux-kernel.
> > >
> > > The good: I was able to compile :-).
> > > The bad: My build-log grew up to 1.2GiB and I could not boot in QEMU.
> > > The ugly: I killed the archive which had all relevant material.
> >
> > I think the build-log grew so much because of warnings about unknown
> > tags. At least when using GCC11 you'll get a couple of standardized
> > DWARF5 tags instead of the GNU extensions to DWARF4. That should be
> > solved by:
> >
> >    commit d783117162c0212d4f75f6cea185f493d2f244e1
> >    Author: Mark Wielaard <mark@klomp.org>
> >    Date:   Sun Jan 31 01:27:31 2021 +0100
> >
> >        dwarf_loader: Handle DWARF5 DW_TAG_call_site like DW_TAG_GNU_call_site
> >
>
> I had some conversation with Mark directly as I dropped by accident the CC list.
>
> With latest pahole from Git and CONFIG_DEBUG_INFO_BTF=y I was not able
> to build with DWARF-v4 and DWARF-v5.

There is hardly anything actionable without all the extra info I've
asked you before. What's the issue? What's the kernel config? Tool
versions?

>
> Hope it is OK for you Mark when I quote you:
>
> > Here I use LLVM/Clang v12.0.0-rc1 with Clang's Integrated Assembler
> > (make LLVM_IAS=1).
>
> Note I haven't personally tested llvm with DWARF5. I know some other
> tools cannot (yet) handle the DWARF5 produced by llvm (for example
> valgrind, rpm debugedit and dwz don't handle all the forms llvm emits
> when it produces DWARF5, which aren't emitted by GCC unless requesting
> split-dwarf). In theory dwarves/pahole should be able to handle it
> because elfutils libdw (at least versions > 0.172) does handle it. But
> I don't know if anybody ever tested that. But I believe llvm will by
> default emit DWARF4, not 5.
>
> More quotes from Mark:
>
> I would try to avoid using clang producing DWARF5. It clearly has some
> incompatibilities with dwarves/pahole. It should work if you don't set
> DEBUG_INFO_DWARF5. Try GCC 11 (which defaults to -gdwarf-5) or an
> earlier version (probably at least GCC 8 or higher) using -gdwarf-5
> explicitly.
>
> What makes me nerves are reports from Red Hat's CKI reporting:
>
> 'failed to validate module [something] BTF: -22 '
>
> This is was from ClangBuiltLinux mailing-list.

And no link to the issue, of course. If you are hoping for someone to
try to help and fix issues, please provide extra info. If this is what
I think it is, that was the problem with kernel rejecting empty BTF
and it was fixed already in v5.11-fbk6. But who knows, I can only
guess.

>
> Looks like CONFIG_DEBUG_INFO_BTF=y makes troubles with LLVM/Clang.
> Can we have a fix for Linux v5.11-rc6+ to avoid a selection of it when
> CC_IS_CLANG=y?

Let's first understand problems and try to fix them, please.

>
> - Sedat -
>
>
> - Sedat -
Arnaldo Carvalho de Melo Feb. 4, 2021, 1:06 a.m. UTC | #24
Em Wed, Feb 03, 2021 at 03:21:58PM -0800, Andrii Nakryiko escreveu:
> On Wed, Feb 3, 2021 at 1:48 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> > Looks like CONFIG_DEBUG_INFO_BTF=y makes troubles with LLVM/Clang.
> > Can we have a fix for Linux v5.11-rc6+ to avoid a selection of it when
> > CC_IS_CLANG=y?
 
> Let's first understand problems and try to fix them, please.

Yeah, from what I've read this is something only related to clang
building the kernel with DWARF5, right?

I just delayed to tomorrow releasing pahole 1.20 as its late here and
there is this discussion going on, what I have is at:

https://git.kernel.org/pub/scm/devel/pahole/pahole.git/log/?h=tmp.1.20

There is one patch more than what I think Sedat tried, that makes a
Mark's patch for DWARF_TAG_call_site{_parameter} to be applied as well
when processing inline expansions, which for what is being discussed
here seems unrelated, just avoids tons of warnings when processing
vmlinux with gcc 11 in fedora rawhide.

- Arnaldo
Sedat Dilek Feb. 4, 2021, 6:40 a.m. UTC | #25
On Thu, Feb 4, 2021 at 12:22 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Feb 3, 2021 at 1:48 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> >
> > On Wed, Feb 3, 2021 at 11:23 AM Mark Wielaard <mark@klomp.org> wrote:
> > >
> > > Hi,
> > >
> > > On Wed, 2021-02-03 at 10:03 +0100, Sedat Dilek wrote:
> > > > > It all looks to be working fine on my side. There is a compilation
> > > > > error in our libbpf CI when building the latest pahole from sources
> > > > > due to DW_FORM_implicit_const being undefined. I'm updating our VMs to
> > > > > use Ubuntu Focal 20.04, up from Bionic 18.04, and that should
> > > > > hopefully solve the issue due to newer versions of libdw. If you worry
> > > > > about breaking others, though, we might want to add #ifndef guards and
> > > > > re-define DW_FORM_implicit_const as 0x21 explicitly in pahole source
> > > > > code.
> > >
> > > I think that might be a good idea for older setups. But that also means
> > > that the underlying elfutils libdw doesn't support DWARF5, so pahole
> > > itself also wouldn't work (the define would only fix the compile time
> > > issue, not the runtime issue of not being able to parse
> > > DW_FORM_implicit_const). That might not be a problem because such
> > > systems also wouldn't have GCC11 defaulting to DWARF5.
> > >
> > > > > But otherwise, all good from what I can see in my environment.
> > > > > Looking
> > > > > forward to 1.20 release! I'll let you know if, after updating to
> > > > > Ubuntu Focal, any new pahole issues crop up.
> > > > >
> > > >
> > > > Last weekend I did some testing with
> > > > <pahole.git#DW_AT_data_bit_offset> and DWARF-v5 support for the
> > > > Linux-kernel.
> > > >
> > > > The good: I was able to compile :-).
> > > > The bad: My build-log grew up to 1.2GiB and I could not boot in QEMU.
> > > > The ugly: I killed the archive which had all relevant material.
> > >
> > > I think the build-log grew so much because of warnings about unknown
> > > tags. At least when using GCC11 you'll get a couple of standardized
> > > DWARF5 tags instead of the GNU extensions to DWARF4. That should be
> > > solved by:
> > >
> > >    commit d783117162c0212d4f75f6cea185f493d2f244e1
> > >    Author: Mark Wielaard <mark@klomp.org>
> > >    Date:   Sun Jan 31 01:27:31 2021 +0100
> > >
> > >        dwarf_loader: Handle DWARF5 DW_TAG_call_site like DW_TAG_GNU_call_site
> > >
> >
> > I had some conversation with Mark directly as I dropped by accident the CC list.
> >
> > With latest pahole from Git and CONFIG_DEBUG_INFO_BTF=y I was not able
> > to build with DWARF-v4 and DWARF-v5.
>
> There is hardly anything actionable without all the extra info I've
> asked you before. What's the issue? What's the kernel config? Tool
> versions?
>
> >
> > Hope it is OK for you Mark when I quote you:
> >
> > > Here I use LLVM/Clang v12.0.0-rc1 with Clang's Integrated Assembler
> > > (make LLVM_IAS=1).
> >
> > Note I haven't personally tested llvm with DWARF5. I know some other
> > tools cannot (yet) handle the DWARF5 produced by llvm (for example
> > valgrind, rpm debugedit and dwz don't handle all the forms llvm emits
> > when it produces DWARF5, which aren't emitted by GCC unless requesting
> > split-dwarf). In theory dwarves/pahole should be able to handle it
> > because elfutils libdw (at least versions > 0.172) does handle it. But
> > I don't know if anybody ever tested that. But I believe llvm will by
> > default emit DWARF4, not 5.
> >
> > More quotes from Mark:
> >
> > I would try to avoid using clang producing DWARF5. It clearly has some
> > incompatibilities with dwarves/pahole. It should work if you don't set
> > DEBUG_INFO_DWARF5. Try GCC 11 (which defaults to -gdwarf-5) or an
> > earlier version (probably at least GCC 8 or higher) using -gdwarf-5
> > explicitly.
> >
> > What makes me nerves are reports from Red Hat's CKI reporting:
> >
> > 'failed to validate module [something] BTF: -22 '
> >
> > This is was from ClangBuiltLinux mailing-list.
>
> And no link to the issue, of course. If you are hoping for someone to
> try to help and fix issues, please provide extra info. If this is what
> I think it is, that was the problem with kernel rejecting empty BTF
> and it was fixed already in v5.11-fbk6. But who knows, I can only
> guess.
>

"Do one thing and do it well."
( Unix philosophy )

https://en.wikipedia.org/wiki/Unix_philosophy#Do_One_Thing_and_Do_It_Well

Yesterday, I did two things in parallel.
( I had some other stuff to dig into which is fixed for me. )

As for fun I will do a new test with <pahole.git#tmp.1.20>.

I will attach kernel-config and BTF/pahole warnings.

For the ClangBuiltLinux mailing-list link:
I have asked on the list to have the links so that people from
outlands can read them.

"Just for fun" (IMHO that is the title of Linus Torvalds' biographie :-)?)

- Sedat -

> >
> > Looks like CONFIG_DEBUG_INFO_BTF=y makes troubles with LLVM/Clang.
> > Can we have a fix for Linux v5.11-rc6+ to avoid a selection of it when
> > CC_IS_CLANG=y?
>
> Let's first understand problems and try to fix them, please.
>
> >
> > - Sedat -
> >
> >
> > - Sedat -
diff mbox series

Patch

diff --git a/btf_encoder.c b/btf_encoder.c
index 333973054b61..17f7a14f2ef0 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -72,6 +72,8 @@  static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
 
 	if (elf_sym__type(sym) != STT_FUNC)
 		return 0;
+	if (!elf_sym__name(sym, btfe->symtab))
+		return 0;
 
 	if (functions_cnt == functions_alloc) {
 		functions_alloc = max(1000, functions_alloc * 3 / 2);
@@ -730,9 +732,11 @@  int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		if (!has_arg_names(cu, &fn->proto))
 			continue;
 		if (functions_cnt) {
-			struct elf_function *func;
+			const char *name = function__name(fn, cu);
+			struct elf_function *func = NULL;
 
-			func = find_function(btfe, function__name(fn, cu));
+			if (name)
+				func = find_function(btfe, name);
 			if (!func || func->generated)
 				continue;
 			func->generated = true;