[PATCH v4 00/10] Add jump table support for objtool on LoongArch

[PATCH v4 00/10] Add jump table support for objtool on LoongArch

[Tiezhu Yang]

This series is based on 6.12-rc7, tested with the latest upstream
mainline Binutils, GCC and Clang, most of the patches are aim to
handle the special cases compiled with Clang on LoongArch.

v4:
  -- Avoid EM_LOONGARCH and R_LARCH_32_PCREL undeclared error
     for various compiling environments.

  -- Remove the check condition "dest_insn->type == INSN_NOP"
     for unreachable entry of rodata.

Tiezhu Yang (10):
  objtool: Handle various symbol types of rodata
  objtool: Handle special cases of dead end insn
  objtool: Handle different entry size of rodata
  objtool: Handle PC relative relocation type
  objtool: Handle unreachable entry of rodata
  objtool: Handle unsorted table offset of rodata
  objtool/LoongArch: Get each table size of rodata
  objtool/LoongArch: Add support for switch table
  objtool/LoongArch: Add support for goto table
  LoongArch: Enable jump table for objtool

 arch/loongarch/Kconfig                 |   3 +
 arch/loongarch/Makefile                |   4 +
 tools/objtool/arch/loongarch/special.c | 156 ++++++++++++++++++++++++-
 tools/objtool/check.c                  |  75 +++++++++++-
 tools/objtool/include/objtool/check.h  |   1 +
 5 files changed, 233 insertions(+), 6 deletions(-)

-- 
2.42.0

[PATCH v4 01/10] objtool: Handle various symbol types of rodata

[Tiezhu Yang]

In the relocation section ".rela.rodata" of each .o file compiled with
LoongArch toolchain, there are various symbol types such as STT_NOTYPE,
STT_OBJECT, STT_FUNC in addition to the usual STT_SECTION, it needs to
use reloc symbol offset instead of reloc addend to find the destination
instruction in find_jump_table() and add_jump_table().

This is preparation for later patch on LoongArch, there is no effect for
the other archs with this patch.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 tools/objtool/check.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 6604f5d038aa..9601235e908d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2079,6 +2079,7 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 	unsigned int prev_offset = 0;
 	struct reloc *reloc = table;
 	struct alternative *alt;
+	unsigned long offset;
 
 	/*
 	 * Each @reloc is a switch table relocation which points to the target
@@ -2094,12 +2095,19 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 		if (prev_offset && reloc_offset(reloc) != prev_offset + 8)
 			break;
 
+		if (reloc->sym->type == STT_SECTION) {
+			/* Addend field in the relocation entry associated with the symbol */
+			offset = reloc_addend(reloc);
+		} else {
+			/* The address of the symbol in the relocation entry */
+			offset = reloc->sym->offset;
+		}
+
 		/* Detect function pointers from contiguous objects: */
-		if (reloc->sym->sec == pfunc->sec &&
-		    reloc_addend(reloc) == pfunc->offset)
+		if (reloc->sym->sec == pfunc->sec && offset == pfunc->offset)
 			break;
 
-		dest_insn = find_insn(file, reloc->sym->sec, reloc_addend(reloc));
+		dest_insn = find_insn(file, reloc->sym->sec, offset);
 		if (!dest_insn)
 			break;
 
@@ -2137,6 +2145,7 @@ static struct reloc *find_jump_table(struct objtool_file *file,
 {
 	struct reloc *table_reloc;
 	struct instruction *dest_insn, *orig_insn = insn;
+	unsigned long offset;
 
 	/*
 	 * Backward search using the @first_jump_src links, these help avoid
@@ -2160,7 +2169,16 @@ static struct reloc *find_jump_table(struct objtool_file *file,
 		table_reloc = arch_find_switch_table(file, insn);
 		if (!table_reloc)
 			continue;
-		dest_insn = find_insn(file, table_reloc->sym->sec, reloc_addend(table_reloc));
+
+		if (table_reloc->sym->type == STT_SECTION) {
+			/* Addend field in the relocation entry associated with the symbol */
+			offset = reloc_addend(table_reloc);
+		} else {
+			/* The address of the symbol in the relocation entry */
+			offset = table_reloc->sym->offset;
+		}
+
+		dest_insn = find_insn(file, table_reloc->sym->sec, offset);
 		if (!dest_insn || !insn_func(dest_insn) || insn_func(dest_insn)->pfunc != func)
 			continue;
 
-- 
2.42.0

[PATCH v4 02/10] objtool: Handle special cases of dead end insn

[Tiezhu Yang]

There are some "unreachable instruction" objtool warnings when compling
with Clang on LoongArch, this is because the "break" instruction is set
as dead end due to its type is INSN_BUG in decode_instructions() at the
beginning, and it does not set insn->dead_end of the "break" instruction
as false after checking ".rela.discard.reachable" in add_dead_ends(), so
the next instruction of "break" is marked as unreachable.

Actually, it can find the reachable instruction after parsing the section
".rela.discard.reachable", in some cases, the "break" instruction may not
be the first previous instruction with scheduling by Machine Instruction
Scheduler of LLVM, it should find more times and then set insn->dead_end
of the "break" instruction as false.

This is preparation for later patch on LoongArch, there is no effect for
the other archs with this patch.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 tools/objtool/check.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 9601235e908d..191950551352 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -22,6 +22,10 @@
 #include <linux/static_call_types.h>
 #include <linux/string.h>
 
+#ifndef EM_LOONGARCH
+#define EM_LOONGARCH		258
+#endif
+
 struct alternative {
 	struct alternative *next;
 	struct instruction *insn;
@@ -711,6 +715,18 @@ static int add_dead_ends(struct objtool_file *file)
 		}
 
 		insn->dead_end = false;
+
+		/* Handle the special cases compiled with Clang on LoongArch */
+		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
+		    reloc->sym->type == STT_SECTION) {
+			while (insn && insn_func(insn)) {
+				insn = prev_insn_same_sym(file, insn);
+				if (insn && insn->dead_end) {
+					insn->dead_end = false;
+					break;
+				}
+			}
+		}
 	}
 
 	return 0;
-- 
2.42.0

[PATCH v4 03/10] objtool: Handle different entry size of rodata

[Tiezhu Yang]

In the most cases, the entry size of rodata is 8 bytes because the
relocation type is 64 bit, but when compling with Clang on LoongArch,
there exists 32 bit relocation type, the entry size of rodata should
be 4 bytes in this case.

This is preparation for later patch on LoongArch, there is no effect
for the other archs with this patch.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 tools/objtool/check.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 191950551352..19d1263e64e4 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -26,6 +26,10 @@
 #define EM_LOONGARCH		258
 #endif
 
+#ifndef R_LARCH_32_PCREL
+#define R_LARCH_32_PCREL	99
+#endif
+
 struct alternative {
 	struct alternative *next;
 	struct instruction *insn;
@@ -2096,6 +2100,7 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 	struct reloc *reloc = table;
 	struct alternative *alt;
 	unsigned long offset;
+	unsigned long rodata_entry_size;
 
 	/*
 	 * Each @reloc is a switch table relocation which points to the target
@@ -2107,8 +2112,15 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 		if (reloc != table && reloc == next_table)
 			break;
 
+		/* Handle the special cases compiled with Clang on LoongArch */
+		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
+		    reloc_type(reloc) == R_LARCH_32_PCREL)
+			rodata_entry_size = 4;
+		else
+			rodata_entry_size = 8;
+
 		/* Make sure the table entries are consecutive: */
-		if (prev_offset && reloc_offset(reloc) != prev_offset + 8)
+		if (prev_offset && reloc_offset(reloc) != prev_offset + rodata_entry_size)
 			break;
 
 		if (reloc->sym->type == STT_SECTION) {
-- 
2.42.0

[PATCH v4 04/10] objtool: Handle PC relative relocation type

[Tiezhu Yang]

When compling with Clang on LoongArch, there exists 32 bit PC relative
relocation type, it needs to get the offset with "S + A - PC" according
to the spec of "ELF for the LoongArch Architecture".

This is preparation for later patch on LoongArch, there is no effect for
the other archs with this patch.

Link: https://github.com/loongson/la-abi-specs/blob/release/laelf.adoc
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 tools/objtool/check.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 19d1263e64e4..8733ca620cca 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2126,6 +2126,11 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 		if (reloc->sym->type == STT_SECTION) {
 			/* Addend field in the relocation entry associated with the symbol */
 			offset = reloc_addend(reloc);
+			/* Handle the special cases compiled with Clang on LoongArch */
+			if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
+			    reloc_type(reloc) == R_LARCH_32_PCREL)
+				offset = reloc->sym->offset + reloc_addend(reloc) -
+					 (reloc_offset(reloc) - reloc_offset(table));
 		} else {
 			/* The address of the symbol in the relocation entry */
 			offset = reloc->sym->offset;
-- 
2.42.0

[PATCH v4 05/10] objtool: Handle unreachable entry of rodata

[Tiezhu Yang]

When compling with Clang on LoongArch, there exists unreachable entry of
rodata which points to a position after the function return instruction,
this is generated by compiler to fill the non-existent switch case, just
skip the entry when parsing the relocation section of rodata.

This is preparation for later patch on LoongArch, there is no effect for
the other archs with this patch.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 tools/objtool/check.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 8733ca620cca..b21e47d8d3d1 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2144,6 +2144,13 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 		if (!dest_insn)
 			break;
 
+		/* Handle the special cases compiled with Clang on LoongArch */
+		if (file->elf->ehdr.e_machine == EM_LOONGARCH && reloc->sym->type == STT_SECTION &&
+		    (!insn_func(dest_insn) || insn_func(dest_insn)->pfunc != pfunc)) {
+			prev_offset = reloc_offset(reloc);
+			continue;
+		}
+
 		/* Make sure the destination is in the same function: */
 		if (!insn_func(dest_insn) || insn_func(dest_insn)->pfunc != pfunc)
 			break;
-- 
2.42.0

[PATCH v4 06/10] objtool: Handle unsorted table offset of rodata

[Tiezhu Yang]

When compling with Clang on LoongArch, there are unsorted table offset
of rodata if there exist many jump tables, it will get the wrong table
end and find the wrong jump destination instructions in add_jump_table(),
so it needs to check the table size of rodata to break the process when
parsing the relocation section of rodata.

This is preparation for later patch on LoongArch, there is no effect for
the other archs with this patch.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 tools/objtool/check.c                 | 7 +++++++
 tools/objtool/include/objtool/check.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index b21e47d8d3d1..3b2d94c90011 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2101,6 +2101,7 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 	struct alternative *alt;
 	unsigned long offset;
 	unsigned long rodata_entry_size;
+	unsigned long rodata_table_size = insn->table_size;
 
 	/*
 	 * Each @reloc is a switch table relocation which points to the target
@@ -2112,6 +2113,12 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 		if (reloc != table && reloc == next_table)
 			break;
 
+		/* Handle the special cases compiled with Clang on LoongArch */
+		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
+		    reloc->sym->type == STT_SECTION && reloc != table &&
+		    reloc_offset(reloc) == reloc_offset(table) + rodata_table_size)
+			break;
+
 		/* Handle the special cases compiled with Clang on LoongArch */
 		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
 		    reloc_type(reloc) == R_LARCH_32_PCREL)
