Skip to content

[GlobalOpt] Crash when optimizing global alias to ifunc. #96197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
labrinea opened this issue Jun 20, 2024 · 4 comments · Fixed by #96220 or #96221
Closed

[GlobalOpt] Crash when optimizing global alias to ifunc. #96197

labrinea opened this issue Jun 20, 2024 · 4 comments · Fixed by #96220 or #96221
Assignees
Labels
crash Prefer [crash-on-valid] or [crash-on-invalid] ipo Interprocedural optimizations

Comments

@labrinea
Copy link
Collaborator

labrinea commented Jun 20, 2024

We found a crash when compiling a trivial examle of Function Multiversioning on AArch64:

__attribute__((target_version("simd"))) void helper(void);

__attribute__((target_version("default"))) void helper(void);

int main() { helper(); }

$ clang -O3 --target=aarch64-linux-gnu -rtlib=compiler-rt -c test.c

llvm/include/llvm/ADT/ilist_iterator.h:138: llvm::ilist_iterator<OptionsT, IsReverse, IsConst>::reference llvm::ilist_iterator<OptionsT, IsReverse, IsConst>::operator*() const [with OptionsT = llvm::ilist_detail::node_options<llvm::BasicBlock, true, false, void, false>; bool IsReverse = false; bool IsConst = false; reference = llvm::BasicBlock&]: Assertion `!NodePtr->isKnownSentinel()' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: ../opensource/build_rel_assert/bin/clang -O3 -march=armv8-a tu2.c -rtlib=compiler-rt -c
1.	<eof> parser at end of file
2.	Optimizer
 #0 0x0000af4708f68b80 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
 #1 0x0000af4708f66230 llvm::sys::RunSignalHandlers() (../opensource/build_rel_assert/bin/clang+0x3d36230)
 #2 0x0000af4708ebcda4 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
 #3 0x0000eb2cad92c9d0 (linux-vdso.so.1+0x9d0)
 #4 0x0000eb2cac6ef200 __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x0000eb2cac6aa67c gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #6 0x0000eb2cac697130 abort ./stdlib/abort.c:81:7
 #7 0x0000eb2cac6a3fd0 __assert_fail_base ./assert/assert.c:89:7
 #8 0x0000eb2cac6a4040 __assert_perror_fail ./assert/assert-perr.c:31:1
 #9 0x0000af470889f4d4 llvm::DomTreeBuilder::SemiNCAInfo<llvm::DominatorTreeBase<llvm::BasicBlock, false>>::CalculateFromScratch(llvm::DominatorTreeBase<llvm::BasicBlock, false>&, llvm::DomTreeBuilder::SemiNCAInfo<llvm::DominatorTreeBase<llvm::BasicBlock, false>>::BatchUpdateInfo*) (../opensource/build_rel_assert/bin/clang+0x366f4d4)
#10 0x0000af470889f57c llvm::DominatorTreeAnalysis::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (../opensource/build_rel_assert/bin/clang+0x366f57c)
#11 0x0000af470a1ea728 llvm::detail::AnalysisPassModel<llvm::Function, llvm::DominatorTreeAnalysis, llvm::AnalysisManager<llvm::Function>::Invalidator>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (../opensource/build_rel_assert/bin/clang+0x4fba728)
#12 0x0000af4708984e88 llvm::AnalysisManager<llvm::Function>::getResultImpl(llvm::AnalysisKey*, llvm::Function&) (../opensource/build_rel_assert/bin/clang+0x3754e88)
#13 0x0000af4708e42800 llvm::SROAPass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (../opensource/build_rel_assert/bin/clang+0x3c12800)
#14 0x0000af470a16bf7c llvm::detail::PassModel<llvm::Function, llvm::SROAPass, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (../opensource/build_rel_assert/bin/clang+0x4f3bf7c)
#15 0x0000af4706808310 llvm::detail::PassModel<llvm::Function, llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function>>, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (../opensource/build_rel_assert/bin/clang+0x15d8310)
#16 0x0000af4707f12758 llvm::CGSCCToFunctionPassAdaptor::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) (../opensource/build_rel_assert/bin/clang+0x2ce2758)
#17 0x0000af47067d2bdc llvm::detail::PassModel<llvm::LazyCallGraph::SCC, llvm::CGSCCToFunctionPassAdaptor, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) (../opensource/build_rel_assert/bin/clang+0x15a2bdc)
#18 0x0000af4707f0cc58 llvm::PassManager<llvm::LazyCallGraph::SCC, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) (../opensource/build_rel_assert/bin/clang+0x2cdcc58)
#19 0x0000af470a16be4c llvm::detail::PassModel<llvm::LazyCallGraph::SCC, llvm::PassManager<llvm::LazyCallGraph::SCC, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) (../opensource/build_rel_assert/bin/clang+0x4f3be4c)
#20 0x0000af4707f13338 llvm::DevirtSCCRepeatedPass::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) (../opensource/build_rel_assert/bin/clang+0x2ce3338)

Looks like we are trying to run an analysis on a function declaration or something like that. Digging more I found that the problem comes from GlobalOpt. Here is the minimal reproducer:

target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128-Fn32"
target triple = "aarch64-unknown-linux-gnu"

$helper.resolver = comdat any

@helper.ifunc = weak_odr dso_local alias void (), ptr @helper

@helper = weak_odr dso_local ifunc void (), ptr @helper.resolver

declare void @helper._Msimd()

define weak_odr ptr @helper.resolver() comdat {
resolver_entry:
  ret ptr @helper._Msimd
}

$ opt -passes=globalopt -S test.ll -debug

Args: ./bin/opt -passes=globalopt -S test.ll -debug 
@helper.ifunc = weak_odr dso_local alias void (), ptr @helper
@helper.ifunc = weak_odr dso_local alias void (), ptr @helper._Msimd
Alias must point to a definition
ptr @helper.ifunc
LLVM ERROR: Broken module found, compilation aborted!
@labrinea
Copy link
Collaborator Author

labrinea commented Jun 20, 2024

@jroelofs I believe the optimization of static ifuncs does not interact well with the optimization of global aliases. If we guard your optimization by checking that none of the ifunc users is a global alias, then it will never trigger. So perhaps we want to remove the emission of the alias from clang in the first place. I am not sure why it's there, if anyone relies on it. Perhaps @DanielKristofKiss knows.

More digging -> #74358 (comment)

@EugeneZelenko EugeneZelenko added llvm:optimizations crash Prefer [crash-on-valid] or [crash-on-invalid] and removed new issue labels Jun 20, 2024
@jroelofs
Copy link
Contributor

We could guard it on not having a global alias, or if there is one, on having a definition for the resolvee. It would turn the optimization off for forward declaration uses like this one, and keep it for uses within a TU or in LTO.

@labrinea
Copy link
Collaborator Author

That's an option; a bit conservative in my opinion. For non LTO compilation with version declaration in one file and implementation in another your optimization won't trigger. I guess it's the only option if we care about backwards compatibility, which was the reason we added the alias in the first place as far as I understand from reading #74358.

labrinea added a commit to labrinea/llvm-project that referenced this issue Jun 20, 2024
Fixes llvm#96197.

A global alias should always point to a definition. Ifuncs are
definitions, so far so good. However an ifunc may be statically
resolved to a function that is declared but not defined in the
translation unit.

With this patch we perform static resolution if:
 * the resolvee is defined
 * otherwise none of the ifunc users is a global alias
labrinea added a commit to labrinea/llvm-project that referenced this issue Jun 20, 2024
Long story short the interaction of two optimizations happening in
GlobalOpt results in a crash. For more details look at the issue
llvm#96197. I will be
fixing this in GlobalOpt but it is a conservative solution since
it won't allow us to optimize resolvers which return a pointer
to a function whose definition is in another TU when compiling
without LTO:

__attribute__((target_version("simd"))) void bar(void);
__attribute__((target_version("default"))) void bar(void);
int foo() { bar(); }
@labrinea labrinea self-assigned this Jun 20, 2024
@DanielKristofKiss
Copy link
Member

@jroelofs I believe the optimization of static ifuncs does not interact well with the optimization of global aliases. If we guard your optimization by checking that none of the ifunc users is a global alias, then it will never trigger. So perhaps we want to remove the emission of the alias from clang in the first place. I am not sure why it's there, if anyone relies on it. Perhaps @DanielKristofKiss knows.

More digging -> #74358 (comment)

Alias added due to conservative thinking, nothing more.

@EugeneZelenko EugeneZelenko added ipo Interprocedural optimizations and removed llvm:optimizations labels Jun 21, 2024
labrinea added a commit that referenced this issue Jun 24, 2024
Long story short the interaction of two optimizations happening in
GlobalOpt results in a crash. For more details look at the issue
#96197. I will be fixing this
in GlobalOpt but it is a conservative solution since it won't allow us
to optimize resolvers which return a pointer to a function whose
definition is in another TU when compiling without LTO:

```
__attribute__((target_version("simd"))) void bar(void);
__attribute__((target_version("default"))) void bar(void);
int foo() { bar(); }
```

fixes: #96197
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this issue Jul 9, 2024
…lvm#96220)

Fixes llvm#96197.

A global alias should always point to a definition. Ifuncs are
definitions, so far so good. However an ifunc may be statically resolved
to a function that is declared but not defined in the translation unit.

With this patch we perform static resolution if:
 * the resolvee is defined, else if
 * none of the ifunc users is a global alias
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this issue Jul 9, 2024
Long story short the interaction of two optimizations happening in
GlobalOpt results in a crash. For more details look at the issue
llvm#96197. I will be fixing this
in GlobalOpt but it is a conservative solution since it won't allow us
to optimize resolvers which return a pointer to a function whose
definition is in another TU when compiling without LTO:

```
__attribute__((target_version("simd"))) void bar(void);
__attribute__((target_version("default"))) void bar(void);
int foo() { bar(); }
```

fixes: llvm#96197
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crash Prefer [crash-on-valid] or [crash-on-invalid] ipo Interprocedural optimizations
Projects
None yet
4 participants