Page MenuHomePhabricator

smeenai (Shoaib Meenai)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 19 2016, 10:21 AM (217 w, 5 d)

Recent Activity

Yesterday

smeenai added a comment to D89561: [MC] Adjust StringTableBuilder for linked Mach-O binaries.

@mtrent, do you have the historical context for why ld64 adds a leading space to the string table?

Yes.

The actual details are locked in an office in a building I don't have access to because of global pandemic. But I can give you the gist with citations. I'm also not the right person to ask, but I'm the person most likely to answer this question, so, hey, maybe I am!

However, the Mach-O documentation says:

A union that holds an index into the string table, n_strx. To specify an empty string (""), set this value to 0.

The Mach-O file format descends in part from the UNIX a.out file format. Both (the new defunct) ld and ld64 agree on the historical definition of n_strx as defined in the historical a.out(5) manpage. I believe the original BSD 4.2, 4.3, 4.4 man pages had this definition, and here is where I have to get a little hand-wavy. But this link from the Internet has text that matches my memory of that historical man content (and note that this differs from newer versions of the a.out man page):

http://man.cat-v.org/unix_8th/5/a.out

In the a.out file a symbol's n_un.n_strx field gives an
index into the string table.  A n_strx value of 0 indicates
that no name is associated with a particular symbol table
entry.  The field n_un.n_name can be used to refer to the
symbol name only if the program sets this up using n_strx
and appropriate data from the string table.

Let me call your attention to this sentence:

A n_strx value of 0 indicates that no name is associated with a particular symbol table entry.

A strict reading of this sentence is: If you encounter a n_strx value of 0, there is no string for this value, therefore do not consult the strings table.

Kevin's ld linker agreed with this position, a n_strx == 0 means don't look at the string table, but as a matter of mercy also wrote a \0 byte at the front of the string table, in case someone accidentally dereferenced the string table anyway. If they accidentally tried to read a string from index 0 they'd get back a null terminated string, instead of crashing or getting whatever string happened to be the first item in the string table.

Nick's ld64 linker agreed with this position, a n_strx == 0 means don't look at the string table, but thought "if you get a null string back, that might not be a strong enough indication that you are reading an illegal value from the string table." Consider the case where nm prints an address and then prints the name of the symbol at that address. If you get back "" no name will be printed so there's no indication something went wrong. But if you got back something that wasn't "" you have a chance of noticing something bad has happened. So instead of writing a \0 byte at the front of the string table, ld64 writes a " \0" string at the front of the string table. Then everyone who writes code that walks the nlist can have a unit test that tests for strings that say " " and then fail their test: you're not allowed to look at the string table if n_strx is 0, so if your symbol name is " " you have a bug in your program. Of course, no one actually writes this test, and every Mach-O binary has an extra byte in it. But that's what it's there for. Why Nick chose ' ' and not a visible character that isn't a valid symbol name like '*' or '~' I have no idea.

The thing both ld and l64 have in common is both agree the value of a string at index 0 in the string table is undefined. And that's the most important takeaway. ld64 chooses to write a non-trivial string at this location to help you find your bugs. ld chooses to write an empty string at this location out of a sense of mercy. Neither are wrong. You could write a linker that stores "YOU HAVE A BUG IN YOUR PROGRAM" at the start of the strings table and your linker would not be wrong.

which suggests that the first byte of the string table should be the 0 byte and not the space.

I'm not familiar with the documentation you are citing. It sounds like it's describing the implications of the original ld behavior, but not it's reasoning.

mach-o/nlist.h is a bit more ambiguous. It says:

/*
 * Symbols with a index into the string table of zero (n_un.n_strx == 0) are
 * defined to have a null, "", name.  Therefore all string indexes to non null
 * names must not have a zero string index.  This is bit historical information
 * that has never been well documented.
 */

This comment documents the original ld behavior. It has been known to happen that ld64 has changed the format of the Mach-O binary without updating the appropriate header files or man pages. This might be one of those situations.

Tue, Oct 20, 10:01 PM · Restricted Project
smeenai accepted D89639: [lld-macho] Emit empty string as first entry of string table.

