Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Transforms] LowerSwitch tries to get sext on i128, causing an assertion error. #59316

Closed
DataCorrupted opened this issue Dec 3, 2022 · 9 comments
Closed

[Transforms] LowerSwitch tries to get sext on i128, causing an assertion error. #59316

DataCorrupted opened this issue Dec 3, 2022 · 9 comments
Assignees
@DataCorrupted
Labels
crash Prefer [crash-on-valid] or [crash-on-invalid] llvm:optimizations

Comments

@DataCorrupted
Copy link
Member

DataCorrupted commented Dec 3, 2022 •
edited by VoltrexKeyva

In LowerSwitch, we are assuming treating switch cases is smaller than int64 by directly asking for a sext value.
However, this can go wrong if the case value is higher typed huge number.
For example, https://godbolt.org/z/f8cMnKnGE shows a case where we use i128 and crashes the compiler.

llvm-project/llvm/lib/Transforms/Utils/LowerSwitch.cpp

Line 331 in 7cf5581

int64_t nextValue = J->Low->getSExtValue();

Here is a stack trace:

llc: /home/peter/aflplusplus-isel/llvm-fix/llvm/include/llvm/ADT/APInt.h:1501: int64_t llvm::APInt::getSExtValue() const: Assertion `getSignificantBits() <= 64 && "Too many bits for int64_t"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: ./bin/llc -mtriple=amdgcn crash/amdgcn/0/poc.ll
1.      Running pass 'CallGraph Pass Manager' on module 'crash/amdgcn/0/poc.ll'.
2.      Running pass 'Lower SwitchInst's to branches' on function '@f'
 #0 0x00007f52dcdce4ba llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Support/Unix/Signals.inc:567:11
 #1 0x00007f52dcdce66b PrintStackTraceSignalHandler(void*) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Support/Unix/Signals.inc:641:1
 #2 0x00007f52dcdcccc6 llvm::sys::RunSignalHandlers() /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Support/Signals.cpp:104:5
 #3 0x00007f52dcdced95 SignalHandler(int) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Support/Unix/Signals.inc:412:1
 #4 0x00007f52db58c980 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12980)
 #5 0x00007f52da888e87 raise /build/glibc-CVJwZb/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #6 0x00007f52da88a7f1 abort /build/glibc-CVJwZb/glibc-2.27/stdlib/abort.c:81:0
 #7 0x00007f52da87a3fa __assert_fail_base /build/glibc-CVJwZb/glibc-2.27/assert/assert.c:89:0
 #8 0x00007f52da87a472 (/lib/x86_64-linux-gnu/libc.so.6+0x30472)
 #9 0x00007f52dc04bfc5 llvm::APInt::getSExtValue() const /home/peter/aflplusplus-isel/llvm-fix/llvm/include/llvm/ADT/APInt.h:0:5
#10 0x00007f52dc1d2a89 llvm::ConstantInt::getSExtValue() const /home/peter/aflplusplus-isel/llvm-fix/llvm/include/llvm/IR/Constants.h:148:41
#11 0x00007f52dc258a65 (anonymous namespace)::Clusterify(std::vector<(anonymous namespace)::CaseRange, std::allocator<(anonymous namespace)::CaseRange>>&, llvm::SwitchInst*) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Transforms/Utils/LowerSwitch.cpp:331:15
#12 0x00007f52dc257dc6 (anonymous namespace)::ProcessSwitchInst(llvm::SwitchInst*, llvm::SmallPtrSetImpl<llvm::BasicBlock*>&, llvm::AssumptionCache*, llvm::LazyValueInfo*) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Transforms/Utils/LowerSwitch.cpp:372:18
#13 0x00007f52dc257af3 (anonymous namespace)::LowerSwitch(llvm::Function&, llvm::LazyValueInfo*, llvm::AssumptionCache*) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Transforms/Utils/LowerSwitch.cpp:535:3
#14 0x00007f52dc257ceb (anonymous namespace)::LowerSwitchLegacyPass::runOnFunction(llvm::Function&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Transforms/Utils/LowerSwitch.cpp:595:3
#15 0x00007f52df7149e6 llvm::FPPassManager::runOnFunction(llvm::Function&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/IR/LegacyPassManager.cpp:1430:23
#16 0x00007f52e1fa9eed (anonymous namespace)::CGPassManager::RunPassOnSCC(llvm::Pass*, llvm::CallGraphSCC&, llvm::CallGraph&, bool&, bool&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Analysis/CallGraphSCCPass.cpp:179:20
#17 0x00007f52e1fa987e (anonymous namespace)::CGPassManager::RunAllPassesOnSCC(llvm::CallGraphSCC&, llvm::CallGraph&, bool&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Analysis/CallGraphSCCPass.cpp:476:10
#18 0x00007f52e1fa91ff (anonymous namespace)::CGPassManager::runOnModule(llvm::Module&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Analysis/CallGraphSCCPass.cpp:542:18
#19 0x00007f52df7152b9 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/IR/LegacyPassManager.cpp:1545:23
#20 0x00007f52df714e2d llvm::legacy::PassManagerImpl::run(llvm::Module&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/IR/LegacyPassManager.cpp:535:16
#21 0x00007f52df719af1 llvm::legacy::PassManager::run(llvm::Module&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/IR/LegacyPassManager.cpp:1672:3
#22 0x00000000004199ac compileModule(char**, llvm::LLVMContext&) /home/peter/aflplusplus-isel/llvm-fix/llvm/tools/llc/llc.cpp:736:41
#23 0x0000000000417d52 main /home/peter/aflplusplus-isel/llvm-fix/llvm/tools/llc/llc.cpp:417:13
#24 0x00007f52da86bc87 __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:344:0
#25 0x000000000041755a _start (./bin/llc+0x41755a)
Aborted

I see two ways to fix it:

  1. Change the documentation and clarify that switch case can be at most int64_t and will be treated as signed integer.
  2. Change the whole file with APInt API.
@DataCorrupted DataCorrupted added bug Indicates an unexpected problem or unintended behavior llvm Umbrella label for LLVM issues labels Dec 3, 2022
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 3, 2022

@llvm/issue-subscribers-bug

@DataCorrupted DataCorrupted changed the title int64_t APInt::getSExtValue() on i128 caused assertion error. [Transforms] LowerSwitch tries to get sext on i128, causing an assertion error. Dec 3, 2022
@EugeneZelenko EugeneZelenko added llvm:optimizations crash Prefer [crash-on-valid] or [crash-on-invalid] and removed bug Indicates an unexpected problem or unintended behavior llvm Umbrella label for LLVM issues labels Dec 3, 2022
@rotateright
Copy link
Contributor

rotateright commented Dec 5, 2022 •
edited

I think option 2 is the only viable solution. LowerSwitch is not part of the default set of codegen passes; only enabled for amdgcn? Otherwise, this bug would have been noticed sooner.

@rotateright
Copy link
Contributor

Another potential option (but I have no idea if it's viable): just bail out on switches with oversized type. Presumably, the default switch lowering can handle those minimally (otherwise every target would be crashing). Since oversized switches were never expected to exist for targets that use this pass, it shouldn't be a real-world concern as long as we don't crash?

@DataCorrupted
Copy link
Member Author

I am not really comfortable with bail out solution. That doesn't fit into the expectation of this pass (unless we document it explictly), other people may use it in the future and expecting it to work on i128 when in fact we bail, that could be a bigger problem.

I am happy to apply option 2 when I have time.

@DataCorrupted
Copy link
Member Author

I think there is another discussion here I would like to talk about. @RKSimon

It seems to me APInt exists to represent integers with (probably) higher precisions than 64.
API like getSExtValue or getZExtValue that assumes the integer is less than 64bit or crash, this doesn't really make sense.
I am thinking if we should make some changes to that.

Here's one idea: getSExtValue returns std::optional<int64_t>, so a) this api doesn't crash on arbitrary integer, and b) we force the user to think about the bitwidth before they call it.
For now it seems people are just calling it without thinking twice (like this file).

@RKSimon
Copy link
Collaborator

RKSimon commented Dec 5, 2022

There might be uses for an alternative helper (getLegalSExtValue?) but the existing getSExtValue/getZExtValue methods are very widespread and make use of being implicitly inrange.

@RKSimon
Copy link
Collaborator

RKSimon commented Dec 6, 2022

Something else that has encouraged people to use getSExtValue/getZExtValue is because APInt values don't work well with other APInt values if their bitwidths aren't the same - IMO at least the non-equality comparisons shouldn't have that limit.

@DataCorrupted
Copy link
Member Author

There might be uses for an alternative helper (getLegalSExtValue?) but the existing getSExtValue/getZExtValue methods are very widespread and make use of being implicitly inrange.

I wonder what would 'getLegalSExtValue' return, an option like I mentioned?

I think another idea is to roll out new, safe APIs and try to slowly deprecate the old ones, if this API is widely used.

DataCorrupted added a commit that referenced this issue Dec 15, 2022
@DataCorrupted
[APInt] provide a safe API for zext value and sext value.
5596810 
Currently, APInt::getSExtValue and getZExtValue crashes on values with more than 64 bits.
Users may accidently crash the compiler with this setting when the integer may be i128.
As shown in #59316

In this patch we provide a trySExtValue and tryZExtValue to return an Optional, the user
needs to explictly unwrap it and condsier the possibility where there my no value in it.

Reviewed By: RKSimon

Differential Revision: https://reviews.llvm.org/D139683
GuilhermeValarini pushed a commit to GuilhermeValarini/llvm-project that referenced this issue Dec 24, 2022
@DataCorrupted @GuilhermeValarini
[APInt] provide a safe API for zext value and sext value.
5c962ae 
Currently, APInt::getSExtValue and getZExtValue crashes on values with more than 64 bits.
Users may accidently crash the compiler with this setting when the integer may be i128.
As shown in llvm#59316

In this patch we provide a trySExtValue and tryZExtValue to return an Optional, the user
needs to explictly unwrap it and condsier the possibility where there my no value in it.

Reviewed By: RKSimon

Differential Revision: https://reviews.llvm.org/D139683
@DataCorrupted DataCorrupted self-assigned this Dec 29, 2022
@DataCorrupted
Copy link
Member Author

Candidate patch: https://reviews.llvm.org/D140747

CarlosAlbertoEnciso pushed a commit to SNSystems/llvm-debuginfo-analyzer that referenced this issue Jan 6, 2023
@DataCorrupted @CarlosAlbertoEnciso
[Transform] Rewrite LowerSwitch using APInt
1b6c632 
This rewrite fixes llvm/llvm-project#59316.

Previously LowerSwitch uses int64_t, which will crash on case branches using integers with more than 64 bits.
Using APInt fixes this problem. This patch also includes a test

Reviewed By: RKSimon

Differential Revision: https://reviews.llvm.org/D140747
@DataCorrupted DataCorrupted mentioned this issue Jan 7, 2023
Closed
DataCorrupted added a commit that referenced this issue Jan 25, 2023
@DataCorrupted
[Transform] Rewrite LowerSwitch using APInt
9b70a28 
This rewrite fixes #59316.

Previously LowerSwitch uses int64_t, which will crash on case branches using integers with more than 64 bits.
Using APInt fixes this problem. This patch also includes a test

Reviewed By: RKSimon

Differential Revision: https://reviews.llvm.org/D140747
CarlosAlbertoEnciso pushed a commit to SNSystems/llvm-debuginfo-analyzer that referenced this issue Jan 25, 2023
@DataCorrupted @CarlosAlbertoEnciso
[Transform] Rewrite LowerSwitch using APInt
85966a5 
This rewrite fixes llvm/llvm-project#59316.

Previously LowerSwitch uses int64_t, which will crash on case branches using integers with more than 64 bits.
Using APInt fixes this problem. This patch also includes a test

Reviewed By: RKSimon

Differential Revision: https://reviews.llvm.org/D140747
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@DataCorrupted DataCorrupted

Labels
crash Prefer [crash-on-valid] or [crash-on-invalid] llvm:optimizations
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@RKSimon @EugeneZelenko @rotateright @DataCorrupted @llvmbot