diff --git a/tools/objtool/include/objtool/check.h b/tools/objtool/include/objtool/check.h
index daa46f1f0965..11aafcc7b0c7 100644
--- a/tools/objtool/include/objtool/check.h
+++ b/tools/objtool/include/objtool/check.h
@@ -46,6 +46,7 @@ struct instruction {
 	struct section *sec;
 	unsigned long offset;
 	unsigned long immediate;
+	unsigned long table_size;
 
 	u8 len;
 	u8 prev_len;
-- 
2.42.0

[PATCH v4 07/10] objtool/LoongArch: Get each table size of rodata

[Tiezhu Yang]

When compling with Clang on LoongArch, there are unsorted table offset
of rodata if there exist many jump tables, it will get the wrong table
end and find the wrong jump destination instructions in add_jump_table().

Sort the rodata table offset by parsing ".rela.discard.tablejump_annotate"
and then get each table size of rodata corresponded with each table jump
instruction, it is used to check the table end and will break the process
when parsing ".rela.rodata" to avoid getting the wrong jump destination
instructions.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 tools/objtool/arch/loongarch/special.c | 72 ++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/tools/objtool/arch/loongarch/special.c b/tools/objtool/arch/loongarch/special.c
index 9bba1e9318e0..454bd01226a4 100644
--- a/tools/objtool/arch/loongarch/special.c
+++ b/tools/objtool/arch/loongarch/special.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 #include <objtool/special.h>
+#include <objtool/warn.h>
 
 bool arch_support_alt_relocation(struct special_alt *special_alt,
 				 struct instruction *insn,
@@ -8,6 +9,77 @@ bool arch_support_alt_relocation(struct special_alt *special_alt,
 	return false;
 }
 
+struct table_info {
+	struct list_head jump_info;
+	unsigned long insn_offset;
+	unsigned long rodata_offset;
+};
+
+static void get_rodata_table_size_by_table_annotate(struct objtool_file *file,
+						    struct instruction *insn)
+{
+	struct section *rsec;
+	struct reloc *reloc;
+	struct list_head table_list;
+	struct table_info *orig_table;
+	struct table_info *next_table;
+	unsigned long tmp_insn_offset;
+	unsigned long tmp_rodata_offset;
+
+	rsec = find_section_by_name(file->elf, ".rela.discard.tablejump_annotate");
+	if (!rsec)
+		return;
+
+	INIT_LIST_HEAD(&table_list);
+
+	for_each_reloc(rsec, reloc) {
+		if (reloc->sym->type != STT_SECTION)
+			return;
+
+		orig_table = malloc(sizeof(struct table_info));
+		if (!orig_table) {
+			WARN("malloc failed");
+			return;
+		}
+
+		orig_table->insn_offset = reloc_addend(reloc);
+		reloc++;
+		orig_table->rodata_offset = reloc_addend(reloc);
+
+		list_add_tail(&orig_table->jump_info, &table_list);
+
+		if (reloc_idx(reloc) + 1 == sec_num_entries(rsec))
+			break;
+	}
+
+	list_for_each_entry(orig_table, &table_list, jump_info) {
+		next_table = list_next_entry(orig_table, jump_info);
+		list_for_each_entry_from(next_table, &table_list, jump_info) {
+			if (next_table->rodata_offset < orig_table->rodata_offset) {
+				tmp_insn_offset = next_table->insn_offset;
+				tmp_rodata_offset = next_table->rodata_offset;
+				next_table->insn_offset = orig_table->insn_offset;
+				next_table->rodata_offset = orig_table->rodata_offset;
+				orig_table->insn_offset = tmp_insn_offset;
+				orig_table->rodata_offset = tmp_rodata_offset;
+			}
+		}
+	}
+
+	list_for_each_entry(orig_table, &table_list, jump_info) {
+		if (insn->offset == orig_table->insn_offset) {
+			next_table = list_next_entry(orig_table, jump_info);
+			if (&next_table->jump_info == &table_list)
+				break;
+
+			while (next_table->rodata_offset == orig_table->rodata_offset)
+				next_table = list_next_entry(next_table, jump_info);
+
+			insn->table_size = next_table->rodata_offset - orig_table->rodata_offset;
+		}
+	}
+}
+
 struct reloc *arch_find_switch_table(struct objtool_file *file,
 				     struct instruction *insn)
 {
-- 
2.42.0

[PATCH v4 08/10] objtool/LoongArch: Add support for switch table

[Tiezhu Yang]

The objtool program needs to analysis the control flow of each object file
generated by compiler toolchain, it needs to know all the locations that a
branch instruction may jump into, if a jump table is used, objtool has to
correlate the jump instruction with the table.

On x86 which is the only port supported by objtool before LoongArch, there
is a relocation on the jump instruction and to the table directly. But on
LoongArch, the relocation is on some kind of instruction prior to the jump
instruction, and also with scheduling it is not easy to tell the offset of
that instruction from the jump instruction. Furthermore, because LoongArch
has -fsection-anchors (often enabled at -O1 or above) the relocation may
actually points to a section anchor instead of the table itself.

The good news is that after continuous analysis and discussions, at last a
GCC patch "LoongArch: Add support to annotate tablejump" and a Clang patch
"[LoongArch] Add options for annotate tablejump" have been merged into the
upstream mainline, the compiler changes make life much easier for switch
table support of objtool on LoongArch.

By now, there is an additional section ".discard.tablejump_annotate" to
store the jump info as pairs of addresses, each pair contains the address
of jump instruction and the address of jump table.

In order to find switch table, it is easy to parse the relocation section
".rela.discard.tablejump_annotate" to get table_sec and table_offset, the
rest process is somehow like x86.

Link: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=0ee028f55640
Link: https://github.com/llvm/llvm-project/commit/4c2c17756739
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 tools/objtool/arch/loongarch/special.c | 60 +++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/arch/loongarch/special.c b/tools/objtool/arch/loongarch/special.c
index 454bd01226a4..366517deb35b 100644
--- a/tools/objtool/arch/loongarch/special.c
+++ b/tools/objtool/arch/loongarch/special.c
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
+#include <string.h>
 #include <objtool/special.h>
 #include <objtool/warn.h>
 
@@ -80,8 +81,65 @@ static void get_rodata_table_size_by_table_annotate(struct objtool_file *file,
 	}
 }
 
+static struct reloc *find_reloc_by_table_annotate(struct objtool_file *file,
+						  struct instruction *insn)
+{
+	struct section *rsec;
+	struct reloc *reloc;
+	unsigned long offset;
+
+	rsec = find_section_by_name(file->elf, ".rela.discard.tablejump_annotate");
+	if (!rsec)
+		return NULL;
+
+	for_each_reloc(rsec, reloc) {
+		if (reloc->sym->sec->rodata)
+			continue;
+
+		if (strcmp(insn->sec->name, reloc->sym->sec->name))
+			continue;
+
+		if (reloc->sym->type == STT_SECTION)
+			offset = reloc_addend(reloc);
+		else
+			offset = reloc->sym->offset;
+
+		if (insn->offset == offset) {
+			get_rodata_table_size_by_table_annotate(file, insn);
+			reloc++;
+			return reloc;
+		}
+	}
+
+	return NULL;
+}
+
 struct reloc *arch_find_switch_table(struct objtool_file *file,
 				     struct instruction *insn)
 {
-	return NULL;
+	struct reloc *annotate_reloc;
+	struct reloc *rodata_reloc;
+	struct section *table_sec;
+	unsigned long table_offset;
+
+	annotate_reloc = find_reloc_by_table_annotate(file, insn);
+	if (!annotate_reloc)
+		return NULL;
+
+	table_sec = annotate_reloc->sym->sec;
+	if (annotate_reloc->sym->type == STT_SECTION)
+		table_offset = reloc_addend(annotate_reloc);
+	else
+		table_offset = annotate_reloc->sym->offset;
+
+	/*
+	 * Each table entry has a rela associated with it.  The rela
+	 * should reference text in the same function as the original
+	 * instruction.
+	 */
+	rodata_reloc = find_reloc_by_dest(file->elf, table_sec, table_offset);
+	if (!rodata_reloc)
+		return NULL;
+
+	return rodata_reloc;
 }
-- 
2.42.0

[PATCH v4 09/10] objtool/LoongArch: Add support for goto table

[Tiezhu Yang]

The objtool program needs to analysis the control flow of each object file
generated by compiler toolchain, it needs to know all the locations that a
branch instruction may jump into, if a jump table is used, objtool has to
correlate the jump instruction with the table.

On x86 which is the only port supported by objtool before LoongArch, there
is a relocation on the jump instruction and to the table directly. But on
LoongArch, the relocation is on some kind of instruction prior to the jump
instruction, and also with scheduling it is not easy to tell the offset of
that instruction from the jump instruction. Furthermore, because LoongArch
has -fsection-anchors (often enabled at -O1 or above) the relocation may
actually points to a section anchor instead of the table itself.

For the jump table of switch cases, a GCC patch "LoongArch: Add support to
annotate tablejump" and a Clang patch "[LoongArch] Add options for annotate
tablejump" have been merged into the upstream mainline, it can parse the
additional section ".discard.tablejump_annotate" which stores the jump info
as pairs of addresses, each pair contains the address of jump instruction
and the address of jump table.

For the jump table of computed gotos, it is indeed not easy to implement
in the compiler, especially if there is more than one computed goto in a
function such as ___bpf_prog_run(). objdump kernel/bpf/core.o shows that
there are many table jump instructions in ___bpf_prog_run(), but there are
no relocations on the table jump instructions and to the table directly on
LoongArch.

Without the help of compiler, in order to figure out the address of goto
table for the special case of ___bpf_prog_run(), since the instruction
sequence is relatively single and stable, it makes sense to add a helper
find_reloc_of_rodata_c_jump_table() to find the relocation which points
to the section ".rodata..c_jump_table".

If find_reloc_by_table_annotate() failed, it means there is no relocation
info of switch table address in ".rela.discard.tablejump_annotate", then
objtool may find the relocation info of goto table ".rodata..c_jump_table"
with find_reloc_of_rodata_c_jump_table().

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 tools/objtool/arch/loongarch/special.c | 28 ++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/tools/objtool/arch/loongarch/special.c b/tools/objtool/arch/loongarch/special.c
index 366517deb35b..c5f4217df3a7 100644
--- a/tools/objtool/arch/loongarch/special.c
+++ b/tools/objtool/arch/loongarch/special.c
@@ -114,6 +114,27 @@ static struct reloc *find_reloc_by_table_annotate(struct objtool_file *file,
 	return NULL;
 }
 
+static struct reloc *find_reloc_of_rodata_c_jump_table(struct section *sec,
+						       unsigned long offset)
+{
+	struct section *rsec;
+	struct reloc *reloc;
+
+	rsec = sec->rsec;
+	if (!rsec)
+		return NULL;
+
+	for_each_reloc(rsec, reloc) {
+		if (reloc_offset(reloc) > offset)
+			break;
+
+		if (!strncmp(reloc->sym->sec->name, ".rodata..c_jump_table", 21))
+			return reloc;
+	}
+
+	return NULL;
+}
+
 struct reloc *arch_find_switch_table(struct objtool_file *file,
 				     struct instruction *insn)
 {
@@ -123,8 +144,11 @@ struct reloc *arch_find_switch_table(struct objtool_file *file,
 	unsigned long table_offset;
 
 	annotate_reloc = find_reloc_by_table_annotate(file, insn);
-	if (!annotate_reloc)
-		return NULL;
+	if (!annotate_reloc) {
+		annotate_reloc = find_reloc_of_rodata_c_jump_table(insn->sec, insn->offset);
+		if (!annotate_reloc)
+			return NULL;
+	}
 
 	table_sec = annotate_reloc->sym->sec;
 	if (annotate_reloc->sym->type == STT_SECTION)
-- 
2.42.0

[PATCH v4 10/10] LoongArch: Enable jump table for objtool

[Tiezhu Yang]

For now, it is time to remove -fno-jump-tables to enable jump table for
objtool if the compiler has -mannotate-tablejump, otherwise it is better
to remain -fno-jump-tables to keep compatibility with the older compilers.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/loongarch/Kconfig  | 3 +++
 arch/loongarch/Makefile | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
index bb35c34f86d2..500ee9b2cd88 100644
--- a/arch/loongarch/Kconfig
+++ b/arch/loongarch/Kconfig
@@ -284,6 +284,9 @@ config AS_HAS_LBT_EXTENSION
 config AS_HAS_LVZ_EXTENSION
 	def_bool $(as-instr,hvcl 0)
 
+config CC_HAS_ANNOTATE_TABLEJUMP
+	def_bool $(cc-option,-mannotate-tablejump)
+
 menu "Kernel type and options"
 
 source "kernel/Kconfig.hz"
diff --git a/arch/loongarch/Makefile b/arch/loongarch/Makefile
index ae3f80622f4c..6b84e633b8ce 100644
--- a/arch/loongarch/Makefile
+++ b/arch/loongarch/Makefile
@@ -101,8 +101,12 @@ KBUILD_AFLAGS			+= $(call cc-option,-mthin-add-sub) $(call cc-option,-Wa$(comma)
 KBUILD_CFLAGS			+= $(call cc-option,-mthin-add-sub) $(call cc-option,-Wa$(comma)-mthin-add-sub)
 
 ifdef CONFIG_OBJTOOL
+ifdef CONFIG_CC_HAS_ANNOTATE_TABLEJUMP
+KBUILD_CFLAGS			+= $(call cc-option,-mannotate-tablejump)
+else
 KBUILD_CFLAGS			+= -fno-jump-tables
 endif
+endif
 
 KBUILD_RUSTFLAGS		+= --target=loongarch64-unknown-none-softfloat
 KBUILD_RUSTFLAGS_KERNEL		+= -Zdirect-access-external-data=yes
-- 
2.42.0

Re: [PATCH v4 00/10] Add jump table support for objtool on LoongArch

[Huacai Chen]

Series applied, thanks.

Huacai

On Fri, Nov 22, 2024 at 12:50 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
> This series is based on 6.12-rc7, tested with the latest upstream
> mainline Binutils, GCC and Clang, most of the patches are aim to
> handle the special cases compiled with Clang on LoongArch.
>
> v4:
>   -- Avoid EM_LOONGARCH and R_LARCH_32_PCREL undeclared error
>      for various compiling environments.
>
>   -- Remove the check condition "dest_insn->type == INSN_NOP"
>      for unreachable entry of rodata.
>
> Tiezhu Yang (10):
>   objtool: Handle various symbol types of rodata
>   objtool: Handle special cases of dead end insn
>   objtool: Handle different entry size of rodata
>   objtool: Handle PC relative relocation type
>   objtool: Handle unreachable entry of rodata
>   objtool: Handle unsorted table offset of rodata
>   objtool/LoongArch: Get each table size of rodata
>   objtool/LoongArch: Add support for switch table
>   objtool/LoongArch: Add support for goto table
>   LoongArch: Enable jump table for objtool
>
>  arch/loongarch/Kconfig                 |   3 +
>  arch/loongarch/Makefile                |   4 +
>  tools/objtool/arch/loongarch/special.c | 156 ++++++++++++++++++++++++-
>  tools/objtool/check.c                  |  75 +++++++++++-
>  tools/objtool/include/objtool/check.h  |   1 +
>  5 files changed, 233 insertions(+), 6 deletions(-)
>
> --
> 2.42.0
>

Re: [PATCH v4 01/10] objtool: Handle various symbol types of rodata

[Josh Poimboeuf]

On Fri, Nov 22, 2024 at 12:49:56PM +0800, Tiezhu Yang wrote:
> @@ -2094,12 +2095,19 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>  		if (prev_offset && reloc_offset(reloc) != prev_offset + 8)
>  			break;
>  
> +		if (reloc->sym->type == STT_SECTION) {
> +			/* Addend field in the relocation entry associated with the symbol */
> +			offset = reloc_addend(reloc);
> +		} else {
> +			/* The address of the symbol in the relocation entry */
> +			offset = reloc->sym->offset;

The comments don't seem helpful.

In the case of STT_SECTION, sym->offset is always zero.  Therefore the
if-else can be converted to a simple unconditional statement:

	offset = reloc->sym->offset + reloc_addend(reloc);

'prev_offset' needs to be updated as well.

> @@ -2137,6 +2145,7 @@ static struct reloc *find_jump_table(struct objtool_file *file,
>  {
>  	struct reloc *table_reloc;
>  	struct instruction *dest_insn, *orig_insn = insn;
> +	unsigned long offset;
>  
>  	/*
>  	 * Backward search using the @first_jump_src links, these help avoid
> @@ -2160,7 +2169,16 @@ static struct reloc *find_jump_table(struct objtool_file *file,
>  		table_reloc = arch_find_switch_table(file, insn);
>  		if (!table_reloc)
>  			continue;
> -		dest_insn = find_insn(file, table_reloc->sym->sec, reloc_addend(table_reloc));
> +
> +		if (table_reloc->sym->type == STT_SECTION) {
> +			/* Addend field in the relocation entry associated with the symbol */
> +			offset = reloc_addend(table_reloc);
> +		} else {
> +			/* The address of the symbol in the relocation entry */
> +			offset = table_reloc->sym->offset;
> +		}

Same comment here.

-- 
Josh

Re: [PATCH v4 02/10] objtool: Handle special cases of dead end insn

[Josh Poimboeuf]

On Fri, Nov 22, 2024 at 12:49:57PM +0800, Tiezhu Yang wrote:
> There are some "unreachable instruction" objtool warnings when compling
> with Clang on LoongArch, this is because the "break" instruction is set
> as dead end due to its type is INSN_BUG in decode_instructions() at the
> beginning, and it does not set insn->dead_end of the "break" instruction
> as false after checking ".rela.discard.reachable" in add_dead_ends(), so
> the next instruction of "break" is marked as unreachable.
> 
> Actually, it can find the reachable instruction after parsing the section
> ".rela.discard.reachable", in some cases, the "break" instruction may not
> be the first previous instruction with scheduling by Machine Instruction
> Scheduler of LLVM, it should find more times and then set insn->dead_end
> of the "break" instruction as false.
> 
> This is preparation for later patch on LoongArch, there is no effect for
> the other archs with this patch.
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>

I'm having trouble understanding this commit log, is the problem that
the compiler is sometimes inserting code between 'break' and the
unreachable() inline asm?

If so, this sounds like a problem that was already solved for x86 with:

  bfb1a7c91fb7 ("x86/bug: Merge annotate_reachable() into _BUG_FLAGS() asm")

Can you check if that fixes it?

-- 
Josh

Re: [PATCH v4 03/10] objtool: Handle different entry size of rodata

[Josh Poimboeuf]

On Fri, Nov 22, 2024 at 12:49:58PM +0800, Tiezhu Yang wrote:
> @@ -2107,8 +2112,15 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>  		if (reloc != table && reloc == next_table)
>  			break;
>  
> +		/* Handle the special cases compiled with Clang on LoongArch */

This comment is not helpful at all.  A comment is only needed if the
code is not already obvious.  In that case it should describe what is
being done and why.

> +		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
> +		    reloc_type(reloc) == R_LARCH_32_PCREL)
> +			rodata_entry_size = 4;
> +		else
> +			rodata_entry_size = 8;

Is this really loongarch-specific or is it only related to the size of
the reloc?  Can this be abstracted out to a reloc_size() function like
so?

  https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/tree/tools/objtool/klp-diff.c?h=objtool-diff#n834

maybe it could live in elf.h.

-- 
Josh

Re: [PATCH v4 04/10] objtool: Handle PC relative relocation type

[Josh Poimboeuf]

On Fri, Nov 22, 2024 at 12:49:59PM +0800, Tiezhu Yang wrote:
> When compling with Clang on LoongArch, there exists 32 bit PC relative
> relocation type, it needs to get the offset with "S + A - PC" according
> to the spec of "ELF for the LoongArch Architecture".
> 
> This is preparation for later patch on LoongArch, there is no effect for
> the other archs with this patch.
> 
> Link: https://github.com/loongson/la-abi-specs/blob/release/laelf.adoc
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  tools/objtool/check.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 19d1263e64e4..8733ca620cca 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2126,6 +2126,11 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>  		if (reloc->sym->type == STT_SECTION) {
>  			/* Addend field in the relocation entry associated with the symbol */
>  			offset = reloc_addend(reloc);
> +			/* Handle the special cases compiled with Clang on LoongArch */
> +			if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
> +			    reloc_type(reloc) == R_LARCH_32_PCREL)
> +				offset = reloc->sym->offset + reloc_addend(reloc) -
> +					 (reloc_offset(reloc) - reloc_offset(table));

Please, no more "special cases".  It makes the common code unreadable.
We should avoid checking 'elf->ehdr.e_machine' in check.c.

Maybe there should be an arch-specific function arch_adjusted_addend().

-- 
Josh

Re: [PATCH v4 05/10] objtool: Handle unreachable entry of rodata

[Josh Poimboeuf]

On Fri, Nov 22, 2024 at 12:50:00PM +0800, Tiezhu Yang wrote:
> When compling with Clang on LoongArch, there exists unreachable entry of
> rodata which points to a position after the function return instruction,
> this is generated by compiler to fill the non-existent switch case, just
> skip the entry when parsing the relocation section of rodata.
> 
> This is preparation for later patch on LoongArch, there is no effect for
> the other archs with this patch.
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  tools/objtool/check.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 8733ca620cca..b21e47d8d3d1 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2144,6 +2144,13 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>  		if (!dest_insn)
>  			break;
>  
> +		/* Handle the special cases compiled with Clang on LoongArch */
> +		if (file->elf->ehdr.e_machine == EM_LOONGARCH && reloc->sym->type == STT_SECTION &&
> +		    (!insn_func(dest_insn) || insn_func(dest_insn)->pfunc != pfunc)) {
> +			prev_offset = reloc_offset(reloc);
> +			continue;

Are you sure this is specific to loongarch?

-- 
Josh

Re: [PATCH v4 00/10] Add jump table support for objtool on LoongArch

[Josh Poimboeuf]

On Sun, Nov 24, 2024 at 01:33:43PM +0800, Huacai Chen wrote:
> Series applied, thanks.
> 
> Huacai

Please don't merge any objtool code without maintainer acks.

-- 
Josh

Re: [PATCH v4 00/10] Add jump table support for objtool on LoongArch

[Huacai Chen]

Hi, Josh,

On Tue, Nov 26, 2024 at 3:29 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Sun, Nov 24, 2024 at 01:33:43PM +0800, Huacai Chen wrote:
> > Series applied, thanks.
> >
> > Huacai
>
> Please don't merge any objtool code without maintainer acks.
I'm sorry for that, I'll drop it first, and then wait for Tiezhu's new version.


Huacai
>
> --
> Josh

Re: [PATCH v4 01/10] objtool: Handle various symbol types of rodata

[Tiezhu Yang]

On 11/26/2024 02:44 PM, Josh Poimboeuf wrote:
> On Fri, Nov 22, 2024 at 12:49:56PM +0800, Tiezhu Yang wrote:
>> @@ -2094,12 +2095,19 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>>  		if (prev_offset && reloc_offset(reloc) != prev_offset + 8)
>>  			break;
>>
>> +		if (reloc->sym->type == STT_SECTION) {
>> +			/* Addend field in the relocation entry associated with the symbol */
>> +			offset = reloc_addend(reloc);
>> +		} else {
>> +			/* The address of the symbol in the relocation entry */
>> +			offset = reloc->sym->offset;
>
> The comments don't seem helpful.

Will remove it.

>
> In the case of STT_SECTION, sym->offset is always zero.  Therefore the
> if-else can be converted to a simple unconditional statement:
>
> 	offset = reloc->sym->offset + reloc_addend(reloc);

OK, let me test it.

>
> 'prev_offset' needs to be updated as well.

I am not sure I understand your comment correctly, I can not see
what should to do about 'prev_offset'.

>
>> @@ -2137,6 +2145,7 @@ static struct reloc *find_jump_table(struct objtool_file *file,
>>  {
>>  	struct reloc *table_reloc;
>>  	struct instruction *dest_insn, *orig_insn = insn;
>> +	unsigned long offset;
>>
>>  	/*
>>  	 * Backward search using the @first_jump_src links, these help avoid
>> @@ -2160,7 +2169,16 @@ static struct reloc *find_jump_table(struct objtool_file *file,
>>  		table_reloc = arch_find_switch_table(file, insn);
>>  		if (!table_reloc)
>>  			continue;
>> -		dest_insn = find_insn(file, table_reloc->sym->sec, reloc_addend(table_reloc));
>> +
>> +		if (table_reloc->sym->type == STT_SECTION) {
>> +			/* Addend field in the relocation entry associated with the symbol */
>> +			offset = reloc_addend(table_reloc);
>> +		} else {
>> +			/* The address of the symbol in the relocation entry */
>> +			offset = table_reloc->sym->offset;
>> +		}
>
> Same comment here.

OK, will do it.

Thanks,
Tiezhu

Re: [PATCH v4 02/10] objtool: Handle special cases of dead end insn

[Tiezhu Yang]

On 11/26/2024 02:45 PM, Josh Poimboeuf wrote:
> On Fri, Nov 22, 2024 at 12:49:57PM +0800, Tiezhu Yang wrote:
>> There are some "unreachable instruction" objtool warnings when compling
>> with Clang on LoongArch, this is because the "break" instruction is set
>> as dead end due to its type is INSN_BUG in decode_instructions() at the
>> beginning, and it does not set insn->dead_end of the "break" instruction
>> as false after checking ".rela.discard.reachable" in add_dead_ends(), so
>> the next instruction of "break" is marked as unreachable.
>>
>> Actually, it can find the reachable instruction after parsing the section
>> ".rela.discard.reachable", in some cases, the "break" instruction may not
>> be the first previous instruction with scheduling by Machine Instruction
>> Scheduler of LLVM, it should find more times and then set insn->dead_end
>> of the "break" instruction as false.
>>
>> This is preparation for later patch on LoongArch, there is no effect for
>> the other archs with this patch.
>>
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>
> I'm having trouble understanding this commit log, is the problem that
> the compiler is sometimes inserting code between 'break' and the
> unreachable() inline asm?
>
> If so, this sounds like a problem that was already solved for x86 with:
>
>   bfb1a7c91fb7 ("x86/bug: Merge annotate_reachable() into _BUG_FLAGS() asm")
>
> Can you check if that fixes it?

I will try, thank you.

Re: [PATCH v4 03/10] objtool: Handle different entry size of rodata

[Tiezhu Yang]

On 11/26/2024 03:02 PM, Josh Poimboeuf wrote:
> On Fri, Nov 22, 2024 at 12:49:58PM +0800, Tiezhu Yang wrote:
>> @@ -2107,8 +2112,15 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>>  		if (reloc != table && reloc == next_table)
>>  			break;
>>
>> +		/* Handle the special cases compiled with Clang on LoongArch */
>
> This comment is not helpful at all.  A comment is only needed if the
> code is not already obvious.  In that case it should describe what is
> being done and why.

Will remove it.

>> +		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
>> +		    reloc_type(reloc) == R_LARCH_32_PCREL)
>> +			rodata_entry_size = 4;
>> +		else
>> +			rodata_entry_size = 8;
>
> Is this really loongarch-specific or is it only related to the size of
> the reloc?

This is related with the reloc type, but it may be wrong to only
check the reloc type to assign the value of rodata_entry_size,
becasue the value of reloc type for different archs may be same,
so it needs to check ehdr.e_machine and relocation type.

> Can this be abstracted out to a reloc_size() function like
> so?
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/tree/tools/objtool/klp-diff.c?h=objtool-diff#n834
>
> maybe it could live in elf.h.

OK, if I understand your comment correctly, this should be an
arch-specific function defined in 
tools/objtool/arch/*/include/arch/elf.h, otherwise it also needs to 
check ehdr.e_machine
in tools/objtool/include/objtool/elf.h.

Thanks,
Tiezhu

Re: [PATCH v4 04/10] objtool: Handle PC relative relocation type

[Tiezhu Yang]

On 11/26/2024 03:19 PM, Josh Poimboeuf wrote:
> On Fri, Nov 22, 2024 at 12:49:59PM +0800, Tiezhu Yang wrote:
>> When compling with Clang on LoongArch, there exists 32 bit PC relative
>> relocation type, it needs to get the offset with "S + A - PC" according
>> to the spec of "ELF for the LoongArch Architecture".
>>
>> This is preparation for later patch on LoongArch, there is no effect for
>> the other archs with this patch.
>>
>> Link: https://github.com/loongson/la-abi-specs/blob/release/laelf.adoc
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>> ---
>>  tools/objtool/check.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index 19d1263e64e4..8733ca620cca 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -2126,6 +2126,11 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>>  		if (reloc->sym->type == STT_SECTION) {
>>  			/* Addend field in the relocation entry associated with the symbol */
>>  			offset = reloc_addend(reloc);
>> +			/* Handle the special cases compiled with Clang on LoongArch */
>> +			if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
>> +			    reloc_type(reloc) == R_LARCH_32_PCREL)
>> +				offset = reloc->sym->offset + reloc_addend(reloc) -
>> +					 (reloc_offset(reloc) - reloc_offset(table));
>
> Please, no more "special cases".  It makes the common code unreadable.
> We should avoid checking 'elf->ehdr.e_machine' in check.c.

OK.

>
> Maybe there should be an arch-specific function arch_adjusted_addend().
>

Will do it.

Re: [PATCH v4 05/10] objtool: Handle unreachable entry of rodata

[Tiezhu Yang]

On 11/26/2024 03:25 PM, Josh Poimboeuf wrote:
> On Fri, Nov 22, 2024 at 12:50:00PM +0800, Tiezhu Yang wrote:
>> When compling with Clang on LoongArch, there exists unreachable entry of
>> rodata which points to a position after the function return instruction,
>> this is generated by compiler to fill the non-existent switch case, just
>> skip the entry when parsing the relocation section of rodata.
>>
>> This is preparation for later patch on LoongArch, there is no effect for
>> the other archs with this patch.
>>
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>> ---
>>  tools/objtool/check.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index 8733ca620cca..b21e47d8d3d1 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -2144,6 +2144,13 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>>  		if (!dest_insn)
>>  			break;
>>
>> +		/* Handle the special cases compiled with Clang on LoongArch */
>> +		if (file->elf->ehdr.e_machine == EM_LOONGARCH && reloc->sym->type == STT_SECTION &&
>> +		    (!insn_func(dest_insn) || insn_func(dest_insn)->pfunc != pfunc)) {
>> +			prev_offset = reloc_offset(reloc);
>> +			continue;
>
> Are you sure this is specific to loongarch?

I am not sure, I only found this issue on LoongArch compiled with Clang,
but I think there is no effect if make it generic, like this:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index f7586f82b967..87302e6fc07f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2103,9 +2103,10 @@ static int add_jump_table(struct objtool_file 
*file, struct instruction *insn,
                 if (!dest_insn)
                         break;

-               /* Make sure the destination is in the same function: */
-               if (!insn_func(dest_insn) || insn_func(dest_insn)->pfunc 
!= pfunc)
-                       break;
+               if (!insn_func(dest_insn) || insn_func(dest_insn)->pfunc 
!= pfunc) {
+                       prev_offset = reloc_offset(reloc);
+                       continue;
+               }

                 alt = malloc(sizeof(*alt));
                 if (!alt) {

If you are OK, I will modify it.

Thanks,
Tiezhu

Re: [PATCH v4 03/10] objtool: Handle different entry size of rodata

[Tiezhu Yang]

On 11/26/2024 06:59 PM, Tiezhu Yang wrote:
> On 11/26/2024 03:02 PM, Josh Poimboeuf wrote:
>> On Fri, Nov 22, 2024 at 12:49:58PM +0800, Tiezhu Yang wrote:
>>> @@ -2107,8 +2112,15 @@ static int add_jump_table(struct objtool_file
>>> *file, struct instruction *insn,
>>>          if (reloc != table && reloc == next_table)
>>>              break;
>>>
>>> +        /* Handle the special cases compiled with Clang on LoongArch */
>>
>> This comment is not helpful at all.  A comment is only needed if the
>> code is not already obvious.  In that case it should describe what is
>> being done and why.
>
> Will remove it.
>
>>> +        if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
>>> +            reloc_type(reloc) == R_LARCH_32_PCREL)
>>> +            rodata_entry_size = 4;
>>> +        else
>>> +            rodata_entry_size = 8;
>>
>> Is this really loongarch-specific or is it only related to the size of
>> the reloc?
>
> This is related with the reloc type, but it may be wrong to only
> check the reloc type to assign the value of rodata_entry_size,
> becasue the value of reloc type for different archs may be same,
> so it needs to check ehdr.e_machine and relocation type.
>
>> Can this be abstracted out to a reloc_size() function like
>> so?
>>
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/tree/tools/objtool/klp-diff.c?h=objtool-diff#n834
>>
>>
>> maybe it could live in elf.h.
>
> OK, if I understand your comment correctly, this should be an
> arch-specific function defined in
> tools/objtool/arch/*/include/arch/elf.h, otherwise it also needs to
> check ehdr.e_machine
> in tools/objtool/include/objtool/elf.h.

There are only macro definitions in
tools/objtool/arch/*/include/arch/elf.h,
so I think it is better to add reloc_size() in
tools/objtool/include/objtool/elf.h,
like this:

static inline unsigned int reloc_size(struct elf *elf, struct reloc *reloc)
{
         if (elf->ehdr.e_machine == EM_X86_64) {
                 switch (reloc_type(reloc)) {
                 case R_X86_64_32:
                 case R_X86_64_PC32:
                         return 4;
                 case R_X86_64_64:
                 case R_X86_64_PC64:
                         return 8;
                 default:
                         ERROR("unknown X86_64 reloc type");
                 }
         } else if (elf->ehdr.e_machine == EM_LOONGARCH) {
                 switch (reloc_type(reloc)) {
                 case R_LARCH_32:
                 case R_LARCH_32_PCREL:
                         return 4;
                 case R_LARCH_64:
                 case R_LARCH_64_PCREL:
                         return 8;
                 default:
                         ERROR("unknown LoongArch reloc type");
                 }
         } else {
                 return 8;
         }
}

and call it in check.c, like this:

rodata_entry_size = reloc_size(file->elf, reloc);

What do you think?

Thanks,
Tiezhu

Re: [PATCH v4 04/10] objtool: Handle PC relative relocation type

[Tiezhu Yang]

On 11/26/2024 07:00 PM, Tiezhu Yang wrote:
> On 11/26/2024 03:19 PM, Josh Poimboeuf wrote:
>> On Fri, Nov 22, 2024 at 12:49:59PM +0800, Tiezhu Yang wrote:
>>> When compling with Clang on LoongArch, there exists 32 bit PC relative
>>> relocation type, it needs to get the offset with "S + A - PC" according
>>> to the spec of "ELF for the LoongArch Architecture".
>>>
>>> This is preparation for later patch on LoongArch, there is no effect for
>>> the other archs with this patch.
>>>
>>> Link: https://github.com/loongson/la-abi-specs/blob/release/laelf.adoc
>>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>>> ---
>>>  tools/objtool/check.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>>> index 19d1263e64e4..8733ca620cca 100644
>>> --- a/tools/objtool/check.c
>>> +++ b/tools/objtool/check.c
>>> @@ -2126,6 +2126,11 @@ static int add_jump_table(struct objtool_file
>>> *file, struct instruction *insn,
>>>          if (reloc->sym->type == STT_SECTION) {
>>>              /* Addend field in the relocation entry associated with
>>> the symbol */
>>>              offset = reloc_addend(reloc);
>>> +            /* Handle the special cases compiled with Clang on
>>> LoongArch */
>>> +            if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
>>> +                reloc_type(reloc) == R_LARCH_32_PCREL)
>>> +                offset = reloc->sym->offset + reloc_addend(reloc) -
>>> +                     (reloc_offset(reloc) - reloc_offset(table));
>>
>> Please, no more "special cases".  It makes the common code unreadable.
>> We should avoid checking 'elf->ehdr.e_machine' in check.c.
>
> OK.
>
>>
>> Maybe there should be an arch-specific function arch_adjusted_addend().

Add adjust_offset() in
tools/objtool/include/objtool/elf.h,
like this:

static inline unsigned long adjust_offset(struct elf *elf, struct reloc 
*reloc,
                                           unsigned long offset)
{
         if (elf->ehdr.e_machine == EM_LOONGARCH && reloc_type(reloc) == 
R_LARCH_32_PCREL)
                 offset = reloc->sym->offset + reloc_addend(reloc) -
                          (reloc_offset(reloc) - reloc_offset(table));

         return offset;
}

then call it in check.c, like this:

offset = reloc->sym->offset + reloc_addend(reloc);
offset = adjust_offset(file->elf, reloc, offset);

What do you think?

Thanks,
Tiezhu

Re: [PATCH v4 06/10] objtool: Handle unsorted table offset of rodata

[Tiezhu Yang]

On 11/22/2024 12:50 PM, Tiezhu Yang wrote:
> When compling with Clang on LoongArch, there are unsorted table offset
> of rodata if there exist many jump tables, it will get the wrong table
> end and find the wrong jump destination instructions in add_jump_table(),
> so it needs to check the table size of rodata to break the process when
> parsing the relocation section of rodata.
>
> This is preparation for later patch on LoongArch, there is no effect for
> the other archs with this patch.
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  tools/objtool/check.c                 | 7 +++++++
>  tools/objtool/include/objtool/check.h | 1 +
>  2 files changed, 8 insertions(+)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index b21e47d8d3d1..3b2d94c90011 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2101,6 +2101,7 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>  	struct alternative *alt;
>  	unsigned long offset;
>  	unsigned long rodata_entry_size;
> +	unsigned long rodata_table_size = insn->table_size;
>
>  	/*
>  	 * Each @reloc is a switch table relocation which points to the target
> @@ -2112,6 +2113,12 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>  		if (reloc != table && reloc == next_table)
>  			break;
>
> +		/* Handle the special cases compiled with Clang on LoongArch */
> +		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
> +		    reloc->sym->type == STT_SECTION && reloc != table &&
> +		    reloc_offset(reloc) == reloc_offset(table) + rodata_table_size)
> +			break;

I think it can be generic, like this:

                 /* Check for the end of the table: */
                 if (reloc != table && reloc == next_table)
                         break;

                 if (reloc != table &&
                     reloc_offset(reloc) == reloc_offset(table) + 
rodata_table_size)
                         break;

What do you think?

Thanks,
Tiezhu

Re: [PATCH v4 02/10] objtool: Handle special cases of dead end insn

[Peter Zijlstra]

On Tue, Nov 26, 2024 at 06:42:15PM +0800, Tiezhu Yang wrote:
> On 11/26/2024 02:45 PM, Josh Poimboeuf wrote:
> > On Fri, Nov 22, 2024 at 12:49:57PM +0800, Tiezhu Yang wrote:
> > > There are some "unreachable instruction" objtool warnings when compling
> > > with Clang on LoongArch, this is because the "break" instruction is set
> > > as dead end due to its type is INSN_BUG in decode_instructions() at the
> > > beginning, and it does not set insn->dead_end of the "break" instruction
> > > as false after checking ".rela.discard.reachable" in add_dead_ends(), so
> > > the next instruction of "break" is marked as unreachable.
> > > 
> > > Actually, it can find the reachable instruction after parsing the section
> > > ".rela.discard.reachable", in some cases, the "break" instruction may not
> > > be the first previous instruction with scheduling by Machine Instruction
> > > Scheduler of LLVM, it should find more times and then set insn->dead_end
> > > of the "break" instruction as false.
> > > 
> > > This is preparation for later patch on LoongArch, there is no effect for
> > > the other archs with this patch.
> > > 
> > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> > 
> > I'm having trouble understanding this commit log, is the problem that
> > the compiler is sometimes inserting code between 'break' and the
> > unreachable() inline asm?
> > 
> > If so, this sounds like a problem that was already solved for x86 with:
> > 
> >   bfb1a7c91fb7 ("x86/bug: Merge annotate_reachable() into _BUG_FLAGS() asm")
> > 
> > Can you check if that fixes it?
> 
> I will try, thank you.
> 

I was poking at the reachable annotations and ended up with this:

--- a/arch/loongarch/include/asm/bug.h
+++ b/arch/loongarch/include/asm/bug.h
@@ -4,6 +4,7 @@
 
 #include <asm/break.h>
 #include <linux/stringify.h>
+#include <linux/objtool.h>
 
 #ifndef CONFIG_DEBUG_BUGVERBOSE
 #define _BUGVERBOSE_LOCATION(file, line)
@@ -37,21 +38,21 @@
 
 #define ASM_BUG()	ASM_BUG_FLAGS(0)
 
-#define __BUG_FLAGS(flags)					\
-	asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags)));
+#define __BUG_FLAGS(flags, extra)					\
+	asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags))		\
+			     extra);
 
 #define __WARN_FLAGS(flags)					\
 do {								\
 	instrumentation_begin();				\
-	__BUG_FLAGS(BUGFLAG_WARNING|(flags));			\
-	annotate_reachable();					\
+	__BUG_FLAGS(BUGFLAG_WARNING|(flags), ASM_REACHABLE);	\
 	instrumentation_end();					\
 } while (0)
 
 #define BUG()							\
 do {								\
 	instrumentation_begin();				\
-	__BUG_FLAGS(0);						\
+	__BUG_FLAGS(0, "");					\
 	unreachable();						\
 } while (0)
 

Re: [PATCH v4 02/10] objtool: Handle special cases of dead end insn

[Peter Zijlstra]

> I was poking at the reachable annotations and ended up with this:

Also see here:

  https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/log/?h=objtool/core

Once the robots agree it all compiles, I'll post.

> --- a/arch/loongarch/include/asm/bug.h
> +++ b/arch/loongarch/include/asm/bug.h
> @@ -4,6 +4,7 @@
>  
>  #include <asm/break.h>
>  #include <linux/stringify.h>
> +#include <linux/objtool.h>
>  
>  #ifndef CONFIG_DEBUG_BUGVERBOSE
>  #define _BUGVERBOSE_LOCATION(file, line)
> @@ -37,21 +38,21 @@
>  
>  #define ASM_BUG()	ASM_BUG_FLAGS(0)
>  
> -#define __BUG_FLAGS(flags)					\
> -	asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags)));
> +#define __BUG_FLAGS(flags, extra)					\
> +	asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags))		\
> +			     extra);
>  
>  #define __WARN_FLAGS(flags)					\
>  do {								\
>  	instrumentation_begin();				\
> -	__BUG_FLAGS(BUGFLAG_WARNING|(flags));			\
> -	annotate_reachable();					\
> +	__BUG_FLAGS(BUGFLAG_WARNING|(flags), ASM_REACHABLE);	\
>  	instrumentation_end();					\
>  } while (0)
>  
>  #define BUG()							\
>  do {								\
>  	instrumentation_begin();				\
> -	__BUG_FLAGS(0);						\
> +	__BUG_FLAGS(0, "");					\
>  	unreachable();						\
>  } while (0)
>  

