[PATCHv2 0/3] dwarves,libbpf: Add support to use optional extended section index table

hi,
kpatch guys hit an issue with pahole over their vmlinux, which
contains many (over 100000) sections, pahole crashes.

With so many sections, ELF is using extended section index table,
which is used to hold values for some of the indexes and extra
code is needed to retrieve them.

This patchset adds the support for pahole to properly read string
table index and symbol's section index, which are used in btf_encoder.

This patchset also adds support for libbpf to properly parse .BTF
section on such object.

This patchset is based on previously posted fix [1].

v2 changes:
  - many variables renames [Andrii]
  - use elf_getshdrstrndx() unconditionally [Andrii]
  - add elf_symtab__for_each_symbol_index macro [Andrii]
  - add more comments [Andrii]
  - verify that extended symtab section type is SHT_SYMTAB_SHNDX [Andrii]
  - fix Joe's crash in dwarves build, wrong sym.st_shndx assignment

thanks,
jirka


[1] https://lore.kernel.org/bpf/20210113102509.1338601-1-jolsa@kernel.org/
---
dwarves:

Jiri Olsa (2):
      elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name
      bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

 btf_encoder.c | 36 ++++++++++++++++++++++++++++++++----
 dutil.c       |  8 ++++++--
 elf_symtab.c  | 39 ++++++++++++++++++++++++++++++++++++++-
 elf_symtab.h  |  2 ++
 4 files changed, 78 insertions(+), 7 deletions(-)


libbpf:

Jiri Olsa (1):
      libbpf: Use string table index from index table if needed

 tools/lib/bpf/btf.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

In case the elf's header e_shstrndx contains SHN_XINDEX,
we need to call elf_getshdrstrndx to get the proper
string table index.

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

diff --git a/dutil.c b/dutil.c
index 7b667647420f..9e0fdca3ae04 100644
--- a/dutil.c
+++ b/dutil.c

