[PATCH dwarves] btf_encoder: fix reversed condition for matching ELF section

[PATCH dwarves] btf_encoder: fix reversed condition for matching ELF section

[Stephen Brennan]

We only want to consider PROGBITS and NOBITS. However, when refactoring
this function for clarity, I managed to miss flip this condition. The
result is fewer variables output, and bad section names used for the
ones that are emitted.

Fixes: bf2eedb ("btf_encoder: Stop indexing symbols for VARs")

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---

Hi Arnaldo,

This clearly slipped by me in my last small edit based on Alan's feedback, and I
didn't run a full enough validation test after the last tweak since it was "just
some small nits".

(His code review suggestion was not buggy... I introduced it as I shoddily
redid his suggestion).

Sorry for the bug introduced at the last second - feel free to fold this into
the commit or keep the commit as a monument to the bug :)

Thanks,
Stephen

 btf_encoder.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 201a48c..5954238 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -2137,8 +2137,8 @@ static size_t get_elf_section(struct btf_encoder *encoder, uint64_t addr)
 	/* Start at index 1 to ignore initial SHT_NULL section */
 	for (size_t i = 1; i < encoder->seccnt; i++) {
 		/* Variables are only present in PROGBITS or NOBITS (.bss) */
-		if (encoder->secinfo[i].type == SHT_PROGBITS ||
-		    encoder->secinfo[i].type == SHT_NOBITS)
+		if (!(encoder->secinfo[i].type == SHT_PROGBITS ||
+		     encoder->secinfo[i].type == SHT_NOBITS))
 			continue;
 
 		if (encoder->secinfo[i].addr <= addr &&
-- 
2.43.5

Re: [PATCH dwarves] btf_encoder: fix reversed condition for matching ELF section

[Jiri Olsa]

On Fri, Oct 04, 2024 at 05:01:46PM -0700, Stephen Brennan wrote:
> We only want to consider PROGBITS and NOBITS. However, when refactoring
> this function for clarity, I managed to miss flip this condition. The
> result is fewer variables output, and bad section names used for the
> ones that are emitted.
> 
> Fixes: bf2eedb ("btf_encoder: Stop indexing symbols for VARs")
> 
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> ---
> 
> Hi Arnaldo,
> 
> This clearly slipped by me in my last small edit based on Alan's feedback, and I
> didn't run a full enough validation test after the last tweak since it was "just
> some small nits".
> 
> (His code review suggestion was not buggy... I introduced it as I shoddily
> redid his suggestion).
> 
> Sorry for the bug introduced at the last second - feel free to fold this into
> the commit or keep the commit as a monument to the bug :)
> 
> Thanks,
> Stephen
> 
>  btf_encoder.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

nice ;-) lgtm

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

jirka

> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 201a48c..5954238 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -2137,8 +2137,8 @@ static size_t get_elf_section(struct btf_encoder *encoder, uint64_t addr)
>  	/* Start at index 1 to ignore initial SHT_NULL section */
>  	for (size_t i = 1; i < encoder->seccnt; i++) {
>  		/* Variables are only present in PROGBITS or NOBITS (.bss) */
> -		if (encoder->secinfo[i].type == SHT_PROGBITS ||
> -		    encoder->secinfo[i].type == SHT_NOBITS)
> +		if (!(encoder->secinfo[i].type == SHT_PROGBITS ||
> +		     encoder->secinfo[i].type == SHT_NOBITS))
>  			continue;
>  
>  		if (encoder->secinfo[i].addr <= addr &&
> -- 
> 2.43.5
> 
> 

Re: [PATCH dwarves] btf_encoder: fix reversed condition for matching ELF section

[Alan Maguire]

On 07/10/2024 09:14, Jiri Olsa wrote:
> On Fri, Oct 04, 2024 at 05:01:46PM -0700, Stephen Brennan wrote:
>> We only want to consider PROGBITS and NOBITS. However, when refactoring
>> this function for clarity, I managed to miss flip this condition. The
>> result is fewer variables output, and bad section names used for the
>> ones that are emitted.
>>
>> Fixes: bf2eedb ("btf_encoder: Stop indexing symbols for VARs")
>>
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>> ---
>>
>> Hi Arnaldo,
>>
>> This clearly slipped by me in my last small edit based on Alan's feedback, and I
>> didn't run a full enough validation test after the last tweak since it was "just
>> some small nits".
>>
>> (His code review suggestion was not buggy... I introduced it as I shoddily
>> redid his suggestion).
>>
>> Sorry for the bug introduced at the last second - feel free to fold this into
>> the commit or keep the commit as a monument to the bug :)
>>
>> Thanks,
>> Stephen
>>
>>  btf_encoder.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> nice ;-) lgtm
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> 

