I believe that Android used to use these older versions of libgcc, however Android has now moved to libunwind on aarch64 and although old libgcc versions may still be linked into applications, I do not believe that Android platform code compiled with SCS will run on the same thread as application code, so the old libgcc versions should not be exposed to this unwind info.

LGTM

In D103626#2796554, @DavidSpickett wrote:

The side effect here is that you do "memory read <tagged ptr>" and you see untagged addresses for the lines. It's not really that confusing but maybe something we should make a general decision about.

Same thing applies to the previous "memory region" patch.

(lldb) p ptr1
(char *) $1 = 0x344dfffffffffcb0 ""
(lldb) memory read ptr1
0xfffffffffcb0: f0 05 40 00 00 00 00 00 00 00 00 00 00 00 00 00  ..@.............
0xfffffffffcc0: e0 fc ff ff ff ff 00 00 54 f0 e7 f7 ff ff 00 00  ........T.......
(lldb) memory region ptr1
[0x0000fffffffdf000-0x0001000000000000) rw- [stack]

Personally it doesn't bother me, but then again I know what all the upper bits are doing anyway.

LGTM

LGTM

Do we still need the DSB on line 119?

Thanks for tracking this down. It seems best to change the Printf call to add the newline, since otherwise it looks like you'd be adding a spurious newline to the other callers of RenderFrame on Fuchsia.

LGTM

In D103845#2806211, @leonardchan wrote:

In D103845#2804441, @pcc wrote:

This isn't how the output looks on Android. Are you sure this isn't a Fuchsia-specific bug in the output formatting?

I think the newline gets added to the frame description only if Symbolizer::GetOrInit()->SymbolizePC(pc) is nonnull, so if it fails then no newline is added.

This isn't how the output looks on Android. Are you sure this isn't a Fuchsia-specific bug in the output formatting?

LGTM

LGTM

LGTM

LGTM

LGTM

There are probably going to be more callers of selectRandomTag and I don't think we want to have to deal with tag 0 in any of them. Could we just or 1 into the mask in selectRandomTag?

TBH, I'm not sure it's worth going to this much effort to work around this QEMU bug. It seems better to figure out a way to use QEMU in system mode instead.

Isn't the bug here that module hashing is using hash_code? So shouldn't the correct fix be to use a specific hashing algorithm for module hashes?

Which version of Clang are you using? I'm reasonably sure that we're building libunwind on Android with Clang, and the pointer authentication code works there.

It doesn't look like this was reviewed by anyone. Could you revert this please until it gets reviewed?

In D102716#2767104, @hctim wrote:

It's funny you mention this, I think this is one of the problems that I saw in https://reviews.llvm.org/D102543#change-dE7v8cbuK8l4 (but there are other bugs also in realloc.cpp that need to be addressed as well). Is the fix already merged into the tree or yet to come?

This warning seems to have a lot of false positives on things like reference arguments that are used as output parameters. For example here is a small sample of output from a stage2 build of part of LLVM:

In file included from ../llvm/lib/BinaryFormat/Minidump.cpp:9:
In file included from ../llvm/include/llvm/BinaryFormat/Minidump.h:21:
In file included from ../llvm/include/llvm/ADT/BitmaskEnum.h:16:
../llvm/include/llvm/Support/MathExtras.h:822:9: warning: variable 'Overflowed' set but not used [-Wunused-but-set-variable]
  bool &Overflowed = ResultOverflowed ? *ResultOverflowed : Dummy;
        ^
../llvm/include/llvm/Support/MathExtras.h:936:72: warning: parameter 'Result' set but not used [-Wunused-but-set-parameter]
std::enable_if_t<std::is_signed<T>::value, T> MulOverflow(T X, T Y, T &Result) {
                                                                       ^
In file included from ../llvm/lib/BinaryFormat/Minidump.cpp:9:
In file included from ../llvm/include/llvm/BinaryFormat/Minidump.h:22:
In file included from ../llvm/include/llvm/ADT/DenseMapInfo.h:20:
../llvm/include/llvm/ADT/StringRef.h:511:37: warning: parameter 'Result' set but not used [-Wunused-but-set-parameter]
    getAsInteger(unsigned Radix, T &Result) const {
                                    ^
../llvm/include/llvm/ADT/StringRef.h:522:37: warning: parameter 'Result' set but not used [-Wunused-but-set-parameter]
    getAsInteger(unsigned Radix, T &Result) const {
                                    ^
../llvm/include/llvm/ADT/StringRef.h:545:39: warning: parameter 'Result' set but not used [-Wunused-but-set-parameter]
    consumeInteger(unsigned Radix, T &Result) {
                                      ^
../llvm/include/llvm/ADT/StringRef.h:556:39: warning: parameter 'Result' set but not used [-Wunused-but-set-parameter]
    consumeInteger(unsigned Radix, T &Result) {
                                      ^
6 warnings generated.

Could you please take a look?

• Add comments

This looks like a reasonable starting point for switching to opaque pointers in LLVM.

I'm not sure that we can do this until all of our minimum compiler versions support C++17. E.g. minimum gcc version is 5.1.0 which was released in 2015, so it presumably doesn't have C++17 support.

Have you reported this bug to QEMU? Unfortunately the comment you linked to is inaccurate about madvise being a hint on Linux [1].

In D101105#2710053, @eugenis wrote:

Nice.
If this is a significant optimization, we might want to request it through the allocator config and fail if it is impossible - in case we change the size class map in the future and break this by accident.

Fix build

Wouldn't this make all address significant symbols weak external?

It seems that even with the LLVM assembler the ; character isn't understood everywhere. For example at one point I noticed that ; is treated as a comment character on Mach-O targets, which caused the following instructions to silently disappear. I think we can reasonably count on newline being understood as a delimiter everywhere.

In D99240#2684236, @rnk wrote:

I think Arthur is right, it should be safe to look through local aliases to local linkage globals. I think we can actually do this in the "for alias analysis" variant of strip pointer casts, and we should consider that.

In D99240#2651342, @aeubanks wrote:

Ah I see, https://github.com/llvm/llvm-project/blob/61a55c8812e790842799ba1de5bd81fe8afb3b16/llvm/lib/Transforms/IPO/GlobalOpt.cpp#L2953. It only works if the aliasee is a global value. Extending globalopt to handle aliasees that may be a constant expression containing a global value, rather than just a global value, would probably work as well. Either way is fine by me.

globalopt specifically deletes aliases when possible, so it seems fair to do that here as well, even if that may remove entries from the symbol table.

In D99240#2650901, @aeubanks wrote:

In D99240#2649567, @pcc wrote:

Can you make it so that the load ends up being replaced with the loaded value, without replacing the GEP itself? That would seem to address the MSVC issue.

What's the benefit of that over this approach?

Can you make it so that the load ends up being replaced with the loaded value, without replacing the GEP itself? That would seem to address the MSVC issue.

In D98529#2631946, @DavidSpickett wrote:

TCR_ELx begins at 1 (see D13.2.123 TCR_EL1, Translation Control Register (EL1) in the armarm) and covers EL0 and 1. Looking at the pseudocode access to this is undefined at EL0.

So the OS would have to provide you some other way to read that, I know this is missing on Linux at the moment. A user setting might be good for us supporting Linux too. (though I realise your immediate concern is MacOS)