Re: [PATCH v4 01/10] objtool: Handle various symbol types of rodata

[Josh Poimboeuf]

On Tue, Nov 26, 2024 at 06:41:29PM +0800, Tiezhu Yang wrote:
> On 11/26/2024 02:44 PM, Josh Poimboeuf wrote:
> > On Fri, Nov 22, 2024 at 12:49:56PM +0800, Tiezhu Yang wrote:
> > > @@ -2094,12 +2095,19 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
> >
> > 'prev_offset' needs to be updated as well.
> 
> I am not sure I understand your comment correctly, I can not see
> what should to do about 'prev_offset'.

Further down the function there is

  prev_offset = reloc_offset(reloc);

which needs to be changed to

  prev_offset = offset;

as part of the patch.

-- 
Josh

Re: [PATCH v4 03/10] objtool: Handle different entry size of rodata

[Josh Poimboeuf]

On Tue, Nov 26, 2024 at 06:59:38PM +0800, Tiezhu Yang wrote:
> > Can this be abstracted out to a reloc_size() function like
> > so?
> > 
> >   https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/tree/tools/objtool/klp-diff.c?h=objtool-diff#n834
> > 
> > maybe it could live in elf.h.
> 
> OK, if I understand your comment correctly, this should be an
> arch-specific function defined in tools/objtool/arch/*/include/arch/elf.h,
> otherwise it also needs to check ehdr.e_machine
> in tools/objtool/include/objtool/elf.h.

It should probably be an arch-specific function in tools/objtool/arch
somewhere.

-- 
Josh

Re: [PATCH v4 05/10] objtool: Handle unreachable entry of rodata

[Josh Poimboeuf]

On Tue, Nov 26, 2024 at 07:04:39PM +0800, Tiezhu Yang wrote:
> On 11/26/2024 03:25 PM, Josh Poimboeuf wrote:
> > Are you sure this is specific to loongarch?
> 
> I am not sure, I only found this issue on LoongArch compiled with Clang,
> but I think there is no effect if make it generic, like this:
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index f7586f82b967..87302e6fc07f 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2103,9 +2103,10 @@ static int add_jump_table(struct objtool_file *file,
> struct instruction *insn,
>                 if (!dest_insn)
>                         break;
> 
> -               /* Make sure the destination is in the same function: */
> -               if (!insn_func(dest_insn) || insn_func(dest_insn)->pfunc !=
> pfunc)
> -                       break;
> +               if (!insn_func(dest_insn) || insn_func(dest_insn)->pfunc !=
> pfunc) {
> +                       prev_offset = reloc_offset(reloc);
> +                       continue;
> +               }