@@ -179,13 +179,17 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
 {
 	Elf_Scn *sec = NULL;
 	size_t cnt = 1;
+	size_t str_idx;
+
+	if (elf_getshdrstrndx(elf, &str_idx))
+		return NULL;
 
 	while ((sec = elf_nextscn(elf, sec)) != NULL) {
 		char *str;
 
 		gelf_getshdr(sec, shp);
-		str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
-		if (!strcmp(name, str)) {
+		str = elf_strptr(elf, str_idx, shp->sh_name);
+		if (str && !strcmp(name, str)) {
 			if (index)
 				*index = cnt;
 			break;

-- 
2.27.0

For very large ELF objects (with many sections), we could
get special value SHN_XINDEX (65535) for symbol's st_shndx.

This patch is adding code to detect the optional extended
section index table and use it to resolve symbol's section
index.

Adding elf_symtab__for_each_symbol_index macro that returns
symbol's section index and usign it in collect_symbols function.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 36 ++++++++++++++++++++++++++++++++----
 elf_symtab.c  | 39 ++++++++++++++++++++++++++++++++++++++-
 elf_symtab.h  |  2 ++
 3 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 5557c9efd365..6e6f22c438ce 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c

@@ -585,12 +585,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
 	return 0;
 }
 
-static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
+static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
+			   Elf32_Word sym_sec_idx)
 {
 	if (!fl->mcount_start &&
 	    !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
 		fl->mcount_start = sym->st_value;
-		fl->mcount_sec_idx = sym->st_shndx;
+		fl->mcount_sec_idx = sym_sec_idx;
 	}
 
 	if (!fl->mcount_stop &&

@@ -598,9 +599,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
 		fl->mcount_stop = sym->st_value;
 }
 
+static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
+			 int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
+{
+	if (!gelf_getsym(syms, id, sym))
+		return false;
+
+	*sym_sec_idx = sym->st_shndx;
+
+	if (sym->st_shndx == SHN_XINDEX) {
+		if (!syms_sec_idx_table)
+			return false;
+		if (!gelf_getsymshndx(syms, syms_sec_idx_table,
+				      id, sym, sym_sec_idx))
+			return false;
+	}
+
+	return true;
+}
+
+#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)		\
+	for (id = 0, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,	\
+				  id, &sym, &sym_sec_idx);			\
+	     id < symtab->nr_syms;						\
+	     id++, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,	\
+				id, &sym, &sym_sec_idx))
+
 static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 {
 	struct funcs_layout fl = { };
+	Elf32_Word sym_sec_idx;
 	uint32_t core_id;
 	GElf_Sym sym;
 

@@ -608,12 +636,12 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 	percpu_var_cnt = 0;
 
 	/* search within symtab for percpu variables */
-	elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
+	elf_symtab__for_each_symbol_index(btfe->symtab, core_id, sym, sym_sec_idx) {
 		if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
 			return -1;
 		if (collect_function(btfe, &sym))
 			return -1;
-		collect_symbol(&sym, &fl);
+		collect_symbol(&sym, &fl, sym_sec_idx);
 	}
 
 	if (collect_percpu_vars) {

diff --git a/elf_symtab.c b/elf_symtab.c
index 741990ea3ed9..fad5e0c0ba3c 100644
--- a/elf_symtab.c
+++ b/elf_symtab.c

@@ -17,11 +17,13 @@
 
 struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
 {
+	size_t symtab_index;
+
 	if (name == NULL)
 		name = ".symtab";
 
 	GElf_Shdr shdr;
-	Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, NULL);
+	Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, &symtab_index);
 
 	if (sec == NULL)
 		return NULL;

@@ -41,6 +43,12 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
 	if (symtab->syms == NULL)
 		goto out_free_name;
 
+	/*
+	 * This returns extended section index table's
+	 * section index, if it exists.
+	 */
+	int symtab_xindex = elf_scnshndx(sec);
+
 	sec = elf_getscn(elf, shdr.sh_link);
 	if (sec == NULL)
 		goto out_free_name;

@@ -49,6 +57,35 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
 	if (symtab->symstrs == NULL)
 		goto out_free_name;
 
+	/*
+	 * The .symtab section has optional extended section index
+	 * table, load its data so it can be used to resolve symbol's
+	 * section index.
+	 **/
+	if (symtab_xindex > 0) {
+		GElf_Shdr shdr_xindex;
+		Elf_Scn *sec_xindex;
+
+		sec_xindex = elf_getscn(elf, symtab_xindex);
+		if (sec_xindex == NULL)
+			goto out_free_name;
+
+		if (gelf_getshdr(sec_xindex, &shdr_xindex) == NULL)
+			goto out_free_name;
+
+		/* Extra check to verify it's correct type */
+		if (shdr_xindex.sh_type != SHT_SYMTAB_SHNDX)
+			goto out_free_name;
+
+		/* Extra check to verify it belongs to the .symtab */
+		if (symtab_index != shdr_xindex.sh_link)
+			goto out_free_name;
+
+		symtab->syms_sec_idx_table = elf_getdata(elf_getscn(elf, symtab_xindex), NULL);
+		if (symtab->syms_sec_idx_table == NULL)
+			goto out_free_name;
+	}
+
 	symtab->nr_syms = shdr.sh_size / shdr.sh_entsize;
 
 	return symtab;

diff --git a/elf_symtab.h b/elf_symtab.h
index 359add69c8ab..2e05ca98158b 100644
--- a/elf_symtab.h
+++ b/elf_symtab.h

@@ -16,6 +16,8 @@ struct elf_symtab {
 	uint32_t  nr_syms;
 	Elf_Data  *syms;
 	Elf_Data  *symstrs;
+	/* Data of SHT_SYMTAB_SHNDX section. */
+	Elf_Data  *syms_sec_idx_table;
 	char	  *name;
 };
 

-- 
2.27.0

For very large ELF objects (with many sections), we could
get special value SHN_XINDEX (65535) for elf object's string
table index - e_shstrndx.

Call elf_getshdrstrndx to get the proper string table index,
instead of reading it directly from ELF header.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/btf.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 9970a288dda5..d9c10830d749 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c

@@ -858,6 +858,7 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
 	Elf_Scn *scn = NULL;
 	Elf *elf = NULL;
 	GElf_Ehdr ehdr;
+	size_t shstrndx;
 
 	if (elf_version(EV_CURRENT) == EV_NONE) {
 		pr_warn("failed to init libelf for %s\n", path);

@@ -882,7 +883,14 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
 		pr_warn("failed to get EHDR from %s\n", path);
 		goto done;
 	}
-	if (!elf_rawdata(elf_getscn(elf, ehdr.e_shstrndx), NULL)) {
+
+	if (elf_getshdrstrndx(elf, &shstrndx)) {
+		pr_warn("failed to get section names section index for %s\n",
+			path);
+		goto done;
+	}
+
+	if (!elf_rawdata(elf_getscn(elf, shstrndx), NULL)) {
 		pr_warn("failed to get e_shstrndx from %s\n", path);
 		goto done;
 	}

@@ -897,7 +905,7 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
 				idx, path);
 			goto done;
 		}
-		name = elf_strptr(elf, ehdr.e_shstrndx, sh.sh_name);
+		name = elf_strptr(elf, shstrndx, sh.sh_name);
 		if (!name) {
 			pr_warn("failed to get section(%d) name from %s\n",
 				idx, path);

-- 
2.27.0

On Thu, Jan 21, 2021 at 12:24 PM Jiri Olsa [off-list ref] wrote:

if (!str) would be an error? should we bail out here?

                        if (index)
                                *index = cnt;
                        break;
--
2.27.0

On Thu, Jan 21, 2021 at 12:25 PM Jiri Olsa [off-list ref] wrote:

For very large ELF objects (with many sections), we could
get special value SHN_XINDEX (65535) for symbol's st_shndx.

This patch is adding code to detect the optional extended
section index table and use it to resolve symbol's section
index.

Adding elf_symtab__for_each_symbol_index macro that returns
symbol's section index and usign it in collect_symbols function.

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

You missed fixing up collect_function() as well, which is using
elf_sym__section(), which doesn't know about extended numbering.

gelf_getsymshndx() is supposed to work even for cases that don't use
extended numbering, so this should work, right?

if (!gelf_getsymshndx(syms, syms_sec_idx_table, id, sym, sym_sec_idx))
    return false;

if (sym->st_shndx == SHN_XINDEX)
  *sym_sec_idx = sym->st_shndx;

return true;

?

+                       return false;
+       }
+
+       return true;
+}
+
+#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)                \
+       for (id = 0, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,     \
+                                 id, &sym, &sym_sec_idx);                      \
+            id < symtab->nr_syms;                                              \
+            id++, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,       \
+                               id, &sym, &sym_sec_idx))

