This is an archive of the discontinued LLVM Phabricator instance.

[Libomptarget] Fix external visibility for internal variables
ClosedPublic

Authored by jhuber6 on Jan 17 2022, 4:57 PM.

Details

Summary

After the changes in D117362 made variables declared inside of a target
declare directive visible outside the plugin, some variables inside the
runtime were given visiblity that conflicted with their address space
type. This caused problems when shared or local memory was made
externally visible. This patch fixes this issue by making these
varialbes static within the module, therefore limiting their visibility
to being internal.

Diff Detail

Event Timeline

jhuber6 requested review of this revision.Jan 17 2022, 4:57 PM
jhuber6 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2022, 4:57 PM
jhuber6 updated this revision to Diff 400694.Jan 17 2022, 7:30 PM

Add visibility attribute to local data.

jdoerfert accepted this revision.Jan 18 2022, 7:53 AM

LG, even though we need to get rid of the non THREAD_LOCAL globals asap.

This revision is now accepted and ready to land.Jan 18 2022, 7:53 AM

Hi Joseph,
3 commits came in, one bot exited with a fail
https://lab.llvm.org/buildbot/#/builders/193/builds/4828

Hi Joseph,
3 commits came in, one bot exited with a fail
https://lab.llvm.org/buildbot/#/builders/193/builds/4828

Is it possible that AMDGPU doesn't support static on these variables? I tested https://reviews.llvm.org/D117362 on my AMDGPU machine and didn't notice any problems, but didn't test with this one applies. I can test locally but it'll take awhile to rebuild on AMDGPU.

Hi Joseph,
3 commits came in, one bot exited with a fail
https://lab.llvm.org/buildbot/#/builders/193/builds/4828

I get a crash compiling the shared object, can you confirm that on your end? I'm not sure what this would've changed that would break it.

i will give it a try locally ...

i dont see the failures locally, so i am now getting on the buildbot system to see what happens

i will give it a try locally ...

The issue I got locally occurs without my patches so I'm assuming it's not related, running this
clang dynamic_module.c -fopenmp -fopenmp-targets=amdgcn -fPIC -shared -o out.so && clang out.so -fopenmp -fopenmp-targets=amdgcn
Gives me the following crash upstream,