LGTM

Tue, Oct 20, 10:00 PM · Restricted Project

Mon, Oct 19

smeenai updated subscribers of D89561: [MC] Adjust StringTableBuilder for linked Mach-O binaries.
Mon, Oct 19, 7:31 PM · Restricted Project
smeenai added a comment to D89561: [MC] Adjust StringTableBuilder for linked Mach-O binaries.

@mtrent, do you have the historical context for why ld64 adds a leading space to the string table? The relevant lines from its code are:

Mon, Oct 19, 7:30 PM · Restricted Project
smeenai added a comment to D89639: [lld-macho] Emit empty string as first entry of string table.

Do we have any logic to emit an n_strx for an empty string? If so, that would need to be adjusted to emit 1 instead of 0. If not, LGTM.

Mon, Oct 19, 7:03 PM · Restricted Project
smeenai added a comment to D89177: [cmake] Add support for multiple distributions.

Ping. Assuming everyone is okay with the direction, any comments on the change itself?

Mon, Oct 19, 2:24 PM · Restricted Project, Restricted Project, Restricted Project
smeenai accepted D89661: [llvm-objcopy][MachO] Fix the calculation of the output size.

LGTM

Mon, Oct 19, 12:03 PM · Restricted Project

Fri, Oct 16

smeenai accepted D89555: [CMake] Don't add -lstdc++ to LDFLAGS.