what do we want to do if elf_sym__get() returns error (false)? We can
either stop or ignore that symbol, right? But currently you are
returning invalid symbol data.

so either

for (id = 0; id < symtab->nr_syms && elf_sym__get(symtab->syms,
symtab->syms_sec_idx_table, d, &sym, &sym_sec_idx); id++)

or

for (id = 0; id < symtab->nr_syms; id++)
  if (elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, d, &sym,
&sym_sec_idx))


But the current variant looks broken. Oh, and
elf_symtab__for_each_symbol() is similarly broken, can you please fix
that as well?

And this new macro should probably be in elf_symtab.h, along the
elf_symtab__for_each_symbol.

+
 static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 {
        struct funcs_layout fl = { };
+       Elf32_Word sym_sec_idx;
        uint32_t core_id;
        GElf_Sym sym;

[...]

On Thu, Jan 21, 2021 at 03:10:25PM -0800, Andrii Nakryiko wrote:

On Thu, Jan 21, 2021 at 12:24 PM Jiri Olsa [off-list ref] wrote:

quoted

In case the elf's header e_shstrndx contains SHN_XINDEX,
we need to call elf_getshdrstrndx to get the proper
string table index.

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

diff --git a/dutil.c b/dutil.c
index 7b667647420f..9e0fdca3ae04 100644
--- a/dutil.c
+++ b/dutil.c

@@ -179,13 +179,17 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
 {
        Elf_Scn *sec = NULL;
        size_t cnt = 1;
+       size_t str_idx;
+
+       if (elf_getshdrstrndx(elf, &str_idx))
+               return NULL;

        while ((sec = elf_nextscn(elf, sec)) != NULL) {
                char *str;

                gelf_getshdr(sec, shp);
-               str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
-               if (!strcmp(name, str)) {
+               str = elf_strptr(elf, str_idx, shp->sh_name);
+               if (str && !strcmp(name, str)) {

if (!str) would be an error? should we bail out here?

seems like if elf_nextscn returns NULL, it will be the case for all the
sections in here.. but bailing out on (!str) is more direct and safer

I'll send an update

thanks,
jirka

quoted

                        if (index)
                                *index = cnt;
                        break;
--
2.27.0

On Thu, Jan 21, 2021 at 12:26 PM Jiri Olsa [off-list ref] wrote:

For very large ELF objects (with many sections), we could
get special value SHN_XINDEX (65535) for elf object's string
table index - e_shstrndx.

Call elf_getshdrstrndx to get the proper string table index,
instead of reading it directly from ELF header.

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

I've applied this patch to bpf-next, you don't need to re-send it in
the next version of this patch set.

On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:

On Thu, Jan 21, 2021 at 12:25 PM Jiri Olsa [off-list ref] wrote:

quoted

For very large ELF objects (with many sections), we could
get special value SHN_XINDEX (65535) for symbol's st_shndx.

This patch is adding code to detect the optional extended
section index table and use it to resolve symbol's section
index.

Adding elf_symtab__for_each_symbol_index macro that returns
symbol's section index and usign it in collect_symbols function.

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

You missed fixing up collect_function() as well, which is using
elf_sym__section(), which doesn't know about extended numbering.

ah right, it's for modules, I guess it's why it did not show up

thanks,
jirka

On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:

SNIP

quoted

@@ -598,9 +599,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
                fl->mcount_stop = sym->st_value;
 }

+static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
+                        int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
+{
+       if (!gelf_getsym(syms, id, sym))
+               return false;
+
+       *sym_sec_idx = sym->st_shndx;
+
+       if (sym->st_shndx == SHN_XINDEX) {
+               if (!syms_sec_idx_table)
+                       return false;
+               if (!gelf_getsymshndx(syms, syms_sec_idx_table,
+                                     id, sym, sym_sec_idx))

gelf_getsymshndx() is supposed to work even for cases that don't use
extended numbering, so this should work, right?

if (!gelf_getsymshndx(syms, syms_sec_idx_table, id, sym, sym_sec_idx))
    return false;

it seems you're right, gelf_getsymshndx seem to work for
both cases, I'll check

if (sym->st_shndx == SHN_XINDEX)
  *sym_sec_idx = sym->st_shndx;

I don't understand this..  gelf_getsymshndx will return both
symbol and proper index, no? also sym_sec_idx is already
assigned from previou call

return true;

?

quoted

+                       return false;
+       }
+
+       return true;
+}
+
+#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)                \
+       for (id = 0, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,     \
+                                 id, &sym, &sym_sec_idx);                      \
+            id < symtab->nr_syms;                                              \
+            id++, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,       \
+                               id, &sym, &sym_sec_idx))

what do we want to do if elf_sym__get() returns error (false)? We can
either stop or ignore that symbol, right? But currently you are
returning invalid symbol data.

so either

for (id = 0; id < symtab->nr_syms && elf_sym__get(symtab->syms,
symtab->syms_sec_idx_table, d, &sym, &sym_sec_idx); id++)

or

for (id = 0; id < symtab->nr_syms; id++)
  if (elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, d, &sym,
&sym_sec_idx))

if we go ahead with skipping symbols, this one seems good

But the current variant looks broken. Oh, and
elf_symtab__for_each_symbol() is similarly broken, can you please fix
that as well?

And this new macro should probably be in elf_symtab.h, along the
elf_symtab__for_each_symbol.

thanks,
jirka

On Fri, Jan 22, 2021 at 12:47 PM Jiri Olsa [off-list ref] wrote:

On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:

SNIP

quoted

quoted

@@ -598,9 +599,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
                fl->mcount_stop = sym->st_value;
 }

