In D66107#1666702, @Dor1s wrote:

In D66107#1666574, @Hahnfeld wrote:

I think the bots are also green, so it might be just related to how I build Clang (with libc++, for example). I'm half way through building ToT with GCC, that should give insight whether it's related to my system or my configuration.

Actually, some ARM bots got broken, so I've uploaded a fix: https://reviews.llvm.org/D67458

Just curious: So the number of new features and coverage edges does not need to be the same? I also have "15 new freatures" for CHECK2.

Yes, features include coverage edges + additional signal (e.g. value profiling), that's why the number is different and why number of features can differ between platforms.

In D66107#1666569, @Dor1s wrote:

Hm, doesn't fail for me, but I guess the feature detection might be platform-dependent to some extent, so I'm fine with replacing the number of the features with a regex. Do you want to upload a change, or should I?

The new test is failing for me because CHECK1 is not satisfied. Instead the line says MERGE-OUTER: 3 new files with 12 new features added; 11 new coverage edges (instead of 11 new features). I'm currently investigating what's wrong here, let me know if you have an idea.

LGTM

LG iff the following is correct: The leak is when dyn_cast can't cast and returns null, right? If so, please add into the summary (and do so for future revisions, it will speed up review).

The code itself looks good to me (it should probably be clang-formated). If others have comments, please raise them now.

In D67298#1662222, @serge-sans-paille wrote:

Successfully tested on my box! I'd just use

diff
 - UNSUPPORTED: asan-dynamic-runtime
 +REQUIRES: x86_64-target-arch && !android && !asan-dynamic-runtime

if that works on your side too.

For the record, I don't think this would have worked: I do have libstdc++ available on my system (I just choose SANITIZER_CXX_ABI=libcxxabi for other reasons) and passing -stdlib=libstdc++ when building the test doesn't help as I've already said in D67298. Moreover, there's nothing in the test that requires libstdc++, it just happens to pass due to many chained implications, and restricting a test without understanding the root cause is probably no good idea.

In D67298#1662107, @serge-sans-paille wrote:

And that's what I don't understand

My fault for not being clear enough. Let me elaborate the original faulty use case:

You've got an application written in C, compiled with asan, that dlopens a library written in C++, linked to libstdc++
When the application starts, asan installs a few handler based on the loaded symbols. It used to install a handler that points to itself for __cxa_throw because the symbol is not present in the binary.

I think I just figured out that the original problem is about static runtime, so it doesn't make sense to run the test with dynamic asan.

In D67298#1661672, @serge-sans-paille wrote:

Mmmmh the whole point of the test is to test how asan behaves when meeting a symbol like __cxa_throw in a dlopened library when the calling compilation unit is not linked against libstdc++. Previous behavior was a segfault, we should now get a reasonable error.

In D67298#1661672, @serge-sans-paille wrote:

Mmmmh the whole point of the test is to test how asan behaves when meeting a symbol like __cxa_throw in a dlopened library when the calling compilation unit is not linked against libstdc++. Previous behavior was a segfault, we should now get a reasonable error. By defining __cxa_throw in the c file, you remove the origin of the bug, so sure it works, but it no longer tests the faulty situation. Maybe there's a symbol caught by asan and defined in both libcxx and libstdc++?

I've posted a change that makes the test work for me in D67298, please take a look.

Also, there should be a summary of the changes in here.

In D63877#1656860, @serge-sans-paille wrote:

Is that summary correct?

It is :-)

In D63877#1656135, @serge-sans-paille wrote:

@Hahnfeld : I understand your concern. However this test case is meant to test handling of stdlib exceptions, so it doesn't look strange to disable it if stdlib is not used, right? [...]

This changes error messages, so I'd say it's not NFC.

In D63877#1641054, @serge-sans-paille wrote:

@Hahnfeld : I failed to reproduce the issue locally, but I assume the following patch may solve your issue:

--- a/compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
+++ b/compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
@@ -5,7 +5,7 @@
 //
 // CHECK: {{.*}}AddressSanitizer: failed to intercept '__cxa_{{.*}}throw{{.*}}'
 //
-// REQUIRES: x86_64-target-arch && !android
+// REQUIRES: x86_64-target-arch && !android && !cxxabi

can you give it a try?

LG, thanks for the changes.

LG

Please submit the test changes unrelated to the code changes in a separate patch!

In D65868#1625642, @jlpeyton wrote:

Unfortunately, the Windows atomic code will not compile with this change. See in kmp_atomic.h the different atomic types for Windows. The complaints are regarding assignment operations for the complex types.

../../src/kmp_atomic.cpp(1325): error: class "__kmp_cmplx32_t" has no suitable assignment operator
  ATOMIC_CMPXCHG(cmplx4, add, kmp_cmplx32, 64, +, 8c, 7,
  ^

The code changes look good to me, but the test doesn't pass on x86. We've faced the same problem when clang-offload-bundler was initially committed and the current testing is the best we were able to do.

Can we close this after D65341 landed?

Okay, so I wasn't happy with the current explanations because I don't like "fixing" an issue without understanding the problem. Here's a small reproducer:

$  cat main.cpp 
#include "test.h"

Jon, please subscribe to openmp-commits so that commit emails get through immediately. Thanks!

In D65819#1627638, @ABataev wrote:

In D65819#1627631, @Hahnfeld wrote:

In D65819#1627620, @ABataev wrote:

In D65819#1627600, @Hahnfeld wrote:

In D65819#1627036, @ABataev wrote:

In D65819#1627032, @Hahnfeld wrote:

In D65819#1617736, @Hahnfeld wrote:

Will this patch change the ability to consume a bundled object file without calling the unbundler? Using known ELF tools on the produced object files was an important design decision and IIRC was somewhat important for using build systems that are unaware of the bundled format.

Ping.

Missed this. We still produce correct object files, so all the tools will work with this.

I agree on a technical level that it's still a "correct" object, but not what I was looking for: The host object file will only be in the bundled section, so you cannot examine it without unbundling.

For example, with a small test file and clang -fopenmp -fopenmp-targets=x86_64 -c test.c I saw the following:

$ nm test.o                                       
0000000000000000 t .omp_offloading.requires_reg
0000000000000000 T test
                 U __tgt_register_requires

After applying this patch, the output is empty which might be a problem in certain cases.

Unfortunately, this is the only possible solution I see. There are already 2 reports that bundled objects does not work correctly after unbundling.

Can you please again share what exactly is the problem, with a small example? I saw discussions on openmp-dev, but that project was huge, and above you were quoting a man page and hinted to global constructors.

I don't have a small reproducer, unfortunately, only the big one.
Here is the message from the user:

Hi,
I am revisiting this possible compiler bug in Clang 8.0.0.

In the source code I am developing, there's a global static variable, nest::sli_neuron::recordablesMap_ put in the BSS section and it is expected to be fully initialized by the time nest::sli_neuron::sli_neuron() gets called, however in a gdb session:

(gdb) p nest::sli_neuron::recordablesMap_
Python Exception <type 'exceptions.AttributeError'> 'gdb.Type' object has no attribute 'name':
$1 = {<std::map<Name, double (nest::sli_neuron::*)() const, std::less<Name>, std::allocator<std::pair<Name const, double (nest::sli_neuron::*)() const> > >> = std::map with 0 elements, _vptr$RecordablesMap = 0x0}

this doesn't happen when -fopenmp-targets is _not_ used, is it not trivial to come up a reproducer, thus I am sending a work-in-progress report hoping someone will shed some light on this.

Thanks,
Itaru.

Another one:

Hi,
I am seeing a link error shown below:

`.text.startup' referenced in section `.init_array.0' of /tmp/event_delivery_manager-02f392.o: defined in discarded section `.text.startup[_ZN4nest18DataSecondaryEventIdNS_16GapJunctionEventEE18supported_syn_ids_E]' of /tmp/event_delivery_manager-02f392.o
clang-10: error: linker command failed with exit code 1 (use -v to see invocation)

I am not sure how to tackle this as the part is referenced isn't what I am
working on. I am using the latest trunk Clang 10 at this moment.

Steps to reproduce:

Steps to produce:
$ git clone https://https://github.com/nest/nest-simulator/
$ mkdir /tmp/build/nest-clang-offload/
$ cd /tmp/build/nest-clang-offload/
$ cmake -DCMAKE_TOOLCHAIN_FILE=Platform/JURON_Clang -DCMAKE_INSTALL_PREFIX=/path/to/opt/nest-clang -Dwith-gsl=ON -Dwith-openmp=ON -Dwith-mpi=OFF -Dwith-python=OFF -Dwith-offload=ON /path/to/nest-simulator/

In D65819#1627620, @ABataev wrote:

In D65819#1627600, @Hahnfeld wrote:

In D65819#1627036, @ABataev wrote:

In D65819#1627032, @Hahnfeld wrote:

In D65819#1617736, @Hahnfeld wrote:

Will this patch change the ability to consume a bundled object file without calling the unbundler? Using known ELF tools on the produced object files was an important design decision and IIRC was somewhat important for using build systems that are unaware of the bundled format.

Ping.

Missed this. We still produce correct object files, so all the tools will work with this.

I agree on a technical level that it's still a "correct" object, but not what I was looking for: The host object file will only be in the bundled section, so you cannot examine it without unbundling.

For example, with a small test file and clang -fopenmp -fopenmp-targets=x86_64 -c test.c I saw the following:

$ nm test.o                                       
0000000000000000 t .omp_offloading.requires_reg
0000000000000000 T test
                 U __tgt_register_requires

After applying this patch, the output is empty which might be a problem in certain cases.

Unfortunately, this is the only possible solution I see. There are already 2 reports that bundled objects does not work correctly after unbundling.

In D65819#1627036, @ABataev wrote:

In D65819#1627032, @Hahnfeld wrote:

In D65819#1617736, @Hahnfeld wrote:

Will this patch change the ability to consume a bundled object file without calling the unbundler? Using known ELF tools on the produced object files was an important design decision and IIRC was somewhat important for using build systems that are unaware of the bundled format.

Ping.

Missed this. We still produce correct object files, so all the tools will work with this.

Rebase.

Change check_c_compiler_flag() to check_cxx_compiler_flag().

In D65819#1617736, @Hahnfeld wrote:

Will this patch change the ability to consume a bundled object file without calling the unbundler? Using known ELF tools on the produced object files was an important design decision and IIRC was somewhat important for using build systems that are unaware of the bundled format.

Remove compiler check for -Wno-uninitialized.

Remove another unnecessary cast.

In D63877#1624433, @serge-sans-paille wrote:

ok, it's probably a regex issue for the test, I'll check that.

In D63877#1624224, @Hahnfeld wrote:

This test is also failing for me on CentOS 7, Intel x86. Any clues what's wrong here?

This test is also failing for me on CentOS 7, Intel x86. Any clues what's wrong here?

LG after addressing the last minor nits (comment + cast + check).

Can you please rebase on top of D66019? Sorry, that'll probably give you some conflicts :-/

In D65340#1623236, @gtbercea wrote:

@Hahnfeld
I would like to clarify something about tgt_register_requires call.

This call is special and in a compiler that supports handling of requires clauses (like unified_shared_memory clause) ( i.e. Clang 9.0 onwards), this function is called before any main function code. Here is a printout with LIBOMPTARGET_DEBUG=1 to realize the difference:

===== WHERE COMPILER PLACES CALL TO __tgt_register_requires() ========
Libomptarget --> Loading RTLs... <<<< THIS IS THE VERY FIRST PRINT FROM THE RUNTIME. The call to __tgt_register_requires is just before that.
Libomptarget --> Loading library 'libomptarget.rtl.ppc64.so'...
Libomptarget --> Successfully loaded library 'libomptarget.rtl.ppc64.so'!
Libomptarget --> Registering RTL libomptarget.rtl.ppc64.so supporting 4 devices!
Libomptarget --> Loading library 'libomptarget.rtl.x86_64.so'...
Libomptarget --> Unable to load library 'libomptarget.rtl.x86_64.so': libomptarget.rtl.x86_64.so: cannot open shared object file: No such file or directory!
Libomptarget --> Loading library 'libomptarget.rtl.cuda.so'...
Target CUDA RTL --> Start initializing CUDA
Libomptarget --> Successfully loaded library 'libomptarget.rtl.cuda.so'!
Libomptarget --> Registering RTL libomptarget.rtl.cuda.so supporting 4 devices!
Libomptarget --> Loading library 'libomptarget.rtl.aarch64.so'...
Libomptarget --> Unable to load library 'libomptarget.rtl.aarch64.so': libomptarget.rtl.aarch64.so: cannot open shared object file: No such file or directory!
Libomptarget --> RTLs loaded!
Libomptarget --> Image 0x0000000010020080 is compatible with RTL libomptarget.rtl.ppc64.so!
Libomptarget --> RTL 0x000000004747b3e0 has index 0!
Libomptarget --> Registering image 0x0000000010020080 with RTL libomptarget.rtl.ppc64.so!
Libomptarget --> Done registering entries!
===== MANUAL CALL TO __tgt_register_requires() IN MAIN (top of MAIN function!) ========

Manually calling __tgt_register_requires can never actually work correctly because any user space call to this function will be after the binary has been loaded and the flags already decided. The compiler just has to support the handling of the requires clauses.

Thanks, that's even easier than a pointer to NULL :)

Can you detail what "incorrect linking" means? AFAIK the additional sections were just bloating the executable, but how do they affect correctness?

In D65340#1616943, @gtbercea wrote:

In D65340#1616793, @Hahnfeld wrote:

The test will currently fail with older versions of Clang. It must at least be marked UNSUPPORTED for Clang versions older than what-will-be Clang 10.

Additionally, I'd still like to see a small test with manual calls to __tgt_target_data_begin / _end if possible. I'm thinking of the following:

int *a = // malloc
int *device;

__tgt_target_data_begin(...); // with close modifier for a
#pragma omp target data use_device_ptr(a)
{
  device = a;
}
__tgt_target_data_end(...); // correspondingly to the begin call above

// check...

Instead of the nested target data region, you could also use a simple target region in the outer data region, and have a check for use_device_ptr in another test.

I don't mind adding such a test but calling these functions manually is very tedious and error prone and hard to maintain. Are there any benefits I'm missing to this apart from running this test in isolation?

The test will currently fail with older versions of Clang. It must at least be marked UNSUPPORTED for Clang versions older than what-will-be Clang 10.

Apologies, but I think my fix for -Wcast-qual is wrong and the code is still broken. Luckily, the latest Intel Compiler still warns about it which forced me to rethink what the pointers try to achieve.

I'd propose to mark the test close_modifier.c as unsupported with older compiler versions. Additionally, can you add a small, new test with manual calls to __tgt_target_data_begin / _end without relying on the compiler?

Oh, but the tests again look add. Please run them through clang-format.

I'm fine with the last update.

I think this still needs some work.

There's still no call to __tgt_register_requires in the two tests, so I guess they won't pass with older versions of Clang.

In D65001#1607199, @gtbercea wrote:

@Hahnfeld I cannot replace the usage of the requires pragma with the call to the registration function directly because the registration function is the first function that gets called so if I explicitely invoke it will only be the 2nd call to that function. A subsequent call with a different set of flags will lead to a mismatch in requires clauses error. (first implicit call is without unified_shared_memory and the second call is with unified_shared_memory).