Rebase

Define macro to 1

Address review comments

In D126397#3537843, @sammccall wrote:

Thank you! I have been banging my head against this, I'm not entirely sure why this works and my version doesn't.

In D125667#3537678, @smeenai wrote:

In D125667#3537655, @sammccall wrote:

I'll see if I can fix it quickly, else will revert (there are a couple of fixes stacked on top already, so revert isn't quite clean)

Thank you! I'll have some time to look at this a bit later myself as well, and it should hopefully be a quick fix, so we can try to avoid the revert :)

In D125667#3537655, @sammccall wrote:

I'll see if I can fix it quickly, else will revert (there are a couple of fixes stacked on top already, so revert isn't quite clean)

This breaks cross-compilation, because we're building pseudo-gen for the target but then trying to run it on the build machine. You'll need to do something like TableGen does (see https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/CrossCompile.cmake and the uses of build_native_tool). That being said, I'm trying to figure out why our builds are attempting to build this in the first place, since as far as I can tell nothing external depends on it yet.

Better MSAN suppression

XFAIL with MSAN, similar to other tests

Use sizeof instead of raw number

D126343 implements the process_vm_readv approach

In D125827#3534224, @ldionne wrote:

In D125827#3525570, @smeenai wrote:

LGTM, thanks.

In D125827#3525320, @ldionne wrote:

In D125827#3520509, @smeenai wrote:

I think this might potentially break cross-compilation on Windows. We're in a NOT WIN32 block here, but that means you're not targeting Windows; you could still be targeting e.g. Linux but building on a Windows machine. I think NUL is the Windows equivalent, and you could have a CMAKE_HOST_WIN32 block, but it'll depend on how CMake invokes an execute_process command. (I have a Windows machine handy and can experiment with that, if you'd like.) That being said

Hmm, I just tried it out on a Windows machine and it seems to work.

To be clear, were you building for Windows on a Windows machine, or targeting some other platform? The NOT WIN32 above would have already taken care of the building for Windows on Windows case.

Ugh, yeah, then I guess my testing wasn't sound. Do you have a setup to target Linux from a windows host?

LGTM, thanks. I'm surprised these weren't caught by the lint running on the diffs where they were introduced.

In D125827#3525320, @ldionne wrote:

In D125827#3520509, @smeenai wrote:

I think this might potentially break cross-compilation on Windows. We're in a NOT WIN32 block here, but that means you're not targeting Windows; you could still be targeting e.g. Linux but building on a Windows machine. I think NUL is the Windows equivalent, and you could have a CMAKE_HOST_WIN32 block, but it'll depend on how CMake invokes an execute_process command. (I have a Windows machine handy and can experiment with that, if you'd like.) That being said

Hmm, I just tried it out on a Windows machine and it seems to work.

In the LLVM Windows toolchain, we work around this by creating a directory of symlinks: https://github.com/llvm/llvm-project/blob/8527f32f0a16a77edb0b53a5ab8074e518eeff54/llvm/cmake/platforms/WinMsvc.cmake#L133-L157. A VFS overlay would be a nicer solution, and I'm excited to have this supported. I'll leave the review to @rnk and @mstorsjo though.

Removing from my queue in the meantime

I think this might potentially break cross-compilation on Windows. We're in a NOT WIN32 block here, but that means you're not targeting Windows; you could still be targeting e.g. Linux but building on a Windows machine. I think NUL is the Windows equivalent, and you could have a CMAKE_HOST_WIN32 block, but it'll depend on how CMake invokes an execute_process command. (I have a Windows machine handy and can experiment with that, if you'd like.) That being said

We're seeing an internal failure after this change as well :( Great catch, but it looks like it exposed some existing brokenness.

In D124810#3501235, @gracejennings wrote:

In D124810#3498019, @smeenai wrote:

LGTM

Thank you for the review! I don't yet have commit access. Would you commit the change on my behalf?

LGTM

Planning changes to remove from people's queues while I address the comments (which will take a bit, since I'm out next week). Thanks for the reviews!