+static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
+                        int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
+{
+       if (!gelf_getsym(syms, id, sym))
+               return false;
+
+       *sym_sec_idx = sym->st_shndx;
+
+       if (sym->st_shndx == SHN_XINDEX) {
+               if (!syms_sec_idx_table)
+                       return false;
+               if (!gelf_getsymshndx(syms, syms_sec_idx_table,
+                                     id, sym, sym_sec_idx))

gelf_getsymshndx() is supposed to work even for cases that don't use
extended numbering, so this should work, right?

if (!gelf_getsymshndx(syms, syms_sec_idx_table, id, sym, sym_sec_idx))
    return false;

it seems you're right, gelf_getsymshndx seem to work for
both cases, I'll check

quoted

if (sym->st_shndx == SHN_XINDEX)
  *sym_sec_idx = sym->st_shndx;

I don't understand this..  gelf_getsymshndx will return both
symbol and proper index, no? also sym_sec_idx is already
assigned from previou call

Reading (some) implementation of gelf_getsymshndx() that I found
online, it won't set sym_sec_idx, if the symbol *doesn't* use extended
numbering. But it will still return symbol data. So to return the
section index in all cases, we need to check again *after* we got
symbol, and if it's not extended, then set index manually.

quoted

return true;

?

quoted

+                       return false;
+       }
+
+       return true;
+}
+
+#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)                \
+       for (id = 0, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,     \
+                                 id, &sym, &sym_sec_idx);                      \
+            id < symtab->nr_syms;                                              \
+            id++, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,       \
+                               id, &sym, &sym_sec_idx))

