[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

thakis added a comment. Mentioning it here in case others run into the same thing: We bisected a 7x (!) binary size regression to this. Details at https://bugs.chromium.org/p/chromium/issues/detail?id=1261715 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

This revision was automatically updated to reflect the committed changes. Closed by commit rG7ce1c4da7726: ThinLTO: Fix inline assembly references to static functions with CFI (authored by samitolvanen). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

nickdesaulniers added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:45 + } + return true; +} samitolvanen wrote: > nickdesaulniers wrote: > > samitolvanen wrote: > > > nickdesaulniers wrote: > > > > Can llvm::any_of or llvm::none

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

rnk added a comment. I think the new approach of skipping non C-ish identifier names is reasonable. Looks good to me, but wait for the more active reviewers to stamp it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llv

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:45 + } + return true; +} nickdesaulniers wrote: > samitolvanen wrote: > > nickdesaulniers wrote: > > > Can llvm::any_of or llvm::none_of be used here? > > > llvm/AD

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

nickdesaulniers added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:45 + } + return true; +} samitolvanen wrote: > nickdesaulniers wrote: > > Can llvm::any_of or llvm::none_of be used here? > > llvm/ADT/STLExtras.h > Maybe, but I

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 363166. samitolvanen added a comment. Also skip functions with names incompatible with XCOFF. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 Files: llvm/lib/T

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:39 + // Promotion aliases are used only in inline assembly. It's safe to + // simply skip unusual names. Matches MCAsmInfo::isAcceptableChar(). + for (const char &C : Name) { -

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

nickdesaulniers added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:39 + // Promotion aliases are used only in inline assembly. It's safe to + // simply skip unusual names. Matches MCAsmInfo::isAcceptableChar(). + for (const char &C : Name) { --

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 360615. samitolvanen added a comment. This revision is now accepted and ready to land. As we only care about fixing inline assembly references, mangled names are not that important in the first place. This version skips any functions that have unusual cha

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:77 + // references from inline assembly. + std::string Alias = ".set \"" + OldName + "\",\"" + NewName + "\"\n"; + ExportM.appendModuleInlineAsm(Alias); ---

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:77 + // references from inline assembly. + std::string Alias = ".set \"" + OldName + "\",\"" + NewName + "\"\n"; + ExportM.appendModuleInlineAsm(Alias); ---

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

rnk added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:77 + // references from inline assembly. + std::string Alias = ".set \"" + OldName + "\",\"" + NewName + "\"\n"; + ExportM.appendModuleInlineAsm(Alias); nickdes

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

nickdesaulniers added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:77 + // references from inline assembly. + std::string Alias = ".set \"" + OldName + "\",\"" + NewName + "\"\n"; + ExportM.appendModuleInlineAsm(Alias);

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

nickdesaulniers added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:77 + // references from inline assembly. + std::string Alias = ".set \"" + OldName + "\",\"" + NewName + "\"\n"; + ExportM.appendModuleInlineAsm(Alias);

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 360267. samitolvanen added a comment. Folded in D106392 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 Files: llvm/lib/Tra

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added a comment. In D104058#2891551 , @morehouse wrote: > This patch broke the sanitizer-windows bot: > https://lab.llvm.org/buildbot/#/builders/127/builds/14257 > > Failed Tests (4): > cfi-devirt-lld-thinlto-x86_64 :: anon-namespace.

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

morehouse added a comment. This patch broke the sanitizer-windows bot: https://lab.llvm.org/buildbot/#/builders/127/builds/14257 Failed Tests (4): cfi-devirt-lld-thinlto-x86_64 :: anon-namespace.cpp cfi-devirt-lld-thinlto-x86_64 :: simple-pass.cpp cfi-standalone-lld-thinlto-x86_64

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG700d07f8ce6f: ThinLTO: Fix inline assembly references to static functions with CFI (authored by samitolvanen). Repository: rG LLVM Github Monorepo

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