Yeah, something like that might work.

-- 
Josh

Re: [PATCH v4 03/10] objtool: Handle different entry size of rodata

[Josh Poimboeuf]

On Tue, Nov 26, 2024 at 09:22:22PM +0800, Tiezhu Yang wrote:
> > OK, if I understand your comment correctly, this should be an
> > arch-specific function defined in
> > tools/objtool/arch/*/include/arch/elf.h, otherwise it also needs to
> > check ehdr.e_machine
> > in tools/objtool/include/objtool/elf.h.
> 
> There are only macro definitions in
> tools/objtool/arch/*/include/arch/elf.h,
> so I think it is better to add reloc_size() in
> tools/objtool/include/objtool/elf.h,
> like this:
> 
> static inline unsigned int reloc_size(struct elf *elf, struct reloc *reloc)
> {
>         if (elf->ehdr.e_machine == EM_X86_64) {
>                 switch (reloc_type(reloc)) {
>                 case R_X86_64_32:
>                 case R_X86_64_PC32:
>                         return 4;
>                 case R_X86_64_64:
>                 case R_X86_64_PC64:
>                         return 8;
>                 default:
>                         ERROR("unknown X86_64 reloc type");
>                 }
>         } else if (elf->ehdr.e_machine == EM_LOONGARCH) {
>                 switch (reloc_type(reloc)) {
>                 case R_LARCH_32:
>                 case R_LARCH_32_PCREL:
>                         return 4;
>                 case R_LARCH_64:
>                 case R_LARCH_64_PCREL:
>                         return 8;
>                 default:
>                         ERROR("unknown LoongArch reloc type");
>                 }
>         } else {
>                 return 8;
>         }
> }