Both CMake and the driver (assuming it's invoked as clang++/g++) should take care of adding the C++ standard library, so this LGTM.

Fri, Oct 16, 11:43 AM

Thu, Oct 15

smeenai added a comment to D89177: [cmake] Add support for multiple distributions.

That isn't what I meant. It's entirely okay for the runtimes to be driven via AddExternalProject like the runtimes build does, since that's akin to having a separate CMake invocation for each configuration. That's okay.

What I'm saying is that if the next logical step is to also add support for multiple distributions in libc++'s build itself (e.g. adding LIBCXX_<DISTRIBUTION>_ENABLE_SHARED & al), then I don't think that's a good idea.

Totally agree. That would be the path compiler-rt’s Darwin build goes, and it is a frequent problem.

Thu, Oct 15, 5:10 PM · Restricted Project, Restricted Project, Restricted Project

Wed, Oct 14

smeenai added inline comments to D88922: [CMake] Avoid accidental C++ standard library dependency in sanitizers.
Wed, Oct 14, 11:55 PM · Restricted Project
smeenai accepted D88922: [CMake] Avoid accidental C++ standard library dependency in sanitizers.

This is needed to avoid potential race conditions where some of the C++ headers have been copied but not all of them, and some other runtime that doesn't depend on the C++ headers sees this incomplete state and gets a spurious build failure, so LGTM.

Wed, Oct 14, 6:15 PM · Restricted Project
smeenai updated subscribers of D89177: [cmake] Add support for multiple distributions.

We've already considered introducing a similar mechanism so thank you for working on this! There's one issue that I haven't figured out how to resolve and I'd be interested in your thoughts: building executables and libraries in the most optimal may require incompatible flags. For example, when building a shared library you have to use -fPIC but for executables that's suboptimal; similarly, when building executables you may want to use LTO but if you also want to create a distribution that contains static libraries, that's a problem because those will contain bitcode. So far our solution has been to simply do multiple builds with different flags (in GN this can be done in a single build by leveraging the "toolchain" concept that also allows sharing as much work as possible, but I haven't figured out how to do anything like that in CMake).

Good question. We need something similar as well, where some clients of the libraries want them built with RTTI (because their programs rely on RTTI and you can get link errors when linking them against non-RTTI LLVM libraries), but we don't wanna build the executables with RTTI because of the size increases.

We recently ran into this as well where we need RTTI for LLVM libraries but don't want to enable it for the toolchain.

I don't know of any way to do this in CMake other than just configuring multiple times with the different flags, like you said. Maybe we could implement some logic for that in the build system, but I'm not sure if it's worth it. (I'm wondering if the Ninja multi-config generator added in CMake 3.17 could be useful for this, although I haven't played around with that at all yet.) I do recognize it limits the usefulness of this approach quite a bit (since if you're doing separate configurations you can just do different LLVM_DISTRIBUTION_COMPONENT settings), but I'm hoping it can still be valuable for simpler scenarios.

One idea I had would be to eliminate the use of global variables like CMAKE_C_FLAGS and CMAKE_CXX_FLAGS and always set these via target_compile_options (which is something we should do in any case to move towards more modern CMake); since we always use functions like add_llvm_component_library to create targets in LLVM, we could then have these create multiple targets that would differ in flags like -fPIC or -frtti and set up dependencies among these appropriately. It means you'd have to build files multiple times (I don't think there's any way around that) but you'd save time on things like tablegen.

Wed, Oct 14, 3:40 PM · Restricted Project, Restricted Project, Restricted Project
smeenai updated the diff for D89177: [cmake] Add support for multiple distributions.

Address review comments

Wed, Oct 14, 3:29 PM · Restricted Project, Restricted Project, Restricted Project

Tue, Oct 13

smeenai added a comment to D89177: [cmake] Add support for multiple distributions.

Reading up on the Ninja multi-config generator a bit, you can define your own build types, so you should be able to create e.g. one build type for LTO and another for PIC. You'd still need some external logic to define that you use the LTO mode for executables and the PIC mode for libraries, but the multiple distribution support should work nicely in conjunction with that.

Tue, Oct 13, 12:07 AM · Restricted Project, Restricted Project, Restricted Project

Mon, Oct 12

smeenai added a comment to D89177: [cmake] Add support for multiple distributions.

We've already considered introducing a similar mechanism so thank you for working on this! There's one issue that I haven't figured out how to resolve and I'd be interested in your thoughts: building executables and libraries in the most optimal may require incompatible flags. For example, when building a shared library you have to use -fPIC but for executables that's suboptimal; similarly, when building executables you may want to use LTO but if you also want to create a distribution that contains static libraries, that's a problem because those will contain bitcode. So far our solution has been to simply do multiple builds with different flags (in GN this can be done in a single build by leveraging the "toolchain" concept that also allows sharing as much work as possible, but I haven't figured out how to do anything like that in CMake).

Mon, Oct 12, 11:49 PM · Restricted Project, Restricted Project, Restricted Project
smeenai accepted D88468: [llvm-readobj] Don't print out section names for STABS symbols.

LGTM.

Mon, Oct 12, 3:11 PM · Restricted Project

Fri, Oct 9

smeenai updated subscribers of D89177: [cmake] Add support for multiple distributions.
Fri, Oct 9, 7:38 PM · Restricted Project, Restricted Project, Restricted Project
smeenai requested review of D89177: [cmake] Add support for multiple distributions.
Fri, Oct 9, 7:33 PM · Restricted Project, Restricted Project, Restricted Project
smeenai added a comment to D85140: [RFC] Factor out repetitive cmake patterns for llvm-style projects.

Also a styling nit, this should have been cmake/Modules, not cmake/modules. The former is used only by LLVM and is a historic relic, all other projects use cmake/Modules which is more common in CMake-based builds.

Fri, Oct 9, 3:35 PM · Restricted Project, Restricted Project
smeenai added a reviewer for D89142: llvmbuildectomy: beanz.
Fri, Oct 9, 11:25 AM · Restricted Project

Tue, Oct 6

smeenai added a comment to D88922: [CMake] Avoid accidental C++ standard library dependency in sanitizers.

LGTM, but I'll give the sanitizer folks a bit of time to review as well.

Tue, Oct 6, 1:50 PM · Restricted Project

Mon, Oct 5

smeenai added inline comments to D88176: Explicitly specify CMAKE_AR in WinMsvc.cmake.
Mon, Oct 5, 1:20 PM · Restricted Project
smeenai added a comment to D88843: [libcxx] Don't treat Windows specially with visibility annotations.

I think the reason for this restriction is that on Windows, __declspec(dllimport) changes the compiler's code generation to assume that the symbol will be imported (i.e. if the compiler sees a call to a function X which is marked __declspec(dllimport), it'll generate an indirect call to __imp_X instead). If you're linking against libc++ statically, the imported version of the symbol will of course not exist, so you want to disable the __declspec(dllimport) annotations by default if you're building just the static library for Windows.

Mon, Oct 5, 11:57 AM · Restricted Project

Fri, Oct 2

smeenai added a comment to D88629: [lld-macho] Add ARM64 target arch.

It is not entirely clear whether I use the "ARM64" (Apple) or "AArch64" (non-Apple) naming convention. Guidance is appreciated.

Fri, Oct 2, 7:13 PM · Restricted Project
smeenai added a comment to D88400: [llvm-objcopy][MachO] Add support for universal binaries.

LGTM barring the comment about more tests.

Fri, Oct 2, 6:43 PM · Restricted Project
smeenai accepted D88372: [Object][MachO] Refactor MachOUniversalWriter.

LGTM

Fri, Oct 2, 6:15 PM · Restricted Project
smeenai accepted D88756: [CMake] Don't use CMakePushCheckState.

LGTM

Fri, Oct 2, 2:37 PM · Restricted Project

Thu, Oct 1

smeenai added a comment to D88454: [CMake] Use -isystem flag to access libc++ headers.

@smeenai do you already have an alternative for your use case?

Thu, Oct 1, 11:49 AM · Restricted Project, Restricted Project
smeenai committed rGdcb5b6dfbfb5: [runtimes] Remove TOOLCHAIN_TOOLS specialization (authored by smeenai).
[runtimes] Remove TOOLCHAIN_TOOLS specialization
Thu, Oct 1, 9:54 AM
smeenai closed D88627: [runtimes] Remove TOOLCHAIN_TOOLS specialization.
Thu, Oct 1, 9:53 AM · Restricted Project
smeenai retitled D88627: [runtimes] Remove TOOLCHAIN_TOOLS specialization from Revert "[AIX] Try to not use LLVM tools while building runtimes" to [runtimes] Remove TOOLCHAIN_TOOLS specialization.
Thu, Oct 1, 8:50 AM · Restricted Project
smeenai added a comment to D88627: [runtimes] Remove TOOLCHAIN_TOOLS specialization.

This actually isn't exclusively a revert of D85329 "[AIX] Try to not use LLVM tools while building runtimes" as the title seems to suggest. Before that patch there were already explicit TOOLCHAIN_TOOLS specified in the call to llvm_ExternalProject_Add in the runtimes build, which already suppressed the target-specific tool selection as mention in the description here.

That said, I'm not opposed to the resulting change, which moves all that logic to one place rather than having an extra layer on top just for the runtimes build. We should clarify the title and description though, because removing the TOOLCHAIN_TOOLS here entirely will actually be a new change to how runtimes are built.

Thu, Oct 1, 8:49 AM · Restricted Project

Wed, Sep 30

smeenai requested review of D88627: [runtimes] Remove TOOLCHAIN_TOOLS specialization.
Wed, Sep 30, 6:05 PM · Restricted Project

Mon, Sep 28

smeenai added a comment to D88454: [CMake] Use -isystem flag to access libc++ headers.

I was abusing this functionality to be able to get the C++ headers installed without having to build all the runtimes build prerequisites; we have a configuration where we only need the C++ headers installed (and not the library), and using the runtime-libcxx-headers was an easy way to get them without needing to first build Clang and all the other tools the runtimes build requires. I should be able to come up with something else for that use case though.

Mon, Sep 28, 5:36 PM · Restricted Project, Restricted Project

Wed, Sep 23

smeenai updated subscribers of D87199: [lld-macho] Implement support for PIC.

but would any of this be easier if SyntheticSections were InputSections instead of OutputSections

Maybe... we could make them InputSections and then generate unsigned relocations for each of their entries. There would be some additional overhead (e.g. the addend field would be useless) but it would allow us to handle the rebases in one place. One thing I'm not sure about is whether other architectures have an equivalent of X86_64_RELOC_UNSIGNED that can be used for this; it looks like arm64 does, but not sure about PPC (not that it really matters).

There are other benefits to having SyntheticSections be InputSections: there would no longer be a need for the SectionPointerUnion, and DSOHandle could just be an ordinary Defined symbol.

I actually can't think of many real downsides... do you have any in mind? That said, the current architecture is not terrible, and I'm inclined to work on implementing more features and pushing the refactoring till later.

Wed, Sep 23, 11:15 PM · Restricted Project
smeenai added inline comments to D87199: [lld-macho] Implement support for PIC.
Wed, Sep 23, 10:58 PM · Restricted Project
smeenai committed rGee7ee71f40e9: Explicitly specify CMAKE_AR in WinMsvc.cmake (authored by Gwen Mittertreiner <gwenm@fb.com>).
Explicitly specify CMAKE_AR in WinMsvc.cmake
Wed, Sep 23, 6:06 PM
smeenai closed D88176: Explicitly specify CMAKE_AR in WinMsvc.cmake.
Wed, Sep 23, 6:06 PM · Restricted Project
smeenai added a comment to D88176: Explicitly specify CMAKE_AR in WinMsvc.cmake.

The LLVMExternalProjectUtils change is needed either way, but I believe the WinMsvc case to be a CMake bug, and I've put up https://gitlab.kitware.com/cmake/cmake/-/merge_requests/5264 to fix it. (We should still go ahead with it as a workaround for now though.)

Wed, Sep 23, 5:12 PM · Restricted Project
smeenai accepted D88176: Explicitly specify CMAKE_AR in WinMsvc.cmake.

https://gitlab.kitware.com/cmake/cmake/-/commit/55196a1440e26917d40e6a7a3eb8d9fb323fa657 is the relevant CMake change, and I believe we need to add logic to find llvm-lib. I'll play with that.

Wed, Sep 23, 3:47 PM · Restricted Project
smeenai accepted D88176: Explicitly specify CMAKE_AR in WinMsvc.cmake.

It seems strange for CMake to prefer "ar" to "lib" when targeting Windows. Do you happen to know the CMake change that's responsible?

Wed, Sep 23, 11:55 AM · Restricted Project
smeenai accepted D88160: [lld-macho] cleanup unimplemented-option warnings.

You have a small typo in the commit message: s/ldd/lld/

Wed, Sep 23, 11:11 AM · Restricted Project

Tue, Sep 22

smeenai added inline comments to D87852: [lld-macho] Allow the entry symbol to be dynamically bound.
Tue, Sep 22, 2:24 PM · Restricted Project
smeenai accepted D87852: [lld-macho] Allow the entry symbol to be dynamically bound.

LGTM. It's interesting that this is a thing.

Tue, Sep 22, 12:52 PM · Restricted Project
smeenai accepted D87909: [lld-macho] Support absolute symbols.

LGTM

Tue, Sep 22, 12:21 PM · Restricted Project
smeenai accepted D87856: [lld-macho] Support -bundle.

LGTM

Tue, Sep 22, 12:06 PM · Restricted Project
smeenai accepted D87959: [lld-macho][NFC] Refactor syslibroot / library path lookup.

LGTM

Tue, Sep 22, 12:00 PM · Restricted Project
smeenai requested changes to D87960: [lld-macho] Always include custom syslibroot when running tests.

The test cleanups are really nice, but I'm not a fan of coding this sort of test-specific behavior (especially the path) into LLD itself :/ (I know we have some precedent with LLD_IN_TEST, but that's only used in canExitEarly and it's a pretty minor behavior change.)

Tue, Sep 22, 11:58 AM · Restricted Project

Mon, Sep 21

smeenai accepted D88069: [CMake] Use find_dependency in LLVMConfig.cmake.

LGTM

Mon, Sep 21, 11:02 PM · Restricted Project
smeenai accepted D88068: [CMake] Use append for CMAKE_REQUIRED_* variables.

LGTM

Mon, Sep 21, 11:00 PM · Restricted Project

Sep 21 2020

smeenai added inline comments to D87780: [lld-macho] Export trie addresses should be relative to the image base.
Sep 21 2020, 11:55 AM · Restricted Project

Sep 17 2020

smeenai added inline comments to D87780: [lld-macho] Export trie addresses should be relative to the image base.
Sep 17 2020, 5:00 PM · Restricted Project
smeenai accepted D87780: [lld-macho] Export trie addresses should be relative to the image base.

LGTM

Sep 17 2020, 3:58 PM · Restricted Project

Sep 16 2020

smeenai added a comment to D31413: [libc++] Use __attribute__((init_priority(101))) to ensure streams get initialized early.

What was the conclusion for the comments about the priority level (100 vs. 101)?

Sep 16 2020, 1:09 PM · Restricted Project

Sep 9 2020

smeenai added a comment to D87199: [lld-macho] Implement support for PIC.

Is there a funnel point that we know that all the rebase entries will pass through to ensure that we catch them rather than adding them at a couple of sites?

It may be clearer if we scan the GOT/LazyPointerSection inside RebaseSection::finalizeContents() for any absolute addresses, rather than relying on the code that's adding entries to either of those sections to also handle the rebasing. I will try that... however, there isn't a funnel point that will cover absolute addresses from both our SyntheticSections as well as from InputSections.

Sep 9 2020, 6:16 PM · Restricted Project

Sep 8 2020

smeenai added inline comments to D86805: [lld-macho] create __TEXT,__unwind_info from __LD,__compact_unwind.
Sep 8 2020, 5:17 PM · Restricted Project
smeenai accepted D86908: [lld-macho] Mark weak symbols in symbol table.

LGTM

Sep 8 2020, 1:47 PM · Restricted Project

Aug 28 2020

smeenai added a comment to D86762: [ELF] Add documentation for --warn-backrefs: a GNU ld compatibility checking tool (and lesser of layering detection).

Is there a more specific term than "traditional linker"?

I understand that LLD's search strategy is uncommon among linkers in the Posix (and/or ELF?) realm, but it's not uncommon in other linkers, even very old ones. For example, the VMS (now OpenVMS) linker has used LLD's strategy by default for many decades. (It does allow opting in to the more restrictive search.) LLD's approach matches Microsoft's link.exe. I can't be sure, but I don't recall the so-called traditional behavior from linkers from other vendors of Windows, DOS, or CP/M linkers.

I have no objection to turning the warning on in appropriate contexts or with the argument that it helps to ensure good layering. I'm just quibbling over the term "traditional."

Aug 28 2020, 2:17 PM · Restricted Project

Aug 27 2020

smeenai accepted D86749: [lld-macho][NFC] Define isHidden() in LinkEditSection.

LGTM

Aug 27 2020, 4:47 PM · Restricted Project
smeenai accepted D86746: [lld-macho] Weak locals should be relaxed too.

LGTM; good catch.

Aug 27 2020, 4:11 PM · Restricted Project
smeenai accepted D86575: [lld-macho] Emit binding opcodes for defined symbols that override weak dysyms.

LGTM

Aug 27 2020, 2:24 PM · Restricted Project
smeenai accepted D86728: [lld-macho] Disable invalid/stub-link.s test for Mac.
Aug 27 2020, 11:22 AM · Restricted Project
smeenai accepted D86573: [lld-macho] Implement weak binding for branch relocations.

LGTM

Aug 27 2020, 10:56 AM · Restricted Project

Aug 26 2020

smeenai accepted D86642: [lld-macho] Support GOT relocations to __dso_handle.
Aug 26 2020, 10:01 PM · Restricted Project
smeenai accepted D86641: [lld-macho] Implement GOT_LOAD relaxation.

LGTM

Aug 26 2020, 9:59 PM · Restricted Project
smeenai added inline comments to D86575: [lld-macho] Emit binding opcodes for defined symbols that override weak dysyms.
Aug 26 2020, 9:54 PM · Restricted Project
smeenai accepted D86574: [lld-macho] Emit the right header flags for weak bindings/symbols.

LGTM

Aug 26 2020, 9:35 PM · Restricted Project
smeenai accepted D86573: [lld-macho] Implement weak binding for branch relocations.

LGTM

Aug 26 2020, 9:21 PM · Restricted Project
smeenai accepted D86572: [lld-macho] Implement weak bindings for GOT/TLV.

LGTM

Aug 26 2020, 4:54 PM · Restricted Project
smeenai accepted D86640: [lld-macho] Implement -all_load.

LGTM

Aug 26 2020, 4:14 PM · Restricted Project
smeenai accepted D86571: [lld-macho][NFC] Handle GOT bindings and regular bindings more uniformly.

LGTM; this is a nice cleanup.

Aug 26 2020, 4:02 PM · Restricted Project
smeenai accepted D85404: [lld-macho] Handle TAPI and regular re-exports uniformly.

LGTM. Thanks for limiting re-exports to sub-documents in the same TBD!

Aug 26 2020, 2:05 PM · Restricted Project
smeenai accepted D86180: [lld-macho] Make it possible to re-export .tbd files.

LGTM

Aug 26 2020, 1:46 PM · Restricted Project
smeenai added a comment to D86640: [lld-macho] Implement -all_load.

You're missing a test case.

Aug 26 2020, 1:42 PM · Restricted Project
smeenai accepted D86181: [lld-macho] Implement -ObjC.

LGTM

Aug 26 2020, 1:41 PM · Restricted Project

Aug 25 2020

smeenai added a comment to D86359: [llvm-libtool-darwin] Add support for -V option.

It looks like no documentation was added for the new option. Please add it!

Aug 25 2020, 3:04 PM · Restricted Project
smeenai committed rG22cd6bee4a7d: [llvm-libtool-darwin] Address post-commit feedback (authored by smeenai).
[llvm-libtool-darwin] Address post-commit feedback
Aug 25 2020, 3:04 PM

Aug 24 2020

smeenai committed rG2c80e2fe51b6: [runtimes] Use llvm-libtool-darwin for runtimes build (authored by smeenai).
[runtimes] Use llvm-libtool-darwin for runtimes build
Aug 24 2020, 1:49 PM
smeenai committed rGa7d8aabf298c: [runtimes] Remove TOOLCHAIN_TOOLS specialization (authored by smeenai).
[runtimes] Remove TOOLCHAIN_TOOLS specialization
Aug 24 2020, 1:49 PM
smeenai committed rG68bae34c65b3: [llvm-libtool-darwin] Add support for -V option (authored by smeenai).
[llvm-libtool-darwin] Add support for -V option
Aug 24 2020, 1:49 PM
smeenai committed rG26c1d689ae4c: [compiler-rt] Disable ranlib when using libtool (authored by smeenai).
[compiler-rt] Disable ranlib when using libtool
Aug 24 2020, 1:49 PM
smeenai closed D86367: [runtimes] Use llvm-libtool-darwin for runtimes build.
Aug 24 2020, 1:49 PM · Restricted Project, Restricted Project
smeenai closed D86366: [runtimes] Remove TOOLCHAIN_TOOLS specialization.
Aug 24 2020, 1:49 PM · Restricted Project
smeenai closed D86365: [compiler-rt] Disable ranlib when using libtool.
Aug 24 2020, 1:49 PM · Restricted Project
smeenai closed D86359: [llvm-libtool-darwin] Add support for -V option.
Aug 24 2020, 1:49 PM · Restricted Project
smeenai added a comment to D85740: Universal MachO: support LLVM IR objects.

I think llvm-libtool-darwin would also need some adjustments to handle bitcode objects, archives, and universal binaries. That's definitely out of scope for this diff, but I'm just flagging it as another tool with missing bitcode support.

Aug 24 2020, 10:52 AM · Restricted Project

Aug 21 2020

smeenai requested review of D86367: [runtimes] Use llvm-libtool-darwin for runtimes build.
Aug 21 2020, 1:10 PM · Restricted Project, Restricted Project
smeenai requested review of D86366: [runtimes] Remove TOOLCHAIN_TOOLS specialization.
Aug 21 2020, 1:10 PM · Restricted Project
smeenai requested review of D86365: [compiler-rt] Disable ranlib when using libtool.
Aug 21 2020, 1:09 PM · Restricted Project
smeenai updated subscribers of D86263: [ELF] Keep st_type for symbol assignment.

@hans, would you consider this for 11.0? 11.0 LLD will no longer perform ARM-Thumb interworking for symbols that aren't marked STT_FUNC, so this fixes cases where interworking would incorrectly not occur because of a simple alias.

Aug 21 2020, 11:00 AM · Restricted Project
smeenai requested review of D86359: [llvm-libtool-darwin] Add support for -V option.
Aug 21 2020, 10:54 AM · Restricted Project

Aug 20 2020

smeenai committed rG16f27e1e18fd: [cmake] Don't use ld.lld when targeting Darwin (authored by smeenai).
[cmake] Don't use ld.lld when targeting Darwin
Aug 20 2020, 7:52 PM
smeenai committed rGe2ab5bcf5691: [runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin (authored by smeenai).
[runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin
Aug 20 2020, 6:29 PM
smeenai closed D86313: [runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin.
Aug 20 2020, 6:29 PM · Restricted Project
smeenai added a comment to D86313: [runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin.

I agree that the current situation is not optimal, I was never happy with the default case which is confusing for a lot of people I talked to. I would like to come up with a better solution, but I'm not sure if special casing the x86_64-apple-darwin is sufficient though. With Apple Silicon, I assume we're going to be building runtimes as universal binaries that target multiple architectures.

We could consider completely decoupling runtimes from targets. Originally, each runtime build was identified only by its target, so you'd use RUNTIMES_${target}_${variable} to set target-specific variables. That became insufficient when we introduced the support for multilib, so now you also have a name and you can use RUNTIMES_${target}+${name}_${variable}. We could switch to RUNTIMES_${name}_${variable} and it'd be the responsibility of the user to set the target explicitly if they want it (FWIW it's something we already have to do in our build anyway because of Windows where the CMake behavior when you set CMAKE_${LANG}_COMPILER_TARGET breaks the build). If not, they get the default behavior, matching the default case today. What do you think?

To clarify, what would be setting ${name} in this scenario?

It'd be set by user. Today we have BUILTIN_TARGETS and RUNTIME_TARGETS, we could either repurpose those and create new variables. You'd use it as follows:

list(APPEND BUILTIN_TARGETS "darwin")
set(BUILTINS_darwin_CMAKE_SYSTEM_NAME Darwin CACHE STRING "")
...
list(APPEND BUILTIN_TARGETS "linux")
set(BUILTINS_linux_CMAKE_SYSTEM_NAME Linux CACHE STRING "")
set(BUILTINS_linux_CMAKE_C_COMPILER_TARGET x86_64-linux-gnu CACHE STRING "")
...

Here darwin and linux are arbitrary names.

Aug 20 2020, 5:06 PM · Restricted Project
smeenai updated the diff for D86313: [runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin.

Check LLVM_RUNTIME_TARGETS as well

Aug 20 2020, 3:30 PM · Restricted Project
smeenai added a comment to D86313: [runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin.

Okay, I would argue that this version of the change is much better than the current situation. In particular, it disallows the loophole of specifying a triple for a particular platform instead of "darwin", and it forces users of "darwin" to acknowledge they understand how the Darwin compiler-rt build works by setting a particular variable (and it still disallows multiple Darwin triples). Lemme know what you think.

Aug 20 2020, 2:13 PM · Restricted Project
smeenai updated the diff for D86313: [runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin.

Add explicit error and flag

Aug 20 2020, 2:11 PM · Restricted Project
smeenai added a comment to D86313: [runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin.

I think this change makes configuration confusingly inaccurate. Since the compiler-rt Darwin build does not support picking and choosing which targets to configure making the configuration fail if a user tries to is the reasonable approach.

Making the build do something different and unintuitive from what the configuration requested seems undesirable to me.

Aug 20 2020, 1:17 PM · Restricted Project
smeenai requested review of D86313: [runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin.
Aug 20 2020, 12:45 PM · Restricted Project