nickdesaulniers accepted this revision. nickdesaulniers added a comment. Sorry, I always forget when these are necessary. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 __

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 359463. samitolvanen added a comment. Added `REQUIRES: x86-registered-target` to tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 Files: llvm/lib/Transfo

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen reopened this revision. samitolvanen added a comment. This revision is now accepted and ready to land. The tests that now specify a target triple also need `; REQUIRES: x86-registered-target` or they will obviously fail. I'll upload another revision. Repository: rG LLVM Github M

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8e3b5cb39eef: ThinLTO: Fix inline assembly references to static functions with CFI (authored by samitolvanen). Repository: rG LLVM Github Monorepo

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

pcc accepted this revision. pcc added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 ___ cfe-commits mailing list cfe-commits@lists.llvm.org ht

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added a comment. In D104058#2883804 , @nickdesaulniers wrote: > In D104058#2878083 , @samitolvanen > wrote: > >> In D104058#2877631 , >> @nickdesaulniers w

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

nickdesaulniers added a comment. In D104058#2878083 , @samitolvanen wrote: > In D104058#2877631 , > @nickdesaulniers wrote: > >> Change LGTM, but I don't understand why the following tests are modified: >> >> -

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added a comment. In D104058#2877631 , @nickdesaulniers wrote: > Change LGTM, but I don't understand why the following tests are modified: > > - llvm/test/ThinLTO/X86/devirt2.ll This is needed to fix two `missing symbol resolution` errors th

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

nickdesaulniers added a comment. Change LGTM, but I don't understand why the following tests are modified: - llvm/test/ThinLTO/X86/devirt2.ll - llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal2.ll - llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc-internal.ll Repository: rG LLVM

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 358364. samitolvanen added a comment. Moved the alias creation to module level inline assembly to avoid issues with LowerTypeTestsModule, based on pcc's suggestion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added a comment. Thanks for the revert, I'll take a look. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 ___ cfe-commits mailing list cfe-commits@

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

zequanwu added a comment. Hi, this caused compiler crash: "Assertion `materialized_use_empty() && "Uses remain when a value is destroyed!"'" on chromium build https://ci.chromium.org/ui/p/chromium/builders/try/linux-official/151/overview. Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe3d24b45b8f8: ThinLTO: Fix inline assembly references to static functions with CFI (authored by samitolvanen). Repository: rG LLVM Github Monorepo

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:58-80 +std::string OldName = Name.str(); std::string NewName = (Name + ModuleId).str(); if (const auto *C = ExportGV.getComdat()) if (C->getName() == Name)

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

nickdesaulniers added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:58-80 +std::string OldName = Name.str(); std::string NewName = (Name + ModuleId).str(); if (const auto *C = ExportGV.getComdat()) if (C->getName() == Name)

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 353797. samitolvanen added a comment. Fix a use-of-uninitialized-value error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 Files: llvm/lib/Transforms/IPO/Th

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

thakis added a comment. Looks like this breaks check-llvm on mac, see http://45.33.8.238/mac/32814/summary.html Please take a look and revert for now if it takes a while to fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://re

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

This revision was automatically updated to reflect the committed changes. Closed by commit rG4474958d3a97: ThinLTO: Fix inline assembly references to static functions with CFI (authored by samitolvanen). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen updated this revision to Diff 352825. samitolvanen added a comment. Moved the test to llvm/test/Transforms/ThinLTOBitcodeWriter. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llvm.org/D104058 Files: llvm/

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

pcc accepted this revision. pcc added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/test/CodeGen/thinlto-cfi-icall-static-inline-asm.c:3 + +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -flto=thin -fsanitize=cfi-icall -f

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen added reviewers: nickdesaulniers, pcc, tejohnson, kees, eugenis. samitolvanen added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:79 + GlobalValue::InternalLinkage, Name, F, &ExportM); + appendToCompil

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

samitolvanen created this revision. Herald added subscribers: ormris, steven_wu, hiraditya, inglorion. samitolvanen requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Create an internal alias with the original name for static