This is not a linker specific thing - the library name that we are passing is misnamed - the parameter should be -l <library name> which will become lib <library name> [.so` | .a]. If we want to guarantee the static library, using -static, -nostatic around the library is more appropriate.

I think I must have had something else in my tree, cause a clean build does build them.

I do like @rnk's suggestion for the emission of the error, but Im not really tied to it - if it triggers, the TLI for the target is not setup properly or is missing the symbol, in which case, it needs to be addressed anyways. Beyond that, assuming that @rnk is okay with this, LGTM. Thanks for fixing this.

I’m not sure that this is the best direction long term. This is specific to the LLVM build, the runtimes builds do not take these parameters, and they are the primary things we’re trying to cross compile with this infrastructure. But the monorepo makes this more complicated and makes the configure the same, so, yes, I suppose this needs to be preserved. *sigh*

I wish that we had a better type. For the time being, the 64-bit with is sufficient for current needs.

I do believe that we need it for all the targets (x86, x86_64, armv7, aarch64).

Thanks, this should be more robust. Nice catch with the image base being explicitly set and using p_vaddr rather than p_offset, that was something that I failed to account for. I really wish that we had a more complete test suite, but that is *way* beyond this change.

I don't think that generating a file of that size makes sense for the test case. Generating a small file and overwriting the file size would be viable, but in this case, we need to use the file size itself, so that doesn't work either.

I feel like Im not understanding something. The point of CMake is that it tracks dependencies. We are we manually tracking dependencies?

*sigh* I thought it used the size of the function to mark the end of the region. Thanks.

Thanks for cleaning this up

I think that using assembly for the test is fine

@rupprecht as much as I really want to agree with you in that this isn't needed to match the behaviour of the system's libm, the problem is that __builtin_logb can be misused to implement a library. So, really, I would really urge Apple to consider maintaining similarity across all the platforms for the builtins - there is no official guarantee that is made on the behaviour of the builtin operations, so If there is a strong reason to have the divergence, please do share that, and I think that we can probably accommodate that with the small tweak that @rupprecht points out:

Honestly, this is just setting up the register context for ARM64. I dont think that there is much of a test for this. I mean, I suppose you could test this by instantiating the context and trying to read it through the interface. But, I question the value of such a test. Whether you go with the in-process or out-of-process approach and whether you are doing DWARF of CodeView debugging this is going to be needed. As to running the test suite - you can cross-compile and run the tests remotely.

r372667

I think that printf is quite an amazing notification :-)

What do you think of adding some sort of notification that hardware breakpoints are currently unsupported and that we are falling back to software breakpoints for the #else case?

Nit: triple, not triplet.

Please post patches with context.

SVN r372427

SVN r372407

Sure, this seems reasonable.

Okay, both @mtrent and @alexshap bring up good points. I *suppose* that is okay, but I shall string grumble. @mtrent, I think we could also simply make getObjectForArch be internal to the type, and force users to go down either getMachOArchiveForArch or getMachOObjectForArch.

Would be nice if we could have a test case for this.

@yifeihe, you should be able to build with backtraces disabled to get the previous behaviour. The fact that the behaviour was a certain way previously was just an accident, not an explicit design point.

@beanz - I must be mistaken about something. The condition is if LLVM_ENABLE_RUNTIMES is NOT specified for the current target, we pass in the global value (which was previously passed via the PASSTHROUGH_PREFIXES), and if it is specified, we will pass in the specified value. So, the only behaviour change that is visible is if the user specified a target specific LLVM_ENABLE_RUNTIMES value it is now honoured.

@beanz - I can see where you are coming from there. But, the thing is that when you are building for a wide variety of targets at once, that doesn't make sense since not all the runtimes are compatible with all targets (e.g. unwind doesn't support Windows). Calculating the set difference and passing that along seems more complicated, though plausible. Note that this approach preserves the existing behaviour - unless you *explicitly* state the runtimes for a specific target, the LLVM_ENABLE_RUNTIMES passed globally will continue to be passed through, so from a user's perspective nothing has changed (and that is intentional - no one should need to update their CMake caches for this change).

I think that controlling via annotations is actually very useful. It will actually still be needed to ensure that libc++ continues to build on Windows properly (which really does want the annotations). The annotations being applied there makes it easy to then handle properly on the other platforms as well. Additionally, diffs explicitly call out the interface in the change, which again helps review the changes. Finally, there are some symbols which are synthesized by the compiler and the visibility for those are controlled by means of the annotation on the associated class. This again is far easier to manage with the explicit annotation rather than the explicit export list.

This is not a NFCI change - it is adding new functionality. Does it make sense perhaps to have this be a separate pass?

I think that this is pretty easy to forget. Fortunately, last argument wins. Why not sink this into the %clang substitution in lit? That ensures that we run with an empty sysroot and then when the test needs to adjust the sysroot, it can do so explicitly.

Please rename the tests to be more meaningful so that others can possibly identify what they are meant to test and can address things which may break when other changes are made.

I think it may make some sense to strip the user label prefix from the symbols that are listed in the interface, and then add that in when generating the ELF/TBD.

I suppose that the test cases that I think are valuable:

• 1 file
• 1+ file
• functions
• objects
• functions w/ different alignments
• objects w/ different alignments
• functions w/ different sizes
• objects w/ different sizes
• functions w/ different alignments, sizes
• objects w/ different alignments, sizes
• weak symbols
• mismatch (error cases)

Probably is worth having a test for the error case as well.

This definitely needs tests.

SVN r369788

Im not sure if this works. This sets the flags from the global flags, which may not be what we want as compiler-rt can be built as part of llvm itself.

Please add a diagnostic to indicate that the provided value is invalid for the argument (or at at least extend the existing the tests to ensure that the old values are diagnosed in the driver rather hitting the assert in the frontend.

Please add a test case for read/write, otherwise LGTM.

SVN r369307

Python eggs may be another option which might be an easier way forward.

At least for the from-source distribution, where the destination of the image is the build host, Python_EXECUTABLE is definitely preferable (e.g. #!@Python_EXECUTABLE@). Furthermore, there is no reason to assume that /usr/bin/env python3 will give you the same python 3 as what was given to cmake (you can control that with -DPython_EXECUTABLE=...). I think that requiring the user to specify the tool is possibly the best option - that is completely remove the shebang as that ensures that shifts the burden of selection to the user. Another option may be to use something like p2exe and create self-contained tools.

Ugh, this is still not the most structured handling of the arguments. But, yeah, this seems like it should be equivalent. Fine by me if @aaron.ballman has no more comments.

Tests?

Seems like it should be equivalent.

Hmm, yes, it is for use from the build tree. I don't see how INTERFACE_INCLUDE_DIRECTORIES can do a -include though. It has to be INTERFACE_COMPILE_OPTIONS or INTERFACE_COMPILE_DEFINITIONS.