LGTM.

I am not working on lld but this looks good for me. I like this solution better than the alternative you mentioned.

I think it is easier to unregister all the signal handlers in the beginning of the llvm_shutdown() since you should be done with all the work when you call llvm_shutdown(). This can also allow safe access ManagedStatics in signal handler, which I don't know if we have more of those other than the lock you are trying to fix. The only difference might be that if an interrupt is triggered when freeing ManagedStatics, llvm signal handler will not be used.

Are you planning to upstream the module pass to justify this to be part of Support library?

Thanks for working on this Jonas. Duncan and Adrian has already covered pretty much all I want to say. A small additional comments.

Address review feedback

Address review feedback

Address review feedback. Fix typos and comments.

Thanks for reviewing Erik!

LGTM. Thanks!

LGTM with some feedback inline.

I am not trying to discuss which english word is best here. My point is simply:

1. macros are evaluated during compile time
2. "host"means either the platform you compiled on during compile time or the platform you run on during the runtime
3. __is_host_* is not a good name, because it is misleading as it either implies "runtime" as a compile-time constant, or indicates the wrong platform.

It is not about matching command line name to builtin marco name. "target" is the platform we are compiling for, whether it is host or device or something else. It is a different concept when you talking about cross-compiling, which "target" is strictly not host and "build" or "host" doesn't matter to compiler at all.

I disagree. I think "target" is the correct name, even for cross compiling. For something compiled with -target foo, we are consistent calling it "target" during compile time. It only becomes appropriate to call it "host" during the runtime of the executable. There is no such concept as "host" when you are doing cross compiling.

If we all agree that dropping local_unnamed_addr is invalid, I can abandon this. Thanks for the feedback!

I might have missed something. Can you be more specific about how it is going to work?

The solution is to use canBeOmittedFromSymbolTable from include/llvm/Object/IRSymtab.h.

The symbol table is computed earlier and so the above predicate still returns 1. Your examples work with lld and ThinLTO if you want to check the implementation details.

So the original of this patch is thinLTO is promoting linkonce_odr to weak_odr. Think the following two situations:

Additional comment, I didn't realize llvm-nm now prioritize to print bitcode symbol table, rather than the symbol table from the object file. There should be an option to choose which one to see and maybe also an option to see both.

Thanks for fixing this. LGTM.

In D43959#1024157, @kubamracek wrote:

Thanks, yes, AFAIK, ld64 is fine and I don't really know how to test lld on Darwin (is it even supposed to work?).

The idea is to prevent LTO (ConstMerge pass) from merging global constants that has the same value, which has the same side-effect as linker merging two weak symbols that has ODR violation.

LGTM. And yes, order doesn't matter.

The API change is fine for ld64. But you need to do the following when you update LTO C API:

• You need to declare the interface in include/llvm-c/lto.h
• You need to bump LTO_API_VERSION in that header and document the new API is available since the new LTO_API_VERSION.
• You need to update tools/lto/lto.exports. This is the export list used by ld64 and only the interface listed in that file will be exported on darwin platform.

Michael doesn't have commit right yet so I will commit this for him.

I am using local_unnamed_addr as an example. Maybe you are right but I think my point is still valid. That is, LTO optimization and code generation can modify the symbol table from the bitcode file. It is already the case that symbols can be internalized, removed, backend can also insert new symbols. Unless there is a rule saying LTO passes cannot modify the linkage or visibility or any other attributes to the symbols, linker cannot trust whatever the bitcode symbol table says and firmly it will not be changed for some good reason.

I think this approach is much better. Can you update the title because it is not really a version bump anymore? From my understanding, this is good enough for backwards-compatibility. Thanks!

In D43361#1009616, @pcc wrote:

There is an alternative which is to teach linker to deduct auto hide property from the bitcode file and overwrite the linkage type afterwards. For C API, there is a new scope LTO_SYMBOL_SCOPE_DEFAULT_CAN_BE_HIDDEN can be used to do that but that will be forced the work onto all the linker which is probably not desirable but it solves the same problem.

This seems better to me. Ideally the linker should be reading and resolving symbols from bitcode files in the same way as regular object files, including the auto-hide bit. Linkers would need something similar for any other properties that are contained only in bitcode files but not in object files (such as a general address-taken bit).

In D43361#1009486, @pcc wrote:

The last time this was attempted (D28978) we established that the linker can do this itself. I don't think the situation has changed since then.

Generated by mistake. Abandon and I will reformat the patch.

Move the check later so it is only check when needed.

In D42267#999761, @kromanova wrote:

I am definitely not against the change. It would be good to have the number means the same thing in C/C++ API. I think ld64 can be updated in advance before we release 7.0 so we can have a larger compatibility window so I don't think this should be a blocker.

So, can I assume that I got an "OK" from the other main stakeholder who is using C LTO API? BTW, I CC-ed you to another small patch https://reviews.llvm.org/D42446, where I added a couple of missing APIs for controlling cache policy. You might want to expose it to ld64 users.

