| Message ID | 20241026051410.2819338-3-xur@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add AutoFDO and Propeller support for Clang build | expand |
On Fri, Oct 25, 2024 at 10:14:04PM -0700, Rong Xu wrote: > In the presence of both weak and strong function definitions, the > linker drops the weak symbol in favor of a strong symbol, but > leaves the code in place. Code in ignore_unreachable_insn() has > some heuristics to suppress the warning, but it does not work when > -ffunction-sections is enabled. > > Suppose function foo has both strong and weak definitions. > Case 1: The strong definition has an annotated section name, > like .init.text. Only the weak definition will be placed into > .text.foo. But since the section has no symbols, there will be no > "hole" in the section. > > Case 2: Both sections are without an annotated section name. > Both will be placed into .text.foo section, but there will be only one > symbol (the strong one). If the weak code is before the strong code, > there is no "hole" as it fails to find the right-most symbol before > the offset. > > The fix is to use the first node to compute the hole if hole.sym > is empty. If there is no symbol in the section, the first node > will be NULL, in which case, -1 is returned to skip the whole > section. > > Co-developed-by: Han Shen <shenhan@google.com> > Signed-off-by: Han Shen <shenhan@google.com> This seems logically correct to me, but I'd love to see review from Josh and/or Peter Z on this change too. Reviewed-by: Kees Cook <kees@kernel.org>
On 10/28/24 17:16, Kees Cook wrote: > On Fri, Oct 25, 2024 at 10:14:04PM -0700, Rong Xu wrote: >> In the presence of both weak and strong function definitions, the >> linker drops the weak symbol in favor of a strong symbol, but >> leaves the code in place. Code in ignore_unreachable_insn() has >> some heuristics to suppress the warning, but it does not work when >> -ffunction-sections is enabled. >> >> Suppose function foo has both strong and weak definitions. >> Case 1: The strong definition has an annotated section name, >> like .init.text. Only the weak definition will be placed into >> .text.foo. But since the section has no symbols, there will be no >> "hole" in the section. >> >> Case 2: Both sections are without an annotated section name. >> Both will be placed into .text.foo section, but there will be only one >> symbol (the strong one). If the weak code is before the strong code, >> there is no "hole" as it fails to find the right-most symbol before >> the offset. >> >> The fix is to use the first node to compute the hole if hole.sym >> is empty. If there is no symbol in the section, the first node >> will be NULL, in which case, -1 is returned to skip the whole >> section. >> >> Co-developed-by: Han Shen <shenhan@google.com> >> Signed-off-by: Han Shen <shenhan@google.com> > > This seems logically correct to me, but I'd love to see review from Josh > and/or Peter Z on this change too. > > Reviewed-by: Kees Cook <kees@kernel.org> > Does this happen even with -Wl,--gc-sections? -hpa
On Mon, Oct 28, 2024 at 05:16:44PM -0700, Kees Cook wrote: > On Fri, Oct 25, 2024 at 10:14:04PM -0700, Rong Xu wrote: > > In the presence of both weak and strong function definitions, the > > linker drops the weak symbol in favor of a strong symbol, but > > leaves the code in place. Code in ignore_unreachable_insn() has > > some heuristics to suppress the warning, but it does not work when > > -ffunction-sections is enabled. > > > > Suppose function foo has both strong and weak definitions. > > Case 1: The strong definition has an annotated section name, > > like .init.text. Only the weak definition will be placed into > > .text.foo. But since the section has no symbols, there will be no > > "hole" in the section. > > > > Case 2: Both sections are without an annotated section name. > > Both will be placed into .text.foo section, but there will be only one > > symbol (the strong one). If the weak code is before the strong code, > > there is no "hole" as it fails to find the right-most symbol before > > the offset. > > > > The fix is to use the first node to compute the hole if hole.sym > > is empty. If there is no symbol in the section, the first node > > will be NULL, in which case, -1 is returned to skip the whole > > section. > > > > Co-developed-by: Han Shen <shenhan@google.com> > > Signed-off-by: Han Shen <shenhan@google.com> > > This seems logically correct to me, but I'd love to see review from Josh > and/or Peter Z on this change too. > > Reviewed-by: Kees Cook <kees@kernel.org> LGTM, thanks! Acked-by: Josh Poimboeuf <jpoimboe@kernel.org>
I think the objtool works on individual object files (or vmlinux.o). The -Wl,--gc-sections flag, on the other hand, is a linker flag that acts on the final link -- it's applied after objtool invocations. Therefore, even with -Wl,--gc-sections, we'll still encounter those spurious warnings from objtool. -Rong On Mon, Oct 28, 2024 at 5:19 PM H. Peter Anvin <hpa@zytor.com> wrote: > > On 10/28/24 17:16, Kees Cook wrote: > > On Fri, Oct 25, 2024 at 10:14:04PM -0700, Rong Xu wrote: > >> In the presence of both weak and strong function definitions, the > >> linker drops the weak symbol in favor of a strong symbol, but > >> leaves the code in place. Code in ignore_unreachable_insn() has > >> some heuristics to suppress the warning, but it does not work when > >> -ffunction-sections is enabled. > >> > >> Suppose function foo has both strong and weak definitions. > >> Case 1: The strong definition has an annotated section name, > >> like .init.text. Only the weak definition will be placed into > >> .text.foo. But since the section has no symbols, there will be no > >> "hole" in the section. > >> > >> Case 2: Both sections are without an annotated section name. > >> Both will be placed into .text.foo section, but there will be only one > >> symbol (the strong one). If the weak code is before the strong code, > >> there is no "hole" as it fails to find the right-most symbol before > >> the offset. > >> > >> The fix is to use the first node to compute the hole if hole.sym > >> is empty. If there is no symbol in the section, the first node > >> will be NULL, in which case, -1 is returned to skip the whole > >> section. > >> > >> Co-developed-by: Han Shen <shenhan@google.com> > >> Signed-off-by: Han Shen <shenhan@google.com> > > > > This seems logically correct to me, but I'd love to see review from Josh > > and/or Peter Z on this change too. > > > > Reviewed-by: Kees Cook <kees@kernel.org> > > > > Does this happen even with -Wl,--gc-sections? > > -hpa >
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c index 3d27983dc908d..6f64d611faea9 100644 --- a/tools/objtool/elf.c +++ b/tools/objtool/elf.c @@ -224,12 +224,17 @@ int find_symbol_hole_containing(const struct section *sec, unsigned long offset) if (n) return 0; /* not a hole */ - /* didn't find a symbol for which @offset is after it */ - if (!hole.sym) - return 0; /* not a hole */ + /* + * @offset >= sym->offset + sym->len, find symbol after it. + * When hole.sym is empty, use the first node to compute the hole. + * If there is no symbol in the section, the first node will be NULL, + * in which case, -1 is returned to skip the whole section. + */ + if (hole.sym) + n = rb_next(&hole.sym->node); + else + n = rb_first_cached(&sec->symbol_tree); - /* @offset >= sym->offset + sym->len, find symbol after it */ - n = rb_next(&hole.sym->node); if (!n) return -1; /* until end of address space */