How about tools/objtool/arch/loongarch/decode.c?  Then we don't need to
check e_machine.

-- 
Josh

Re: [PATCH v4 04/10] objtool: Handle PC relative relocation type

[Josh Poimboeuf]

On Tue, Nov 26, 2024 at 09:24:59PM +0800, Tiezhu Yang wrote:
> On 11/26/2024 07:00 PM, Tiezhu Yang wrote:
> > > Maybe there should be an arch-specific function arch_adjusted_addend().
> 
> Add adjust_offset() in
> tools/objtool/include/objtool/elf.h,
> like this:
> 
> static inline unsigned long adjust_offset(struct elf *elf, struct reloc
> *reloc,
>                                           unsigned long offset)
> {
>         if (elf->ehdr.e_machine == EM_LOONGARCH && reloc_type(reloc) ==
> R_LARCH_32_PCREL)
>                 offset = reloc->sym->offset + reloc_addend(reloc) -
>                          (reloc_offset(reloc) - reloc_offset(table));
> 
>         return offset;
> }
> 
> then call it in check.c, like this:
> 
> offset = reloc->sym->offset + reloc_addend(reloc);
> offset = adjust_offset(file->elf, reloc, offset);
> 
> What do you think?

Similar to reloc_size() I'm thinking this should also be in
arch-specific decode.c.

