In D63341#1552257, @anushabasana wrote:

In D63341#1549652, @mtrent wrote:

Would be nice if "llvm-lipo -thin" printed a reasonable error message before dumping usage. Apple lipo will print "missing argument to -thin option"

Another difference is llvm-lipo will not validate the arch until all arguments are received, whereas Apple lipo will validate archs immediately. ("lipo -arch asdf")

Would be nice if "llvm-lipo" printed a list of valid archs when the arch is invalid. Currently it prints "llvm-lipo: error: Invalid architecture: asdf" and it exits. Apple lipo prints "unknown architecture specification flag: asdf in specifying thin operation: -thin asdf. known architecture flags are: ..."

PLEASE FIX: llvm-lipo does not support using "-o" in place of "-output"

Note: "llvm-lipo" does not work properly with certain Mach-O executables, such as those built with the -hideARM64 flag.

Fixed most of these issues, but for printing a list of knows architectures I am planning to make a change in a separate diff that returns a list of valid architectures, since I will no longer be modifying llvm-lipo code.

I'll wait for a day in case any other reviewers have comments and then commit this for you.

LGTM

Looking good, just a few style nits.

LGTM

In D63453#1548672, @rnk wrote:

In D63453#1547446, @Bigcheese wrote:

Where do you want to keep these open? The default OS limit on open files for OSX is 256.

In COFF LLD.

Ah, so just having variables to hold the different distribution sets? Makes sense ... you'd need to adjust the CMake exporting logic, and I'm not quite sure how that'll play out yet, but we can see when we get to that.

I'll go ahead and commit this since all comments have been addressed.

Sorry, I thought James was gonna commit this, but I'll go ahead and do so.

This is technically a behavior change because with the old code, you wouldn't run any of the distribution code if LLVM_DISTRIBUTION_COMPONENTS was empty, even if LLVM_RUNTIME_DISTRIBUTION_COMPONENTS was non-empty. The new behavior seems more correct though.

This looks good, but what were you envisioning the interface would look like for configuring multiple distributions? It'd be a lot easier to judge with that context.

Phabricator memes are the best thing about Phabricator :)

<3

LGTM, but I'd wait for @mtrent.

We're actively working on llvm-lipo (we have a bunch more diffs in the pipeline that'll be coming through over the summer). Is it worth adding either a TODO or filing a bug to change these tests once llvm-lipo is complete enough to support the required functionality?

LGTM with the NO_CMAKE_FIND_ROOT_PATH added.

@winksaville I can commit this for you on Monday if no one's gotten to it before.

LGTM

LGTM

(or if they're different, what the difference is and whether that's desirable)

Could you perform a configure before and after this change and confirm that your CMakeCache.txt is the same? If so, this LGTM.

In D62063#1509321, @rnk wrote:

Seems reasonable to me. I'm surprised CMake doesn't have a general facility for escaping paths used in cflags. I suppose users generally don't add paths, just flags.

In D61974#1508097, @ahatanak wrote:

Delete the entire code path that tries to work around a bug in gcc.

I believe MinGW lowercases all their headers, so that's the convention LLVM has been following, since cross-compilation using the Windows SDK generally involves some sort of case correction (through VFS overlays or CIOPFS or similar) anyway.

This is a straightforward build system fix, so it should be fine.

Looks good, though I'm not super familiar with the libObject APIs (yet) :)

D61688 implements -z common-page-size.

Will address any additional comments post-commit. Thanks for the quick review!

LGTM, neat!

LGTM, given it doesn't have any effect by default. I'm not sure if there are negative consequences around e.g. stack walking for crashes, but having the option shouldn't hurt.

Seems like you got the wrong diff.

This is really nasty, but I'm not sure if there's a better way to do this.

In D61275#1484827, @lebedev.ri wrote:

In D61275#1484816, @smeenai wrote:

Last time this came up, we wanted to implement a check to ensure no source files were forgotten, as in do a glob and compare to the explicit list of files.

Is it possible to forget a file, without it failing some lit test?
Are globs used elsewhere in llvm's cmake for this reason? I hope not. I don't remember it causing much issues.

The original plan was to use llvm_process_sources for that, but we couldn't do that cos libc++'s build can't rely on LLVM being around yet (though maybe we ought to reconsider that at some point with the upcoming monorepo move...).

Passing-by 2 cent: that seems like an unreasonably high price for an insufficiently motivating reason.

Do you think it's worth implementing that?

Sorry, had missed this.

LGTM

Last time this came up, we wanted to implement a check to ensure no source files were forgotten, as in do a glob and compare to the explicit list of files. The original plan was to use llvm_process_sources for that, but we couldn't do that cos libc++'s build can't rely on LLVM being around yet (though maybe we ought to reconsider that at some point with the upcoming monorepo move...). Do you think it's worth implementing that?

LGTM with the indent fixed.

This is awesome!

I feel something like llvm::sys::path::PathListSeparator would be nicer, since AFAIK any path list should be separated with semicolons on Windows and colons everywhere else.

LGTM, though you'll wanna wait for a libc++ code owner too.

LGTM, though I'd wait for Hans and/or Reid too.

In D61100#1477850, @ahatanak wrote:

The test case I'm looking at has a function with over 500k instructions and lots of ObjC runtime function calls.

This would break if someone does a debug and release build simultaneously, the copies occur simultaneously and the filesystem gets unhappy, That seems fairly contrived though, and this is also restoring the behavior before your previous change.

LGTM, thanks!

@tstellar ping. Someone appears to be running into this on the CMake mailing list too: https://cmake.org/pipermail/cmake/2019-April/069359.html