clang: /ccs/home/jhuber/llvm/ref/llvm-project/llvm/include/llvm/ADT/SmallVector.h:273: T& llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::operator[](llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::size_type) [with T = clang::driver::InputInfo; <template-parameter-1-2> = void; llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::reference = clang::driver::InputInfo&; llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::size_type = long unsigned int]: Assertion `idx < size()' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: clang out.so -fopenmp -fopenmp-targets=amdgcn
1.	Compilation construction
2.	Building compilation jobs
3.	Building compilation jobs
4.	Building compilation jobs
5.	Building compilation jobs
6.	Building compilation jobs
7.	Building compilation jobs
 #0 0x00007f1cd13d282f PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
 #1 0x00007f1cd13d00d9 SignalHandler(int) Signals.cpp:0:0
 #2 0x00007f1cd8a632d0 __restore_rt (/lib64/libpthread.so.0+0x132d0)
 #3 0x00007f1cd054c420 raise (/lib64/libc.so.6+0x39420)
 #4 0x00007f1cd054da01 abort (/lib64/libc.so.6+0x3aa01)
 #5 0x00007f1cd0544a1a __assert_fail_base (/lib64/libc.so.6+0x31a1a)
 #6 0x00007f1cd0544a92 (/lib64/libc.so.6+0x31a92)
 #7 0x00007f1cd4f41c8d clang::driver::Driver::BuildJobsForActionNoCache(clang::driver::Compilation&, clang::driver::Action const*, clang::driver::ToolChain const*, llvm::StringRef, bool, bool, char const*, std::map<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, clang::driver::InputInfo, std::less<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const, clang::driver::InputInfo> > >&, clang::driver::Action::OffloadKind) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0xa0c8d)
 #8 0x00007f1cd4f41ec2 clang::driver::Driver::BuildJobsForAction(clang::driver::Compilation&, clang::driver::Action const*, clang::driver::ToolChain const*, llvm::StringRef, bool, bool, char const*, std::map<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, clang::driver::InputInfo, std::less<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const, clang::driver::InputInfo> > >&, clang::driver::Action::OffloadKind) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0xa0ec2)
 #9 0x00007f1cd4f42366 void llvm::function_ref<void (clang::driver::Action*, clang::driver::ToolChain const*, char const*)>::callback_fn<clang::driver::Driver::BuildJobsForActionNoCache(clang::driver::Compilation&, clang::driver::Action const*, clang::driver::ToolChain const*, llvm::StringRef, bool, bool, char const*, std::map<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, clang::driver::InputInfo, std::less<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const, clang::driver::InputInfo> > >&, clang::driver::Action::OffloadKind) const::'lambda'(clang::driver::Action*, clang::driver::ToolChain const*, char const*)>(long, clang::driver::Action*, clang::driver::ToolChain const*, char const*) Driver.cpp:0:0
#10 0x00007f1cd4f176fc clang::driver::OffloadAction::doOnEachDeviceDependence(llvm::function_ref<void (clang::driver::Action*, clang::driver::ToolChain const*, char const*)> const&) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0x766fc)
#11 0x00007f1cd4f3f2fb clang::driver::Driver::BuildJobsForActionNoCache(clang::driver::Compilation&, clang::driver::Action const*, clang::driver::ToolChain const*, llvm::StringRef, bool, bool, char const*, std::map<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, clang::driver::InputInfo, std::less<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const, clang::driver::InputInfo> > >&, clang::driver::Action::OffloadKind) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0x9e2fb)
#12 0x00007f1cd4f41ec2 clang::driver::Driver::BuildJobsForAction(clang::driver::Compilation&, clang::driver::Action const*, clang::driver::ToolChain const*, llvm::StringRef, bool, bool, char const*, std::map<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, clang::driver::InputInfo, std::less<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const, clang::driver::InputInfo> > >&, clang::driver::Action::OffloadKind) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0xa0ec2)
#13 0x00007f1cd4f40682 clang::driver::Driver::BuildJobsForActionNoCache(clang::driver::Compilation&, clang::driver::Action const*, clang::driver::ToolChain const*, llvm::StringRef, bool, bool, char const*, std::map<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, clang::driver::InputInfo, std::less<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const, clang::driver::InputInfo> > >&, clang::driver::Action::OffloadKind) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0x9f682)
#14 0x00007f1cd4f41ec2 clang::driver::Driver::BuildJobsForAction(clang::driver::Compilation&, clang::driver::Action const*, clang::driver::ToolChain const*, llvm::StringRef, bool, bool, char const*, std::map<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, clang::driver::InputInfo, std::less<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const, clang::driver::InputInfo> > >&, clang::driver::Action::OffloadKind) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0xa0ec2)
#15 0x00007f1cd4f40682 clang::driver::Driver::BuildJobsForActionNoCache(clang::driver::Compilation&, clang::driver::Action const*, clang::driver::ToolChain const*, llvm::StringRef, bool, bool, char const*, std::map<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, clang::driver::InputInfo, std::less<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const, clang::driver::InputInfo> > >&, clang::driver::Action::OffloadKind) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0x9f682)
#16 0x00007f1cd4f41ec2 clang::driver::Driver::BuildJobsForAction(clang::driver::Compilation&, clang::driver::Action const*, clang::driver::ToolChain const*, llvm::StringRef, bool, bool, char const*, std::map<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, clang::driver::InputInfo, std::less<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const, clang::driver::InputInfo> > >&, clang::driver::Action::OffloadKind) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0xa0ec2)
#17 0x00007f1cd4f3fed1 clang::driver::Driver::BuildJobsForActionNoCache(clang::driver::Compilation&, clang::driver::Action const*, clang::driver::ToolChain const*, llvm::StringRef, bool, bool, char const*, std::map<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, clang::driver::InputInfo, std::less<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const, clang::driver::InputInfo> > >&, clang::driver::Action::OffloadKind) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0x9eed1)
#18 0x00007f1cd4f41ec2 clang::driver::Driver::BuildJobsForAction(clang::driver::Compilation&, clang::driver::Action const*, clang::driver::ToolChain const*, llvm::StringRef, bool, bool, char const*, std::map<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, clang::driver::InputInfo, std::less<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::pair<clang::driver::Action const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const, clang::driver::InputInfo> > >&, clang::driver::Action::OffloadKind) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0xa0ec2)
#19 0x00007f1cd4f4265b clang::driver::Driver::BuildJobs(clang::driver::Compilation&) const (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0xa165b)
#20 0x00007f1cd4f43e03 clang::driver::Driver::BuildCompilation(llvm::ArrayRef<char const*>) (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/lib/libclangDriver.so.14git+0xa2e03)
#21 0x000000000040e4b7 main (/autofs/nccs-svm1_home1/jhuber/llvm/ref/clang/bin/clang-14+0x40e4b7)
#22 0x00007f1cd053734a __libc_start_main (/lib64/libc.so.6+0x2434a)
#23 0x000000000041079a _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:122:0
[1]    185031 abort      clang out.so -fopenmp -fopenmp-targets=amdgcn

This doesn't happen using my new driver also.

Hi Joseph,
3 commits came in, one bot exited with a fail
https://lab.llvm.org/buildbot/#/builders/193/builds/4828

Is it possible that AMDGPU doesn't support static on these variables? I tested https://reviews.llvm.org/D117362 on my AMDGPU machine and didn't notice any problems, but didn't test with this one applies. I can test locally but it'll take awhile to rebuild on AMDGPU.

Previously, static shared variables did not work. Seems plausible that static thread local variables won't work either. Address space 5 roughly means __private, i.e. on the stack alloca style. I haven't worked through what this variable is used for yet.

openmp/libomptarget/DeviceRTL/include/Types.h
195

Why would we want a visibility tag on stack variables? They shouldn't be in the elf at all