-- 
Josh

Re: [PATCH v4 06/10] objtool: Handle unsorted table offset of rodata

[Josh Poimboeuf]

On Tue, Nov 26, 2024 at 09:28:19PM +0800, Tiezhu Yang wrote:
> > +		/* Handle the special cases compiled with Clang on LoongArch */
> > +		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
> > +		    reloc->sym->type == STT_SECTION && reloc != table &&
> > +		    reloc_offset(reloc) == reloc_offset(table) + rodata_table_size)
> > +			break;
> 
> I think it can be generic, like this:
> 
>                 /* Check for the end of the table: */
>                 if (reloc != table && reloc == next_table)
>                         break;
> 
>                 if (reloc != table &&
>                     reloc_offset(reloc) == reloc_offset(table) +
> rodata_table_size)
>                         break;
> 
> What do you think?

I'm not sure, this patch is hard to review because it uses
insn->table_size which doesn't get set until the next patch.

Maybe this patch should come after patches 7 & 8, or maybe they should
be squashed?

-- 
Josh

Re: [PATCH v4 02/10] objtool: Handle special cases of dead end insn

[Tiezhu Yang]

On 11/26/2024 11:22 PM, Peter Zijlstra wrote:
>
>> I was poking at the reachable annotations and ended up with this:
>
> Also see here:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/log/?h=objtool/core
>
> Once the robots agree it all compiles, I'll post.

