CC @hans ... there was discussion in https://bugs.llvm.org/show_bug.cgi?id=44397 about whether this is worth picking to 10.0.

In D73253#1836910, @jyknight wrote:

I think we should instead remove the entire "tracked independently" parts, because that's no longer the recommended directory layout. And also remove these "global" ignores which you moved, because they're already in /.gitignore, and thus don't need to be in /llvm/.gitignore

I'm assuming git-phab is https://pypi.org/project/git-phab/

Comments

This LGTM, but I'll give the clang-tidy developers some time to weigh in.

Can you upload the patch with context (git diff -U9999 if you're using the web interface or via arcanist)?

In D72943#1832114, @MaskRay wrote:

In D72943#1832079, @smeenai wrote:

rGb841e119d77ed0502e3a2e710f26a899bef28b3c is in the 10.0 branch. Should this be cherry picked to 10.0? CC @hans

This commit is in release/10.x

git branch origin/release/10.x --contains 9aacec8331d9bc1f80046e5954be1f5e0e0acd42 => success

rGb841e119d77ed0502e3a2e710f26a899bef28b3c is in the 10.0 branch. Should this be cherry picked to 10.0? CC @hans

CC @plotfi for the InterfaceStubs failures

In D72952#1827678, @JamesNagurne wrote:

In D72952#1827668, @smeenai wrote:

The diff here looks weird ... it looks like a diff against your previous diff instead of a diff against master? (Phabricator says the line you're deleting has LANGUAGE ASM instead of LANGUAGE C, as it does on master.)

Eek, sorry. We use CodeCollaborator internally, which would have effectively shown the diff from the original commit to the final (after 2 change sets), which would have shown the C in the before and nothing after. I mistakenly treated Phabricator the same way erroneously.

I'll re-upload this so it looks right. My apologies!

The diff here looks weird ... it looks like a diff against your previous diff instead of a diff against master? (Phabricator says the line you're deleting has LANGUAGE ASM instead of LANGUAGE C, as it does on master.)

Adding @compnerd, who's familiar with both libunwind and CMake :)

In D72543#1825454, @rjmccall wrote:

In D72543#1825422, @smeenai wrote:

In D72543#1825178, @rjmccall wrote:

In D72543#1825150, @smeenai wrote:

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

Makes sense. Should rL319123 be reverted then? The extra space that's being allocated by that change doesn't do what it's meant to do, so it seems wasteful to keep allocating it.

If __cxa_exception is already sufficiently aligned, I believe that patch does not allocate any extra space.

In D72543#1825178, @rjmccall wrote:

In D72543#1825150, @smeenai wrote:

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

Should this be cherry-picked to the release branch?

Is this worth merging into 10.0 if it lands after the branch?

If any follow-ups get landed after the branch, can we make sure they get merged to the branch? Thanks!

To make sure I'm understanding this correctly, previously, for any function with a loop in it, XRay would ignore the minimum instruction count threshold and instrument it unconditionally. You're adding an option to make XRay respect the threshold even if the function has loops. Is that correct?

Sorry for the delay. Committed this.

In D70764#1803559, @raj.khem wrote:

this is now in master, and I am seeing build failures in cross-building clang, e.g. when building clang for arm on a x86_64 host. its resorting to finding, libz from buildhost instead of target sysroot ( using --sysroot) and failing in link step. e.g.

FAILED: bin/llvm-config
...
.... -o bin/llvm-config -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.a /usr/lib/libz.so -lrt -ldl -ltinfo -lm lib/libLLVMDemangle.a
...

aarch64-yoe-linux-musl/aarch64-yoe-linux-musl-ld: /usr/lib/libz.so: error adding symbols: file in wrong format
clang-10: error: linker command failed with exit code 1 (use -v to see invocation)

you can see that its adding /usr/lib/libz.so to linker cmdline while cross linking.

Sorry, was on vacation. Looks good, thanks!

I'm not sure where best to document this. We have https://llvm.org/docs/HowToBuildWithPGO.html, but that doesn't mention the build system's built-in PGO support as far as I can tell.

LGTM ... I'll wait a few more days in case anyone has comments.

Makes sense to me ... it'd be ideal to limit this to runtimes being built for Darwin targets, but idk if that's possible, and I don't think it's that big of a deal to build llvm-lipo when it's not needed, since it's a pretty tiny dependency.

LGTM

This makes sense to me ... @beanz @phosek what do you think?

LGTM, thanks.

Ping.

LGTM

It's recommended to uploaded patches with full context (e.g. by using git diff -U9999 when uploading a patch using the web interface).

Ping.

Are there plans to move this forward?

Somewhat related to the comments, I believe lld-link's ability to generate an import library from a def file directly is non-standard. MSVC's toolchain requires you to go through lib for that.

LGTM

Accepting to unblock, though I'd like to see the LIBCXXABI_USE_SYSTEM_LIBCXX solution explored as a follow-up.

Thanks. WIll wait for an approval from a libc++ code owner before committing.

Commit message needs to be updated.

In D69973#1738127, @sbc100 wrote:

I don't think so no, its the dependency between libcxxabi and the libcxx that is the issue. I think libcxxabi assumes the libcxx is in non-default place when it builds. Perhaps out case that actually isn't true so another possible fix might be to add LIBCXXABI_USE_SYSTEM_LIBCXX which would avoid the need to -nostdinc (although maybe thats needed for other reasons too).

I converted the test to YAML and committed it as rG56cd447eec8e.

This needs a test case, and it's a good idea to upload patches with more context (use -U99999 if generating a patch via git to upload to the web interface, or just use arcanist).

It seems like the commit dropped the tests. I'm planning to re-add them, and try converting the binary to YAML in the process.

@mvels, I'm assuming you don't have commit access to LLVM. The standard procedure there is that when your commit is accepted, you comment saying you don't have commit access and ask a reviewer to commit it for you. In this case, @mclow.lists should be able to commit it on your behalf.

In D69931#1739063, @beanz wrote:

It is a bit gross that Python does an @rpath install name, that is generally against convention. I can see adding an option to add rpaths to liblldb making sense. I certainly agree LLDB shouldn't be in the business of packaging transitive dependencies.

In a broader sense, Frameworks are dylibs. They can have all the same rpath magic that a dylib has. When you link a macho, the static linker pulls the install_name from each dylib you link against and embeds those as the dylibs to load. Since Python 3.7's install_name is @rpath based, it means that the dynamic linker will look for it in all the @rpath-relative locations specified by liblldb when linking. The proposed path in this patch, -rpath "@loader_path/../../../", uses the @loader_path expansion which is the directory containing the binary that the load command is in (in this case liblldb's directory). Popping 3 directories up from that is likely not sane in most build configurations, but if it works for you meh...

This LGTM, thanks! I'll give a couple of days for @hans and @tstellar to look at this in case they have any thoughts as the release managers.

In D69983#1738155, @enh wrote:

honestly, if it was me, i'd keep all these inlines even for current releases: inlining a call to the underlying function saves you a layer of useless cruft at runtime. (the Android "implementations" are just one-liners that drop the extra argument and call the underlying function, just like these inlines.)

if Android were the only system making use of this and you could otherwise remove it, that would be different, but since you need it for other configurations anyway...

You should report a bug to CMake (you're right that their implicit removal should account for the various -nostdinc-like options), but this seems like an okay workaround in the meantime.

I agree on not getting into the business of installing Python ourselves, but there's also the rpath issue. The Windows case is different because you just put all your DLLs in the same directory (or some other directory in your PATH) and you're done. With macOS, you have to specify an rpath for liblldb to be able to find the Python framework. It'd be really nice to support adding additional rpaths in the build system itself (it could be a more generic mechanism; it doesn't have to be Python-specific). CMake does offer CMAKE_INSTALL_RPATH, but that applies to every single target. You could also use install_name_tool after building to modify the rpath, but that feels like a kludge.

I can commit this for you tomorrow if one of your reviewers hasn't gotten to it by then.

Will you need someone to commit this for you?

LGTM ... seems like a trivial change that matches the existing handling of LC_VERSION_MIN_MACOSX (and adds test coverage as an added bonus).

The change itself looks fine, but idk if we need to preserve support for any of the older variables (LLVM_ENABLE_CXX1Z in particular). Idk if there's an easy way to e.g. audit bot configurations to see what variables they're using ... maybe posting to llvm-dev would be a good idea?

LGTM

In D69407#1720499, @xiaobai wrote:

Why do you want LLVM_CXX_STD to be removed? I suppose somebody will eventually add LLVM_ENABLE_CXX2A, which will require additional logic to ensure that somebody doesn't set LLVM_ENABLE_CXX2A and LLVM_ENABLE_CXX1Z at the same time. I think this can be avoided by removing LLVM_ENABLE_CXX1Z entirely and going through LLVM_CXX_STD entirely.

In D68952#1718235, @CaseyCarter wrote:

(I'm not sure how to withdraw my "Request Changes" without approving - I'm not an authorized approver for libc++ - so I'll "Resign as Reviewer" and see if my red X goes away.)

LGTM