Thanks for the quick fix!

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> jirka
> 
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index 201a48c..5954238 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -2137,8 +2137,8 @@ static size_t get_elf_section(struct btf_encoder *encoder, uint64_t addr)
>>  	/* Start at index 1 to ignore initial SHT_NULL section */
>>  	for (size_t i = 1; i < encoder->seccnt; i++) {
>>  		/* Variables are only present in PROGBITS or NOBITS (.bss) */
>> -		if (encoder->secinfo[i].type == SHT_PROGBITS ||
>> -		    encoder->secinfo[i].type == SHT_NOBITS)
>> +		if (!(encoder->secinfo[i].type == SHT_PROGBITS ||
>> +		     encoder->secinfo[i].type == SHT_NOBITS))
>>  			continue;
>>  
>>  		if (encoder->secinfo[i].addr <= addr &&
>> -- 
>> 2.43.5
>>
>>

Re: [PATCH dwarves] btf_encoder: fix reversed condition for matching ELF section

[Arnaldo Carvalho de Melo]

On Fri, Oct 04, 2024 at 05:01:46PM -0700, Stephen Brennan wrote:
> We only want to consider PROGBITS and NOBITS. However, when refactoring
> this function for clarity, I managed to miss flip this condition. The
> result is fewer variables output, and bad section names used for the
> ones that are emitted.
> 
> Fixes: bf2eedb ("btf_encoder: Stop indexing symbols for VARs")
> 
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> ---
> 
> Hi Arnaldo,
> 
> This clearly slipped by me in my last small edit based on Alan's feedback, and I
> didn't run a full enough validation test after the last tweak since it was "just
> some small nits".
> 
> (His code review suggestion was not buggy... I introduced it as I shoddily
> redid his suggestion).
> 
> Sorry for the bug introduced at the last second - feel free to fold this into
> the commit or keep the commit as a monument to the bug :)

Nope, as it was just in the next branch, I folded it into the fixed
commit and kept just a lore link to this fixup, all is back in next now
and I'm redoing tests here.

Thanks for realizing the mistake and providing a fix, and thanks to Jiri
and Alan for reviewing the fix.

- Arnaldo
 
> Thanks,
> Stephen
> 
>  btf_encoder.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 201a48c..5954238 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -2137,8 +2137,8 @@ static size_t get_elf_section(struct btf_encoder *encoder, uint64_t addr)
>  	/* Start at index 1 to ignore initial SHT_NULL section */
>  	for (size_t i = 1; i < encoder->seccnt; i++) {
>  		/* Variables are only present in PROGBITS or NOBITS (.bss) */
> -		if (encoder->secinfo[i].type == SHT_PROGBITS ||
> -		    encoder->secinfo[i].type == SHT_NOBITS)
> +		if (!(encoder->secinfo[i].type == SHT_PROGBITS ||
> +		     encoder->secinfo[i].type == SHT_NOBITS))
>  			continue;
>  
>  		if (encoder->secinfo[i].addr <= addr &&
> -- 
> 2.43.5