In D123428#3487134, @rprichard wrote:

In D123428#3476371, @rprichard wrote:

In D123428#3475346, @smeenai wrote:

In D123428#3444212, @MaskRay wrote:

https://github.com/abseil/abseil-cpp/blob/master/absl/debugging/internal/address_is_readable.cc has gone through several iterations. We can use rt_sigprocmask.

@rprichard, do you know if this would work for Android? It has the TOCTOU issue, but I imagine it's much simpler than having to manage and synchronize the pipe fd, and we could live with the TOCTOU in practice.

I'm glad @MaskRay found this -- I think it will probably work, and it seems better than assuming the PC is readable.

I see rt_procsigmask listed in bionic/libc/SYSCALLS.TXT, and I don't see it mentioned in any of the bionic/libc/SECCOMP*.txt files. I think that means seccomp is allowing the system call. I looked at kernel/signal.c, and AFAICT it's not doing any security checks that could be a problem. Bionic itself uses rt_sigprocmask for (at least) spawning new processes, creating/exiting threads, TLS-related locking, POSIX timers, and abort(). I think any seccomp-like blocking of rt_sigprocmask would have to be very targeted, so I think the syscall is probably allowed everywhere on Android.

It is assuming that the kernel will validate the address before validating the how. The kernel has a principle of not breaking userland -- is there a more specific guarantee we can rely on? e.g. The code has this comment:

// This strategy depends on Linux implementation details,
// so we rely on the test to alert us if it stops working.

The kernel source is structured as two wrappers around sigprocmask, SYSCALL_DEFINE4(rt_sigprocmask, ...) and COMPAT_SYSCALL_DEFINE4(rt_sigprocmask, ...). The wrappers copy user memory to/from kernel memory before calling sigprocmask, so it makes sense that they would validate the address first.

I talked with cferris today (the maintainer of Android's libunwindstack), and I think LLVM's libunwind could just use process_vm_readv, which is more straightforward and would also avoid the TOCTOU issue.

process_vm_readv is used by libunwindstack, and cferris wasn't aware of any situation where process_vm_readv was blocked.

process_vm_readv is presumably slower than rt_procsigmask, but that's probably acceptable? It's only used when the unwinder can't find DWARF unwind info for a PC.

process_vm_readv requires a 3.2 kernel or newer, but this is syscall only needed for arm64, and arm64 support wasn't added until a later kernel version (3.7, it appears?).

I think the kernel special-cases process_vm_readv so it doesn't need ptrace permissions when reading the calling process's address space. When mm == current->mm, the kernel skips the ptrace_may_access call:

• https://github.com/torvalds/linux/blob/v5.17/mm/process_vm_access.c#L202
• https://github.com/torvalds/linux/blob/v5.17/kernel/fork.c#L1324

LGTM after changing to an error with a warning downgrade. Resigning to clear my queue, plus the Apple folks should get the final accept here :)

IIUC this only affects IDE project generation. This makes sense to me, though I'll give others a few days to take a look as well in case they have any thoughts on the organization.

Commandeering to abandon, since this landed elsewhere

Clearing my queue.

Is this still relevant? Requesting changes to clear my queue.

Sorry, this fell off my radar! Your reasoning makes sense to me, and in particular, I'd forgotten that only compiler-t has the Darwin multi-arch setup (and you'd still need to handle libc++/libc++abi/libunwind differently).

Requesting changes since there are unaddressed reviewer questions.

While we're figuring this out ... @ldionne, @phosek, do the runtimes build changes (https://libcxx.llvm.org/BuildingLibcxx.html) also mean that we assume a full LLVM checkout is present? If so, we could move this header to LLVM and have both libunwind and LLD use it from there, which would sidestep the issue here.

(clearing my queue while there are unaddressed comments)

I'm not really sure about the ".md being more standard than .txt" in the summary, but I guess it's more standard with Github (where Markdown gets nice rendering), so this makes sense.

If I check out this commit and then check out the previous commit, clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp and clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected become modified in my working directory; their line endings are changed from CRLF to LF. That seems undesirable.

