| Message ID | 20241005000147.723515-1-stephen.s.brennan@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [dwarves] btf_encoder: fix reversed condition for matching ELF section | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 > >
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 >> >>
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
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 &&
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(-)