what do we want to do if elf_sym__get() returns error (false)? We can
either stop or ignore that symbol, right? But currently you are
returning invalid symbol data.

so either

for (id = 0; id < symtab->nr_syms && elf_sym__get(symtab->syms,
symtab->syms_sec_idx_table, d, &sym, &sym_sec_idx); id++)

or

for (id = 0; id < symtab->nr_syms; id++)
  if (elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, d, &sym,
&sym_sec_idx))

if we go ahead with skipping symbols, this one seems good

I think skipping symbols is nicer. If ELF is totally broken, then all
symbols are going to be ignored anyway. If it's some one-off issue for
a specific symbol, we'll just ignore it (unfortunately, silently).

quoted

But the current variant looks broken. Oh, and
elf_symtab__for_each_symbol() is similarly broken, can you please fix
that as well?

And this new macro should probably be in elf_symtab.h, along the
elf_symtab__for_each_symbol.

thanks,
jirka

On Fri, Jan 22, 2021 at 02:55:51PM -0800, Andrii Nakryiko wrote:

On Fri, Jan 22, 2021 at 12:47 PM Jiri Olsa [off-list ref] wrote:

quoted

On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:

SNIP

quoted

quoted

@@ -598,9 +599,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
                fl->mcount_stop = sym->st_value;
 }

+static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
+                        int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
+{
+       if (!gelf_getsym(syms, id, sym))
+               return false;
+
+       *sym_sec_idx = sym->st_shndx;
+
+       if (sym->st_shndx == SHN_XINDEX) {
+               if (!syms_sec_idx_table)
+                       return false;
+               if (!gelf_getsymshndx(syms, syms_sec_idx_table,
+                                     id, sym, sym_sec_idx))

gelf_getsymshndx() is supposed to work even for cases that don't use
extended numbering, so this should work, right?

if (!gelf_getsymshndx(syms, syms_sec_idx_table, id, sym, sym_sec_idx))
    return false;

it seems you're right, gelf_getsymshndx seem to work for
both cases, I'll check

quoted

if (sym->st_shndx == SHN_XINDEX)
  *sym_sec_idx = sym->st_shndx;

I don't understand this..  gelf_getsymshndx will return both
symbol and proper index, no? also sym_sec_idx is already
assigned from previou call

Reading (some) implementation of gelf_getsymshndx() that I found
online, it won't set sym_sec_idx, if the symbol *doesn't* use extended
numbering. But it will still return symbol data. So to return the

the latest upstream code seems to set it always,
but I agree we should be careful

Mark, any insight in here? thanks

section index in all cases, we need to check again *after* we got
symbol, and if it's not extended, then set index manually.

hum, then we should use '!=', right?

  if (sym->st_shndx != SHN_XINDEX)
    *sym_sec_idx = sym->st_shndx;

SNIP

quoted

quoted

so either

for (id = 0; id < symtab->nr_syms && elf_sym__get(symtab->syms,
symtab->syms_sec_idx_table, d, &sym, &sym_sec_idx); id++)

or

for (id = 0; id < symtab->nr_syms; id++)
  if (elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, d, &sym,
&sym_sec_idx))

if we go ahead with skipping symbols, this one seems good

I think skipping symbols is nicer. If ELF is totally broken, then all
symbols are going to be ignored anyway. If it's some one-off issue for
a specific symbol, we'll just ignore it (unfortunately, silently).

agreed, I'll use this

thanks,
jirka

On Sat, Jan 23, 2021 at 10:51 AM Jiri Olsa [off-list ref] wrote:

On Fri, Jan 22, 2021 at 02:55:51PM -0800, Andrii Nakryiko wrote:

quoted

On Fri, Jan 22, 2021 at 12:47 PM Jiri Olsa [off-list ref] wrote:

quoted

On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:

SNIP

quoted

quoted

@@ -598,9 +599,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
                fl->mcount_stop = sym->st_value;
 }