In D124563#3478623, @aaronpuchert wrote:

So maybe we should simply remove

# These test input files rely on two-byte Windows (CRLF) line endings.
clang-apply-replacements/Inputs/crlf/crlf.cpp text eol=crlf
clang-apply-replacements/Inputs/crlf/crlf.cpp.expected text eol=crlf

from the .gitattributes file again. Then we can keep them stored with CRLF endings, and each file has a comment anyway that they rely on them. The files have been there for many years and never been a problem.

The following files have their line endings (when checked out on disk) changed from CRLF to LF by this patch. Seems harmless, but I just wanted to confirm that it was expected:

In rGac5f7be6a8688955a282becf00eebc542238a86b#1080897, @Meinersbur wrote:

Since this commit, no matter what I do, crlf.cpp and crlf.cpp.expected show up as modified. No git checkout --force nor git reset --hard changes this. git config.autocrlf is false and the files indeed have CRLF endings. git check-attr -a confirms the attributes are set (text: set eol: crlf).

That is, I have two checkouts, different versions of git showing different modified status.

Showing modified in checkout A, but not in checkout B:

• git --version: git version 2.35.1 (from MSYS2)
• git --version: git version 2.35.2.windows.1 (shipping with Visual Studio 2022)

Showing modified in checkout B, but not in checkout A:

• git --version: git version 2.25.1 (from Ubuntu 22.04, running under WSL1)

In D124557#3478468, @bc-lee wrote:

It seems that codes for tvos simulator also needs to be modified.

Adjust tvos simulator architecture detection as well

In D124433#3476160, @MaskRay wrote:

in order for the wrapping logic to correctly set referencedAfterWrap and prevent LTO from eliminating them.

Perhaps elaborate which symbol's referencedAfterWrap is set (sym's).

Address review comments

I don't think many people are gonna notice a warning in the general spew of CMake configuration, tbh. Would it be too disruptive to make this an error instead and have a CMake option you can pass to get rid of the error?

In D123428#3444212, @MaskRay wrote:

In D123428#3444198, @rprichard wrote:

In D123428#3444170, @smeenai wrote:

In D123428#3444134, @rprichard wrote:

Using /proc/self/maps would be subject to TOCTOU, but I think most methods wouldn't, e.g.:

• Open /proc/self/mem and pread() the address. This seems strictly better than /proc/self/maps?
• Create a pipe using pipe(), write() the bytes into the pipe buffer and read() them back out. I believe a Linux pipe buffer is guaranteed to be big enough (>= 8 bytes).
• process_vm_readv

I wonder if security configurations are a problem. Maybe I should experiment on an Android build.

With all those methods, there's still the chance (however unlikely) that the address is readable at the time of the check but somehow becomes unreadable by the time we perform the actual read, right? I doubt it matters much in practice though.

The unwinder needs to read two aligned 4-byte words at the target PC. I was thinking that it could replace the ordinary reads with system calls. e.g. For the pipe trick, write the target 8 bytes into a pipe and then read them back out. If multiple threads share a pipe fd, then they'd need the synchronize their use of the pipe.

I wonder if we'd need to close the pipe fd for dlclose. On Android, the unwinder is linked statically into app shared objects.

For the pipe, since we'll be checking for validity multiple times, I imagine we'll need to empty out the buffer at some point. libunwind seems to use mincore if available and the pipe as a fallback, but I'm not really understanding how mincore would work here, since a page could be readable even though it's not currently resident.

Yeah, mincore doesn't seem to do what's needed here.

https://github.com/abseil/abseil-cpp/blob/master/absl/debugging/internal/address_is_readable.cc has gone through several iterations. We can use rt_sigprocmask.

In D124315#3469458, @tstellar wrote:

In D124315#3469449, @smeenai wrote:

To be clear, this is to support a build scenario where you only have the source tree under lld available?

Yes, that's correct.

To be clear, this is to support a build scenario where you only have the source tree under lld available?