There are many changes of tools/objtool/check.c in your tree,
I assume the patches in objtool/core tree is to target 6.13-rc1,
it failed when compiling on LoongArch:

   arch/loongarch/include/asm/bug.h:49:60: error: expected ‘:’ or ‘)’ 
before ‘;’ token
   {standard input}:682: Error: no match insn: break	1.pushsection 
.discard.annotate_insn,"M",@progbits,8

I think it needs to do the following changes, please squash them to
your original commit if possible, thanks.

diff --git a/arch/loongarch/include/asm/bug.h 
b/arch/loongarch/include/asm/bug.h
index dfb0cfccf36e..e5d888cb738f 100644
--- a/arch/loongarch/include/asm/bug.h
+++ b/arch/loongarch/include/asm/bug.h
@@ -40,13 +40,14 @@

  #define __BUG_FLAGS(flags, extra)                                      \
         asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags))          \
+                            "\n"                                       \
                              extra);

  #define __WARN_FLAGS(flags)                                    \
  do {                                                           \
         instrumentation_begin();                                \
         __BUG_FLAGS(BUGFLAG_WARNING|(flags),                    \
-                   __ANNOTATE_REACHABLE(__ASM_BREF(10001));    \
+                   __ANNOTATE_REACHABLE(__ASM_BREF(10001)));   \
         instrumentation_end();                                  \
  } while (0)

By the way, there are a lot of new objtool warnings

   Unkonwn annotation type: 8

on LoongArch when compiling the code of your tree.

Thanks,
Tiezhu

Re: [PATCH v4 01/10] objtool: Handle various symbol types of rodata

[Tiezhu Yang]

On 11/27/2024 08:52 AM, Josh Poimboeuf wrote:
> On Tue, Nov 26, 2024 at 06:41:29PM +0800, Tiezhu Yang wrote:
>> On 11/26/2024 02:44 PM, Josh Poimboeuf wrote:
>>> On Fri, Nov 22, 2024 at 12:49:56PM +0800, Tiezhu Yang wrote:
>>>> @@ -2094,12 +2095,19 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>>>
>>> 'prev_offset' needs to be updated as well.
>>
>> I am not sure I understand your comment correctly, I can not see
>> what should to do about 'prev_offset'.
>
> Further down the function there is
>
>   prev_offset = reloc_offset(reloc);
>
> which needs to be changed to
>
>   prev_offset = offset;
>
> as part of the patch.

If I understand correctly, reloc_offset(reloc) is different with
reloc->sym->offset + reloc_addend(reloc), tested on x86 and readelf
shows that their values are different, reloc_offset(reloc) is the
first column of .rela.rodata, reloc->sym->offset is the second to
last column of .rela.rodata, reloc_addend(reloc) is the last column
of .rela.rodata.

If do the above change as you suggested, there will be some objtool
warnings on x86. I think it should be:

   prev_offset = reloc_offset(reloc);

rather than:

   prev_offset = offset;

That is to say, no need to change "prev_offset".
Could you please check it again, please let me know if I am wrong.

Thanks,
Tiezhu

Re: [PATCH v4 03/10] objtool: Handle different entry size of rodata

[Tiezhu Yang]

On 11/27/2024 08:57 AM, Josh Poimboeuf wrote:
> On Tue, Nov 26, 2024 at 09:22:22PM +0800, Tiezhu Yang wrote:
>>> OK, if I understand your comment correctly, this should be an
>>> arch-specific function defined in
>>> tools/objtool/arch/*/include/arch/elf.h, otherwise it also needs to
>>> check ehdr.e_machine
>>> in tools/objtool/include/objtool/elf.h.
>>
>> There are only macro definitions in
>> tools/objtool/arch/*/include/arch/elf.h,
>> so I think it is better to add reloc_size() in
>> tools/objtool/include/objtool/elf.h,
>> like this:
>>
>> static inline unsigned int reloc_size(struct elf *elf, struct reloc *reloc)
>> {
>>         if (elf->ehdr.e_machine == EM_X86_64) {
>>                 switch (reloc_type(reloc)) {
>>                 case R_X86_64_32:
>>                 case R_X86_64_PC32:
>>                         return 4;
>>                 case R_X86_64_64:
>>                 case R_X86_64_PC64:
>>                         return 8;
>>                 default:
>>                         ERROR("unknown X86_64 reloc type");
>>                 }
>>         } else if (elf->ehdr.e_machine == EM_LOONGARCH) {
>>                 switch (reloc_type(reloc)) {
>>                 case R_LARCH_32:
>>                 case R_LARCH_32_PCREL:
>>                         return 4;
>>                 case R_LARCH_64:
>>                 case R_LARCH_64_PCREL:
>>                         return 8;
>>                 default:
>>                         ERROR("unknown LoongArch reloc type");
>>                 }
>>         } else {
>>                 return 8;
>>         }
>> }
>
> How about tools/objtool/arch/loongarch/decode.c?  Then we don't need to
> check e_machine.

OK, makes sense, will do it.

Re: [PATCH v4 06/10] objtool: Handle unsorted table offset of rodata

[Tiezhu Yang]

On 11/27/2024 09:20 AM, Josh Poimboeuf wrote:
> On Tue, Nov 26, 2024 at 09:28:19PM +0800, Tiezhu Yang wrote:
>>> +		/* Handle the special cases compiled with Clang on LoongArch */
>>> +		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
>>> +		    reloc->sym->type == STT_SECTION && reloc != table &&
>>> +		    reloc_offset(reloc) == reloc_offset(table) + rodata_table_size)
>>> +			break;
>>
>> I think it can be generic, like this:
>>
>>                 /* Check for the end of the table: */
>>                 if (reloc != table && reloc == next_table)
>>                         break;
>>
>>                 if (reloc != table &&
>>                     reloc_offset(reloc) == reloc_offset(table) +
>> rodata_table_size)
>>                         break;
>>
>> What do you think?
>
> I'm not sure, this patch is hard to review because it uses
> insn->table_size which doesn't get set until the next patch.
>
> Maybe this patch should come after patches 7 & 8, or maybe they should
> be squashed?

OK, I will squash the changes into patch #7.

Re: [PATCH v4 01/10] objtool: Handle various symbol types of rodata

[Josh Poimboeuf]

On Wed, Nov 27, 2024 at 02:39:13PM +0800, Tiezhu Yang wrote:
> On 11/27/2024 08:52 AM, Josh Poimboeuf wrote:
> > On Tue, Nov 26, 2024 at 06:41:29PM +0800, Tiezhu Yang wrote:
> > > On 11/26/2024 02:44 PM, Josh Poimboeuf wrote:
> > > > On Fri, Nov 22, 2024 at 12:49:56PM +0800, Tiezhu Yang wrote:
> > > > > @@ -2094,12 +2095,19 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
> > > > 
> > > > 'prev_offset' needs to be updated as well.
> > > 
> > > I am not sure I understand your comment correctly, I can not see
> > > what should to do about 'prev_offset'.
> > 
> > Further down the function there is
> > 
> >   prev_offset = reloc_offset(reloc);
> > 
> > which needs to be changed to
> > 
> >   prev_offset = offset;
> > 
> > as part of the patch.
> 
> If I understand correctly, reloc_offset(reloc) is different with
> reloc->sym->offset + reloc_addend(reloc), tested on x86 and readelf
> shows that their values are different, reloc_offset(reloc) is the
> first column of .rela.rodata, reloc->sym->offset is the second to
> last column of .rela.rodata, reloc_addend(reloc) is the last column
> of .rela.rodata.
> 
> If do the above change as you suggested, there will be some objtool
> warnings on x86. I think it should be:
> 
>   prev_offset = reloc_offset(reloc);
> 
> rather than:
> 
>   prev_offset = offset;
> 
> That is to say, no need to change "prev_offset".
> Could you please check it again, please let me know if I am wrong.

Sorry, I was confused by the fact there are two different meanings for
"offset": one for where the relocation is written, and one for the
symbol it refers to.

How about instead of 'offset', call it 'sym_offset'?

-- 
Josh

Re: [PATCH v4 06/10] objtool: Handle unsorted table offset of rodata

[Josh Poimboeuf]

On Wed, Nov 27, 2024 at 03:01:33PM +0800, Tiezhu Yang wrote:
> 
> 
> On 11/27/2024 09:20 AM, Josh Poimboeuf wrote:
> > On Tue, Nov 26, 2024 at 09:28:19PM +0800, Tiezhu Yang wrote:
> > > > +		/* Handle the special cases compiled with Clang on LoongArch */
> > > > +		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
> > > > +		    reloc->sym->type == STT_SECTION && reloc != table &&
> > > > +		    reloc_offset(reloc) == reloc_offset(table) + rodata_table_size)
> > > > +			break;
> > > 
> > > I think it can be generic, like this:
> > > 
> > >                 /* Check for the end of the table: */
> > >                 if (reloc != table && reloc == next_table)
> > >                         break;
> > > 
> > >                 if (reloc != table &&
> > >                     reloc_offset(reloc) == reloc_offset(table) +
> > > rodata_table_size)
> > >                         break;
> > > 
> > > What do you think?
> > 
> > I'm not sure, this patch is hard to review because it uses
> > insn->table_size which doesn't get set until the next patch.
> > 
> > Maybe this patch should come after patches 7 & 8, or maybe they should
> > be squashed?
> 
> OK, I will squash the changes into patch #7.

I remembered Ard already solved a similar problem when he prototyped x86
jump table annotation.  Can you pull this patch into your series:

https://lore.kernel.org/20241011170847.334429-12-ardb+git@google.com

-- 
Josh

Re: [PATCH v4 06/10] objtool: Handle unsorted table offset of rodata

[Josh Poimboeuf]