+static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
+                        int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
+{
+       if (!gelf_getsym(syms, id, sym))
+               return false;
+
+       *sym_sec_idx = sym->st_shndx;
+
+       if (sym->st_shndx == SHN_XINDEX) {
+               if (!syms_sec_idx_table)
+                       return false;
+               if (!gelf_getsymshndx(syms, syms_sec_idx_table,
+                                     id, sym, sym_sec_idx))

gelf_getsymshndx() is supposed to work even for cases that don't use
extended numbering, so this should work, right?

if (!gelf_getsymshndx(syms, syms_sec_idx_table, id, sym, sym_sec_idx))
    return false;

it seems you're right, gelf_getsymshndx seem to work for
both cases, I'll check

quoted

if (sym->st_shndx == SHN_XINDEX)
  *sym_sec_idx = sym->st_shndx;

I don't understand this..  gelf_getsymshndx will return both
symbol and proper index, no? also sym_sec_idx is already
assigned from previou call

Reading (some) implementation of gelf_getsymshndx() that I found
online, it won't set sym_sec_idx, if the symbol *doesn't* use extended
numbering. But it will still return symbol data. So to return the

the latest upstream code seems to set it always,
but I agree we should be careful

oh, then maybe it's not necessary. I honestly don't even know where
the authoritative source code of libelf is, so I just found some
random source code with Google.

Mark, any insight in here? thanks

quoted

section index in all cases, we need to check again *after* we got
symbol, and if it's not extended, then set index manually.

hum, then we should use '!=', right?

  if (sym->st_shndx != SHN_XINDEX)
    *sym_sec_idx = sym->st_shndx;

yeah, sorry, that was a typo

SNIP

quoted

quoted

quoted

so either

for (id = 0; id < symtab->nr_syms && elf_sym__get(symtab->syms,
symtab->syms_sec_idx_table, d, &sym, &sym_sec_idx); id++)

or

for (id = 0; id < symtab->nr_syms; id++)
  if (elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, d, &sym,
&sym_sec_idx))

if we go ahead with skipping symbols, this one seems good

I think skipping symbols is nicer. If ELF is totally broken, then all
symbols are going to be ignored anyway. If it's some one-off issue for
a specific symbol, we'll just ignore it (unfortunately, silently).

agreed, I'll use this

sounds good

thanks,
jirka

On Sat, Jan 23, 2021 at 09:08:15PM +0100, Mark Wielaard wrote:

Hi Jiri,

On Sat, 2021-01-23 at 19:51 +0100, Jiri Olsa wrote:

quoted

On Fri, Jan 22, 2021 at 02:55:51PM -0800, Andrii Nakryiko wrote:

quoted

quoted

I don't understand this..  gelf_getsymshndx will return both
symbol and proper index, no? also sym_sec_idx is already
assigned from previou call

Reading (some) implementation of gelf_getsymshndx() that I found
online, it won't set sym_sec_idx, if the symbol *doesn't* use
extended
numbering. But it will still return symbol data. So to return the

the latest upstream code seems to set it always,
but I agree we should be careful

Mark, any insight in here? thanks

GElf_Sym *
gelf_getsymshndx (Elf_Data *symdata, Elf_Data *shndxdata, int ndx,
                  GElf_Sym *dst, Elf32_Word *dstshndx)

Will always set *dst, but only set *dstshndx if both it and shndxdata
are not NULL and no error occurred (the function returns NULL and set
libelf_error in case of error).

So as long as shndxdata != NULL you can rely on *dstshndx being set.
Otherwise you get the section index from dst->st_shndx.

ok, so it's as Andrii said, I'll make the extra check then

thanks,
jirka

Hi Jiri,

On Sat, 2021-01-23 at 19:51 +0100, Jiri Olsa wrote:

On Fri, Jan 22, 2021 at 02:55:51PM -0800, Andrii Nakryiko wrote:

quoted

quoted

I don't understand this..  gelf_getsymshndx will return both
symbol and proper index, no? also sym_sec_idx is already
assigned from previou call

Reading (some) implementation of gelf_getsymshndx() that I found
online, it won't set sym_sec_idx, if the symbol *doesn't* use
extended
numbering. But it will still return symbol data. So to return the

the latest upstream code seems to set it always,
but I agree we should be careful

Mark, any insight in here? thanks

GElf_Sym *
gelf_getsymshndx (Elf_Data *symdata, Elf_Data *shndxdata, int ndx,
                  GElf_Sym *dst, Elf32_Word *dstshndx)

Will always set *dst, but only set *dstshndx if both it and shndxdata
are not NULL and no error occurred (the function returns NULL and set
libelf_error in case of error).

So as long as shndxdata != NULL you can rely on *dstshndx being set.
Otherwise you get the section index from dst->st_shndx.

Cheers,

Mark

Hi,

On Sat, 2021-01-23 at 12:07 -0800, Andrii Nakryiko wrote:

quoted

the latest upstream code seems to set it always,
but I agree we should be careful

oh, then maybe it's not necessary. I honestly don't even know where
the authoritative source code of libelf is, so I just found some
random source code with Google.

The elfutils.org libelf implementation can be found here:
https://sourceware.org/git/?p=elfutils.git;a=tree;f=libelf;hb=HEAD

There are some other implementations, but some aren't maintained and
others aren't packaged for any distro (anymore). libelf is a semi-
standard "SVR4 Unix" library, so you might also find it for some none
GNU/Linux OSes like Solaris. The ELF specification itself is contained
in the System V Application Binary Interface (gABI). The libelf library
itself isn't actually officially part of the specification. But we
still do try to keep the implementations (source) compatible through
the generic-abi mailinglist.

Cheers,

Mark

On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:

SNIP

But the current variant looks broken. Oh, and
elf_symtab__for_each_symbol() is similarly broken, can you please fix
that as well?

we'll have to change its callers a bit, because of hanging 'else'
I'll send this separately if that's ok, when I figure out how to
test ctf code

jirka


---

diff --git a/elf_symtab.h b/elf_symtab.h
index 489e2b1a3505..6823a8c37ecf 100644
--- a/elf_symtab.h
+++ b/elf_symtab.h

@@ -99,10 +99,9 @@ elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
  * @index: uint32_t index
  * @sym: GElf_Sym iterator
  */
-#define elf_symtab__for_each_symbol(symtab, index, sym) \
-	for (index = 0, gelf_getsym(symtab->syms, index, &sym);\
-	     index < symtab->nr_syms; \
-	     index++, gelf_getsym(symtab->syms, index, &sym))
+#define elf_symtab__for_each_symbol(symtab, index, sym)		\
+	for (index = 0; index < symtab->nr_syms; index++)	\
+		if (gelf_getsym(symtab->syms, index, &sym))
 
 /**
  * elf_symtab__for_each_symbol_index - iterate through all the symbols,

diff --git a/libctf.h b/libctf.h
index 749be8955c52..ee5412bec77e 100644
--- a/libctf.h
+++ b/libctf.h

@@ -90,11 +90,9 @@ char *ctf__string(struct ctf *ctf, uint32_t ref);
  */
 #define ctf__for_each_symtab_function(ctf, index, sym)			      \
 	elf_symtab__for_each_symbol(ctf->symtab, index, sym)		      \
-		if (ctf__ignore_symtab_function(&sym,			      \
+		if (!ctf__ignore_symtab_function(&sym,			      \
 						elf_sym__name(&sym,	      \
 							      ctf->symtab)))  \
-			continue;					      \
-		else
 
 /**
  * ctf__for_each_symtab_object - iterate thru all the symtab objects

@@ -105,11 +103,9 @@ char *ctf__string(struct ctf *ctf, uint32_t ref);
  */
 #define ctf__for_each_symtab_object(ctf, index, sym)			      \
 	elf_symtab__for_each_symbol(ctf->symtab, index, sym)		      \
-		if (ctf__ignore_symtab_object(&sym,			      \
+		if (!ctf__ignore_symtab_object(&sym,			      \
 					      elf_sym__name(&sym,	      \
 							    ctf->symtab)))    \
-			continue;					      \
-		else
 
 
 #endif /* _LIBCTF_H */

On Sat, Jan 23, 2021 at 1:23 PM Jiri Olsa [off-list ref] wrote:

On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:

SNIP

quoted

But the current variant looks broken. Oh, and
elf_symtab__for_each_symbol() is similarly broken, can you please fix
that as well?

we'll have to change its callers a bit, because of hanging 'else'
I'll send this separately if that's ok, when I figure out how to
test ctf code

oh, else sucks. Sure, no problem doing it separately.