This is an archive of the discontinued LLVM Phabricator instance.

[Alignment][NFC] Use proper getter to retrieve alignment from ConstantInt and ConstantSDNode
ClosedPublic

Authored by gchatelet on Jul 2 2020, 1:40 PM.

Details

Summary

This patch is part of a series to introduce an Alignment type.
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html
See this patch for the introduction of the type: https://reviews.llvm.org/D64790

Diff Detail

Event Timeline

gchatelet created this revision.Jul 2 2020, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2020, 1:40 PM
courbet accepted this revision.Jul 3 2020, 12:34 AM
This revision is now accepted and ready to land.Jul 3 2020, 12:34 AM
This revision was automatically updated to reflect the committed changes.
sbc100 added a subscriber: sbc100.Jul 5 2020, 2:28 PM

This change seems to have broken one of our tests in emscripten. I reduced it to this:

#include <stdlib.h>
#include <assert.h>

int main() {
  void * ptr = aligned_alloc(3, 64);
  assert(ptr == NULL);
  return 0;
}
$ clang -O2 -c test.c
clang: /usr/local/google/home/sbc/dev/wasm/llvm-project/llvm/include/llvm/Support/Alignment.h:138: llvm::MaybeAlign::MaybeAlign(uint64_t): Assertion `(Value == 0 || llvm::isPowerOf2_64(Value)) && "Alignment is neither 0 nor a power of 2"' 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: ../llvm-build/bin/clang -O2 -c test.c 
1.	<eof> parser at end of file
2.	Per-module optimization passes
3.	Running pass 'Function Pass Manager' on module 'test.c'.
4.	Running pass 'Combine redundant instructions' on function '@main'
 #0 0x00007f437edf9a14 PrintStackTraceSignalHandler(void*) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMSupport.so.11git+0x199a14)
 #1 0x00007f437edf760e llvm::sys::RunSignalHandlers() (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMSupport.so.11git+0x19760e)
 #2 0x00007f437edf8bbd llvm::sys::CleanupOnSignal(unsigned long) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMSupport.so.11git+0x198bbd)
 #3 0x00007f437ed1fc23 (anonymous namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMSupport.so.11git+0xbfc23)
 #4 0x00007f437ed1fd5c CrashRecoverySignalHandler(int) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMSupport.so.11git+0xbfd5c)
 #5 0x00007f4382d87110 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14110)
 #6 0x00007f437e7ac761 raise /build/glibc-M65Gwz/glibc-2.30/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #7 0x00007f437e79655b abort /build/glibc-M65Gwz/glibc-2.30/stdlib/abort.c:81:7
 #8 0x00007f437e79642f get_sysdep_segment_value /build/glibc-M65Gwz/glibc-2.30/intl/loadmsgcat.c:509:8
 #9 0x00007f437e79642f _nl_load_domain /build/glibc-M65Gwz/glibc-2.30/intl/loadmsgcat.c:970:34
#10 0x00007f437e7a5092 (/lib/x86_64-linux-gnu/libc.so.6+0x34092)
#11 0x00007f4380a9ffdf llvm::InstCombiner::visitCallBase(llvm::CallBase&) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMInstCombine.so.11git+0x7dfdf)
#12 0x00007f4380a91542 llvm::InstCombiner::visitCallInst(llvm::CallInst&) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMInstCombine.so.11git+0x6f542)
#13 0x00007f4380a4db88 llvm::InstCombiner::run() (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMInstCombine.so.11git+0x2bb88)
#14 0x00007f4380a500a7 combineInstructionsOverFunction(llvm::Function&, llvm::InstCombineWorklist&, llvm::AAResults*, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::DominatorTree&, llvm::OptimizationRemarkEmitter&, llvm::BlockFrequencyInfo*, llvm::ProfileSummaryInfo*, unsigned int, llvm::LoopInfo*) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMInstCombine.so.11git+0x2e0a7)
#15 0x00007f4380a51917 llvm::InstructionCombiningPass::runOnFunction(llvm::Function&) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMInstCombine.so.11git+0x2f917)
#16 0x00007f437f187894 llvm::FPPassManager::runOnFunction(llvm::Function&) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMCore.so.11git+0x205894)
#17 0x00007f437f187b98 llvm::FPPassManager::runOnModule(llvm::Module&) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMCore.so.11git+0x205b98)
#18 0x00007f437f1882c4 llvm::legacy::PassManagerImpl::run(llvm::Module&) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMCore.so.11git+0x2062c4)
#19 0x00007f4381d70b67 clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libclangCodeGen.so.11git+0xcdb67)
#20 0x00007f438208aa06 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libclangCodeGen.so.11git+0x3e7a06)
#21 0x00007f437cbc6653 clang::ParseAST(clang::Sema&, bool, bool) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/../lib/libclangParse.so.11git+0x35653)
#22 0x00007f438094e653 clang::FrontendAction::Execute() (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libclangFrontend.so.11git+0x105653)
#23 0x00007f43808de1f3 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libclangFrontend.so.11git+0x951f3)
#24 0x00007f4382d6cf32 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libclangFrontendTool.so.11git+0x3f32)
#25 0x000000000041569a cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (../llvm-build/bin/clang+0x41569a)
#26 0x000000000041370c ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) (../llvm-build/bin/clang+0x41370c)
#27 0x00007f43806967d2 void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const::$_1>(long) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libclangDriver.so.11git+0x997d2)
#28 0x00007f437ed1fb41 llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libLLVMSupport.so.11git+0xbfb41)
#29 0x00007f4380695c7d clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libclangDriver.so.11git+0x98c7d)
#30 0x00007f4380662d8b clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&) const (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libclangDriver.so.11git+0x65d8b)
#31 0x00007f4380663177 clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) const (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libclangDriver.so.11git+0x66177)
#32 0x00007f438067c788 clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) (/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/../lib/libclangDriver.so.11git+0x7f788)
#33 0x0000000000413003 main (../llvm-build/bin/clang+0x413003)
#34 0x00007f437e797e0b __libc_start_main /build/glibc-M65Gwz/glibc-2.30/csu/../csu/libc-start.c:342:3
#35 0x000000000041044a _start (../llvm-build/bin/clang+0x41044a)
clang-11: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 11.0.0 (https://github.com/llvm/llvm-project.git 87e2751cf07cc570200d8d356d7f35fee2807779)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/local/google/home/sbc/dev/wasm/emscripten/../llvm-build/bin
clang-11: note: diagnostic msg: 
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang-11: note: diagnostic msg: /tmp/test-0a8871.c
clang-11: note: diagnostic msg: /tmp/test-0a8871.sh
clang-11: note: diagnostic msg: 

********************
jrtc27 added a comment.EditedJul 5 2020, 3:37 PM

This change seems to have broken one of our tests in emscripten. I reduced it to this:

#include <stdlib.h>
#include <assert.h>

int main() {
  void * ptr = aligned_alloc(3, 64);
  assert(ptr == NULL);
  return 0;
}

Yeah, this is testing DR 460's new (and accepted) requirements (previously it was undefined):

If the value of alignment is not a valid alignment supported by the implementation or the value of size is not an integral multiple of alignment the function shall fail by returning a null pointer.

I feel this should probably be handled specially (ie use align(1) or no alignment) in the Clang frontend? Though I wouldn't like to say how things change when you change it to:

int align = 3;
void *ptr = aligned_alloc(align, 64);

For sufficiently obfuscated code I can imagine Clang not noticing but a later LLVM pass discovering the alignment is a constant and trying to set the attributes up as such.

sbc100 added a comment.Jul 5 2020, 4:39 PM

This change seems to have broken one of our tests in emscripten. I reduced it to this:

#include <stdlib.h>
#include <assert.h>

int main() {
  void * ptr = aligned_alloc(3, 64);
  assert(ptr == NULL);
  return 0;
}

Yeah, this is testing DR 460's new (and accepted) requirements (previously it was undefined):

If the value of alignment is not a valid alignment supported by the implementation or the value of size is not an integral multiple of alignment the function shall fail by returning a null pointer.

Regardless of weather its defined or undefined behaviour I wouldn't expect the compiler to crash though right? Clearly this is a compiler bug, right?

I feel this should probably be handled specially (ie use align(1) or no alignment) in the Clang frontend? Though I wouldn't like to say how things change when you change it to:

int align = 3;
void *ptr = aligned_alloc(align, 64);

Same crash happens with this code.

For sufficiently obfuscated code I can imagine Clang not noticing but a later LLVM pass discovering the alignment is a constant and trying to set the attributes up as such.

jrtc27 added a comment.Jul 5 2020, 5:11 PM

This change seems to have broken one of our tests in emscripten. I reduced it to this:

#include <stdlib.h>
#include <assert.h>

int main() {
  void * ptr = aligned_alloc(3, 64);
  assert(ptr == NULL);
  return 0;
}

Yeah, this is testing DR 460's new (and accepted) requirements (previously it was undefined):

If the value of alignment is not a valid alignment supported by the implementation or the value of size is not an integral multiple of alignment the function shall fail by returning a null pointer.

Regardless of weather its defined or undefined behaviour I wouldn't expect the compiler to crash though right? Clearly this is a compiler bug, right?

Oh, definitely, I wasn't intending to suggest otherwise.

I feel this should probably be handled specially (ie use align(1) or no alignment) in the Clang frontend? Though I wouldn't like to say how things change when you change it to:

int align = 3;
void *ptr = aligned_alloc(align, 64);

Same crash happens with this code.

Sure. My point is that if you sufficiently obfuscate it, Clang might not notice what's going on so even if Clang caught these cases, you might have issues with more complex ones later in the pipeline. Though maybe LLVM passes won't move values later determined to be constants into new align attributes.

sbc100 added a comment.Jul 5 2020, 5:45 PM

Yup, it doesn't happen if I remove the ability for clang to determine the actual value. e.g: this version does not cause the crash:

#include <stdlib.h>
#include <assert.h>

int align = 3;

int main() {
  void * ptr = aligned_alloc(align, 64);
  assert(ptr == NULL);
  return 0;
}

I filed a bug for this issue at https://bugs.llvm.org/show_bug.cgi?id=46594 (although I probably wouldn't have bothered if I had refreshed the page and seen @sbc100's comments first. Oops!)

Thank you for reporting the issue and providing a reduced test case.
I'm working on a fix.