On Wed, Nov 27, 2024 at 04:10:18PM -0800, Josh Poimboeuf wrote:
> On Wed, Nov 27, 2024 at 03:01:33PM +0800, Tiezhu Yang wrote:
> > 
> > 
> > On 11/27/2024 09:20 AM, Josh Poimboeuf wrote:
> > > On Tue, Nov 26, 2024 at 09:28:19PM +0800, Tiezhu Yang wrote:
> > > > > +		/* Handle the special cases compiled with Clang on LoongArch */
> > > > > +		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
> > > > > +		    reloc->sym->type == STT_SECTION && reloc != table &&
> > > > > +		    reloc_offset(reloc) == reloc_offset(table) + rodata_table_size)
> > > > > +			break;
> > > > 
> > > > I think it can be generic, like this:
> > > > 
> > > >                 /* Check for the end of the table: */
> > > >                 if (reloc != table && reloc == next_table)
> > > >                         break;
> > > > 
> > > >                 if (reloc != table &&
> > > >                     reloc_offset(reloc) == reloc_offset(table) +
> > > > rodata_table_size)
> > > >                         break;
> > > > 
> > > > What do you think?
> > > 
> > > I'm not sure, this patch is hard to review because it uses
> > > insn->table_size which doesn't get set until the next patch.
> > > 
> > > Maybe this patch should come after patches 7 & 8, or maybe they should
> > > be squashed?
> > 
> > OK, I will squash the changes into patch #7.
> 
> I remembered Ard already solved a similar problem when he prototyped x86
> jump table annotation.  Can you pull this patch into your series:
> 
> https://lore.kernel.org/20241011170847.334429-12-ardb+git@google.com

Actually, I think I'm going to merge patches 2-5 from Ard's series as
they're a nice cleanup.  Let me do that and then you can base your next
version off tip/objtool/core once it gets updated with Ard's and Peter's
patches.

-- 
Josh

Re: [PATCH v4 06/10] objtool: Handle unsorted table offset of rodata

[Josh Poimboeuf]

On Wed, Nov 27, 2024 at 04:16:29PM -0800, Josh Poimboeuf wrote:
> On Wed, Nov 27, 2024 at 04:10:18PM -0800, Josh Poimboeuf wrote:
> > On Wed, Nov 27, 2024 at 03:01:33PM +0800, Tiezhu Yang wrote:
> > > 
> > > 
> > > On 11/27/2024 09:20 AM, Josh Poimboeuf wrote:
> > > > On Tue, Nov 26, 2024 at 09:28:19PM +0800, Tiezhu Yang wrote:
> > > > > > +		/* Handle the special cases compiled with Clang on LoongArch */
> > > > > > +		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
> > > > > > +		    reloc->sym->type == STT_SECTION && reloc != table &&
> > > > > > +		    reloc_offset(reloc) == reloc_offset(table) + rodata_table_size)
> > > > > > +			break;
> > > > > 
> > > > > I think it can be generic, like this:
> > > > > 
> > > > >                 /* Check for the end of the table: */
> > > > >                 if (reloc != table && reloc == next_table)
> > > > >                         break;
> > > > > 
> > > > >                 if (reloc != table &&
> > > > >                     reloc_offset(reloc) == reloc_offset(table) +
> > > > > rodata_table_size)
> > > > >                         break;
> > > > > 
> > > > > What do you think?
> > > > 
> > > > I'm not sure, this patch is hard to review because it uses
> > > > insn->table_size which doesn't get set until the next patch.
> > > > 
> > > > Maybe this patch should come after patches 7 & 8, or maybe they should
> > > > be squashed?
> > > 
> > > OK, I will squash the changes into patch #7.
> > 
> > I remembered Ard already solved a similar problem when he prototyped x86
> > jump table annotation.  Can you pull this patch into your series:
> > 
> > https://lore.kernel.org/20241011170847.334429-12-ardb+git@google.com
> 
> Actually, I think I'm going to merge patches 2-5 from Ard's series as
> they're a nice cleanup.  Let me do that and then you can base your next
> version off tip/objtool/core once it gets updated with Ard's and Peter's
> patches.

Still talking to myself here, I think we'll only merge the above patch,
since we don't know what the generic annotations are going to look like
yet.

-- 
Josh

Re: [PATCH v4 01/10] objtool: Handle various symbol types of rodata

[Tiezhu Yang]

On 11/28/2024 02:53 AM, Josh Poimboeuf wrote:
> On Wed, Nov 27, 2024 at 02:39:13PM +0800, Tiezhu Yang wrote:
>> On 11/27/2024 08:52 AM, Josh Poimboeuf wrote:
>>> On Tue, Nov 26, 2024 at 06:41:29PM +0800, Tiezhu Yang wrote:
>>>> On 11/26/2024 02:44 PM, Josh Poimboeuf wrote:
>>>>> On Fri, Nov 22, 2024 at 12:49:56PM +0800, Tiezhu Yang wrote:
>>>>>> @@ -2094,12 +2095,19 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>>>>>
>>>>> 'prev_offset' needs to be updated as well.
>>>>
>>>> I am not sure I understand your comment correctly, I can not see
>>>> what should to do about 'prev_offset'.
>>>
>>> Further down the function there is
>>>
>>>   prev_offset = reloc_offset(reloc);
>>>
>>> which needs to be changed to
>>>
>>>   prev_offset = offset;
>>>
>>> as part of the patch.
>>
>> If I understand correctly, reloc_offset(reloc) is different with
>> reloc->sym->offset + reloc_addend(reloc), tested on x86 and readelf
>> shows that their values are different, reloc_offset(reloc) is the
>> first column of .rela.rodata, reloc->sym->offset is the second to
>> last column of .rela.rodata, reloc_addend(reloc) is the last column
>> of .rela.rodata.
>>
>> If do the above change as you suggested, there will be some objtool
>> warnings on x86. I think it should be:
>>
>>   prev_offset = reloc_offset(reloc);
>>
>> rather than:
>>
>>   prev_offset = offset;
>>
>> That is to say, no need to change "prev_offset".
>> Could you please check it again, please let me know if I am wrong.
>
> Sorry, I was confused by the fact there are two different meanings for
> "offset": one for where the relocation is written, and one for the
> symbol it refers to.
>
> How about instead of 'offset', call it 'sym_offset'?

OK, looks better, will modify it in the next version.

Re: [PATCH v4 06/10] objtool: Handle unsorted table offset of rodata

[Tiezhu Yang]

On 11/28/2024 09:00 AM, Josh Poimboeuf wrote:
> On Wed, Nov 27, 2024 at 04:16:29PM -0800, Josh Poimboeuf wrote:
>> On Wed, Nov 27, 2024 at 04:10:18PM -0800, Josh Poimboeuf wrote:
>>> On Wed, Nov 27, 2024 at 03:01:33PM +0800, Tiezhu Yang wrote:
>>>>
>>>>
>>>> On 11/27/2024 09:20 AM, Josh Poimboeuf wrote:
>>>>> On Tue, Nov 26, 2024 at 09:28:19PM +0800, Tiezhu Yang wrote:
>>>>>>> +		/* Handle the special cases compiled with Clang on LoongArch */
>>>>>>> +		if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
>>>>>>> +		    reloc->sym->type == STT_SECTION && reloc != table &&
>>>>>>> +		    reloc_offset(reloc) == reloc_offset(table) + rodata_table_size)
>>>>>>> +			break;
>>>>>>
>>>>>> I think it can be generic, like this:
>>>>>>
>>>>>>                 /* Check for the end of the table: */
>>>>>>                 if (reloc != table && reloc == next_table)
>>>>>>                         break;
>>>>>>
>>>>>>                 if (reloc != table &&
>>>>>>                     reloc_offset(reloc) == reloc_offset(table) +
>>>>>> rodata_table_size)
>>>>>>                         break;
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> I'm not sure, this patch is hard to review because it uses
>>>>> insn->table_size which doesn't get set until the next patch.
>>>>>
>>>>> Maybe this patch should come after patches 7 & 8, or maybe they should
>>>>> be squashed?
>>>>
>>>> OK, I will squash the changes into patch #7.
>>>
>>> I remembered Ard already solved a similar problem when he prototyped x86
>>> jump table annotation.  Can you pull this patch into your series:
>>>
>>> https://lore.kernel.org/20241011170847.334429-12-ardb+git@google.com
>>
>> Actually, I think I'm going to merge patches 2-5 from Ard's series as
>> they're a nice cleanup.  Let me do that and then you can base your next
>> version off tip/objtool/core once it gets updated with Ard's and Peter's
>> patches.
>
> Still talking to myself here, I think we'll only merge the above patch,
> since we don't know what the generic annotations are going to look like
> yet.

OK, my next version will be based on tip/objtool/core after
the merge window, by that time, hope the tree include Ard's
and Peter's patches to avoid conflicts.

Thanks,
Tiezhu

Re: [PATCH v4 06/10] objtool: Handle unsorted table offset of rodata

[Tiezhu Yang]

Hi Josh,

On 11/28/2024 10:28 AM, Tiezhu Yang wrote:
> On 11/28/2024 09:00 AM, Josh Poimboeuf wrote:
>> On Wed, Nov 27, 2024 at 04:16:29PM -0800, Josh Poimboeuf wrote:
>>> On Wed, Nov 27, 2024 at 04:10:18PM -0800, Josh Poimboeuf wrote:
>>>> On Wed, Nov 27, 2024 at 03:01:33PM +0800, Tiezhu Yang wrote:
>>>>>
>>>>>
>>>>> On 11/27/2024 09:20 AM, Josh Poimboeuf wrote:
>>>>>> On Tue, Nov 26, 2024 at 09:28:19PM +0800, Tiezhu Yang wrote:

...

>> Still talking to myself here, I think we'll only merge the above patch,
>> since we don't know what the generic annotations are going to look like
>> yet.
>
> OK, my next version will be based on tip/objtool/core after
> the merge window, by that time, hope the tree include Ard's
> and Peter's patches to avoid conflicts.


I need to rebase my patches based on tip/tip.git objtool/core branch.
Could you please update tip/tip.git objtool/core branch to 6.13-rc1,
and then merge jpoimboe/linux.git objtool/core & objtool-loongarch
branches into tip/tip.git objtool/core branch?

Thanks,
Tiezhu

Re: [PATCH v4 06/10] objtool: Handle unsorted table offset of rodata

[Peter Zijlstra]

On Thu, Nov 28, 2024 at 10:28:01AM +0800, Tiezhu Yang wrote:
> On 11/28/2024 09:00 AM, Josh Poimboeuf wrote:

> > Still talking to myself here, I think we'll only merge the above patch,
> > since we don't know what the generic annotations are going to look like
> > yet.
> 
> OK, my next version will be based on tip/objtool/core after
> the merge window, by that time, hope the tree include Ard's
> and Peter's patches to avoid conflicts.

Should now all be pushed out and visible in tip/objtool/core.

Thanks!