In D42267#999711, @kromanova wrote:

In D42267#999581, @steven_wu wrote:

This indeed affects how ld64 interact with libLTO. If user didn't specify a value, ld64 will always call thinlto_codegen_set_cache_pruning_interval with interval 0. This is currently a no-op so it will use whatever the default value in libLTO. With this change, it will be force pruning.

It is possible for us to make change to future ld64 to use different value but it is not going work very well for compatibility. If old ld64 loaded a new libLTO, it is going to lose incremental build.

Well, the main reason for this change was:

(a) compatibility with C++ API. A lot of code in libLTO is common for C API and C++ API. Having the same semantics for the same options in two different APIs will make code simpler and prevents from someone who only cares about C+ API to unintentionally break the C API functionality. The latter is particularly important, since incremental linking is not thoroughly tested.

(b) giving a possibility to a user to run a pruner immediately, vs. "no-op" which could get achieved simply by not passing "pruning-interval" option to the linker, making option value 0 useless.

I understand your compatibility issue, but I don't think it's very serious.

• I doubt it's common for a user to pass a option to the linker, if the same effect could be achieved by not passing any option at all (why would user do extra work?).
• This option will cause the pruner to run, which not necessarily means that the cache files will get removed and incremental build won't happen. Cache files will get removed only if at the same time the files are stale, or their sizes are larger then certain thresholds, which are passed as parameters.
• The pruner runs just 20 minutes earlier than it would have run by default. So, only the builds that are done more frequent than 20 minutes might be affected.
• Only the users who pick up the linker and libLTO from different packages will get affected.
  
  Let me know what you decide. If you want to keep things the old way, I could modify this patch for PS4 only.
  
  So, to summarize, we could add a meaningful value for pruning_interval option, which currently can't get achieved otherwise, and additionally we will offer compatibility with C++API. The only users that will be affected are the ones who because of whatever reason decided to explicitly specify a default option on the command line rather than skipping it, whose builds are happening more frequently than 20 minutes and who had mixed and matched different releases of ld64 and libLTO.

This indeed affects how ld64 interact with libLTO. If user didn't specify a value, ld64 will always call thinlto_codegen_set_cache_pruning_interval with interval 0. This is currently a no-op so it will use whatever the default value in libLTO. With this change, it will be force pruning.

Move STDC pragma handler to parser.

In D41780#968664, @efriedma wrote:

If you move all the #pragma STDC handlers from the lexer to the parser, you might be able to avoid adding an explicit STDC handler in PrintPreprocessedOutput.cpp.

Just a small suggestion. Looks good otherwise.

Other than the comment inline, LGTM.

LGTM

LGTM.

Add more tests! The new tests can trigger two errors during the compilation
but only one should be emitted.

I think I understand what you mean now. You want some cases when the
compilation doesn't failed on the first source it process.

Update testcase for CUDA driver

Improve testcase according to review feedback.

In D39502#917132, @tra wrote:

In D39502#917097, @steven_wu wrote:

Also, the reason I don't know how to craft a testcase is not because I have
trouble with CUDA driver, but how to write a test to check when did the driver
bailed out. Let me know if you have any suggestions.

Here's one way to do it: Run 'cc -v', positively match cc1 invocations that worked and CHECK-NOT for cc1 invocations that would happen after bail-out point.
Use preprocessor to induce the failure during particular compilation phase:

# if __CUDA_ARCH__ == 350
#error Error during compilation for sm_35
#endif

#ifndef __CUDA_ARCH__
#error Error during host compilation
#endif

Add testcase for CUDA driver

This seems to be the cleanest solution I can find for CUDA without
touching the job configuration for CUDA. My proposed fix is to bail
out of the jobs that are parts of CUDA offload if some command failed
before that. Now if you hit failures when compiling multiple GPU
architectures, you will error out only once. This is pretty much the exact
same behavior of current driver.

In D39502#916409, @jlebar wrote:

Note the host clang side has the inputs "test.cu" and "test-c3378c.fatbin". Which means if the device side compilation failed, the host side will not compile because InputOk will return false in this case

The patch handles this case correctly, thank you.

Unfortunately it's a bit more complicated than this. You can pass --cuda-gpu-arch=XXX multiple times when invoking clang, and that will result in multiple compilations for different GPU architectures. In this case we want to fail when the first one fails, on the theory that (most of the time) all compilations will have the same errors.

You can test this with e.g.

$ clang -x cuda test.cu -nocudalib -nocudainc --cuda-gpu-arch=sm_35 --cuda-gpu-arch=sm_60

With this patch, clang prints the errors twice, once for sm_35 and once for sm_60. Without the patch, it only prints one set of errors, which is the expected behavior.

Fix testcase. My test was passing because files left over from previous run.
Now it should work with a clean build.

Address review feedback.

More background here that doesn't fit in the commit message.

Thanks Hans. Yes, it will be good to pick this up for 5.0.1