Messages in this thread | | | From | Nick Desaulniers <> | Date | Mon, 28 Jun 2021 10:54:24 -0700 | Subject | Re: [PATCH] kallsyms: strip LTO suffixes from static functions |
| |
On Tue, Jun 22, 2021 at 1:18 PM Fangrui Song <maskray@google.com> wrote: > > On 2021-06-22, 'Nick Desaulniers' via Clang Built Linux wrote: > >Similar to: > >commit 8b8e6b5d3b01 ("kallsyms: strip ThinLTO hashes from static > >functions") > > > >It's very common for compilers to modify the symbol name for static > >functions as part of optimizing transformations. That makes hooking > >static functions (that weren't inlined or DCE'd) with kprobes difficult. > > > >Full LTO uses a different mangling scheme than thin LTO; full LTO > >imports all code into effectively one big translation unit. It must > >rename static functions to prevent collisions. Strip off these suffixes > >so that we can continue to hook such static functions. > > See below. The message needs a change. > > I can comment on the LTO side thing, but a maintainer needs to check > about the kernel side logic. > > Reviewed-by: Fangrui Song <maskray@google.com> > > >Reported-by: KE.LI(Lieke) <like1@oppo.com> > >Tested-by: KE.LI(Lieke) <like1@oppo.com> > >Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > >--- > > kernel/kallsyms.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > >diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > >index 4067564ec59f..14cf3a6474de 100644 > >--- a/kernel/kallsyms.c > >+++ b/kernel/kallsyms.c > >@@ -188,6 +188,24 @@ static inline bool cleanup_symbol_name(char *s) > > > > return res != NULL; > > } > >+#elif defined(CONFIG_LTO_CLANG_FULL) > >+/* > >+ * LLVM mangles static functions for full LTO so that two static functions with > >+ * the same identifier do not collide when all code is combined into one > >+ * module. The scheme used converts references to foo into > >+ * foo.llvm.974640843467629774, for example. This can break hooking of static > >+ * functions with kprobes. > >+ */ > > The comment should say ThinLTO instead. > > The .llvm.123 suffix is for global scope promotion for local linkage > symbols. The scheme is ThinLTO specific. This ensures that a local
Oh, boy. Indeed. I had identified the mangling coming from getGlobalNameForLocal(), but looking at the call chain now I see:
FunctionImportGlobalProcessing::processGlobalForThinLTO() -> FunctionImportGlobalProcessing::getPromotedName() -> ModuleSummaryIndex::getGlobalNameForLocal()
I'm not sure then how I figured it was specific to full LTO.
Android recently switched from thin LTO to full LTO, which is what I assumed was the cause of the bug report. Rereading our internal bug report, it was tested against a prior version that did the symbol truncation for thinLTO. I then assumed this was full LTO specific for whatever reason, and modified the patch to only apply to full LTO. I see via the above call chain that this patch is not correct. Let me send my original patch as a v2. b/189560201 if you're interested.
> linkage symbol, when imported into multiple translation units, then > compiled into different object files, during linking, the copies can be > deduplicated. This matters for code size and for correctness when the > function address is taken. > > Regular LTO (sometimes called full LTO) uses the regular name.\d+ > scheme. > > >+static inline bool cleanup_symbol_name(char *s) > >+{ > >+ char *res; > >+ > >+ res = strstr(s, ".llvm."); > >+ if (res) > >+ *res = '\0'; > >+ > >+ return res != NULL; > >+} > > #else > > static inline bool cleanup_symbol_name(char *s) { return false; } > > #endif > >-- > >2.32.0.288.g62a8d224e6-goog > > I wonder whether it makes sense to strip all `.something` suffixes. > For example, the recent -funique-internal-linkage-name (which can > improve sample profile accuracy) uses the `.__uniq.1234` scheme. > > Function specialization/clones can create arbitrary `.123` suffixes.
I definitely don't see hooking static functions via kprobes as being scalable. There are numerous different mangling schemes different compilers apply to different static functions.
-- Thanks, ~Nick Desaulniers
| |