Agreed that this should probably be done in the mangler, I'll try to take a look next week.

LGTM

Doesn't this require a wrapper for mallinfo2() as well that returns a struct mallinfo2?

In D108479#3129577, @rjmccall wrote:

In D108479#3129571, @jrtc27 wrote:

For CHERI there's the added complication that descriptors and trampolines can exist for security reasons when crossing security domains, and you absolutely should not let one compartment get pointers to the entry point of another compartment's function. You can hand it out if sealed or the permissions are cleared, as then you can't really do anything with it other than look at the integer address, but that seems a bit odd.

That would be consistent with getting an unsigned pointer under pointer authentication or an address with the THUMB bit potentially stripped: it's just a raw address that you can't safely call. In any case, I suspect it would be fine for us to say that we just don't support this builtin when the target is using weird function pointers unless we have some way to bypass the special treatment in LLVM.

I agree that "symbol" address is probably the wrong name. Maybe __builtin_function_start or something like that? But before we go much further on this, we should get confirmation from Peter that we're targeting the right design.

Please see D63976 where we rejected a similar change in favor of just letting this be controllable at compile time.

LGTM

LGTM

In D113220#3122916, @morehouse wrote:

In D113220#3122771, @pcc wrote:

I think it ought to be possible to write these non-relaxable relocations from assembly, so that the behavior with -c is equivalent to the behavior with -S and then assembling the output. For example, you should be able to assemble

mov foo@gotpcrel_norelax(%rip), %rax

and the -S output from the compiler should look like that as well.

Won't this cause incompatibility with all other assemblers?

I think it ought to be possible to write these non-relaxable relocations from assembly, so that the behavior with -c is equivalent to the behavior with -S and then assembling the output. For example, you should be able to assemble

mov foo@gotpcrel_norelax(%rip), %rax

and the -S output from the compiler should look like that as well.

I don't think we can use llvm_code for this because we do cross-compile other parts of LLVM for Android (llvm-symbolizer, lldb-server).

(Adding back @rsmith, @rjmccall.)

Maybe it should even be semantically restricted to require a constant decl reference as its operand?

In D108479#3108360, @ardb wrote:

I would argue that the existing __builtin_addressof() should absorb this behavior, rather than adding a special builtin related to CFI.

As is documented for __builtin_addressof(), its intended use is in cases where the & operator may return something other than the address of the object, which is exactly the case we are dealing with here.

In D112870#3098512, @kubamracek wrote:

Shouldn't this be fixed in FunctionComparator?

So that's certainly the right question here. Should it? To me it looks like FunctionComparator is ignoring metadata attachments *on purpose*, and that fits the contract of "determine whether 2 functions will generate machine code with the same behavior".

I asked @samitolvanen out-of-band to check whether this really needs a flag since it seems like there could be some underlying issue that needs to be resolved so that we can do this unconditionally.

LGTM

Thanks, something like this seems reasonable to me and I don't have any major issues with it.

LGTM

LGTM

s/TBI/the tagged address ABI/ in the commit message.

LGTM

In D110888#3035297, @leonardchan wrote:

In D110888#3035193, @pcc wrote:

Please remember to upload patches with context: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

The tests in MemtagTests are meant to be skipped if the hardware doesn't support MTE (see the top of MemtagTest::SetUp()). Is that not working for you?

I don't think you should normally need to be defining SCUDO_DISABLE_TBI because it's controlled by the operating system, and all arm64 Linux systems that we normally care about support TBI. I figured that you were maybe trying to run these tests on Fuchsia, but then I noticed that memtag_test.cpp is wrapped in an #if SCUDO_LINUX, so I'm not sure why you're running into an issue. Are you trying to run these tests on arm64 non-Android Linux? Maybe the GTEST_SKIP() isn't working correctly for you for some reason?

I'm attempting to run host-linux runtimes tests. We weren't running them before and this is the first time we're trying it. We just happen to run into these test failures when turning them on before. But I think I found out the underlying issue: prctl(PR_GET_TAGGED_ADDR_CTRL, 0, 0, 0, 0) (in systemDetectsMemoryTagFaultsTestOnly) can return -1 if TBI isn't supported according to https://man7.org/linux/man-pages/man2/prctl.2.html, and this seems to be the case for us. The -1 is cast to an unsigned long and masked, then systemDetectsMemoryTagFaultsTestOnly ends up returning true when comparing a non-zero value after masking. I think the proper fix might be to just account for the -1 result. I'll update the patch later.

Please remember to upload patches with context: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

In D108741#3026318, @kubamracek wrote:

Sorry, I think I managed to confuse myself when I was trying to figure out what VFE actually relies on. It seems that VFE doesn't rely on what I thought it did. Although VFE relies on type metadata, it only relies on it to determine the locations of the address points, and not the locations of the VFE-eligible function slots.

@pcc, I apologize I think I got confused the same way. I think I had the wrong understanding that if any slot in a vtable does not have a matching !type annotation (on the vtable global) with the right offset, it's basically trivially removable by VFE, but apparently, as you're pointing out, that's not the case -- it could still be used by a non-zero offset load via llvm.type.checked.load. In such a case, the slot at (vcall offset + vtable annotation offset) is actually being matched. So even if a vtable slot at offset X is never mentioned in any !type annotation, it could still be matched by any preceding !type type id with a non-zero offset at the call site.

Could you confirm that this understanding I just mentioned is actually correct now? Thanks :)

Here's one alternative proposal: can we make it so that any vtable slots that are eligible for VFE are represented using a new type of Constant (similar to dso_local_equivalent)? This also has the advantage that frontends could also use it for global variables as well as functions.

Just to make sure I understand this proposal, would we *require* frontends to mark all VFE eligible slots with this? I assume the only existing (publicly known) user of VFE is Clang, so as part of the change we'd fix Clang too? And the way this would look on a simple example would be something like the following?

@vtable = internal unnamed_addr constant ... {[
  i8* bitcast (void ()* vfe_eligible @vfunc1 to i8*),
  i8* bitcast (void ()* vfe_eligible @vfunc2 to i8*),
  i8* bitcast (void ()* @non_eligible to i8*)
]}, align 8, !type ...

And in the case of "relative pointers" that Swift uses in its data structures, the Constant would be present at the inner-most reference, like this?

@vtable = internal unnamed_addr constant { [2 x i32] } { [2 x i32] [
  i32 trunc (i64 sub (i64 ptrtoint (void ()* vfe_eligible @vfunc1 to i64), i64 ptrtoint ({ [2 x i32] }* @vtable to i64)) to i32),
  i32 trunc (i64 sub (i64 ptrtoint (void ()* vfe_eligible @vfunc2 to i64), i64 ptrtoint ({ [2 x i32] }* @vtable to i64)) to i32)
]}, align 8, !type ...

In D108741#3023267, @fhahn wrote:

In D108741#3021933, @pcc wrote:

In D108741#3021793, @kubamracek wrote:

@pcc, could I ask for some details about your last comment? I'm specifically wondering whether you're against proceeding with the change from this review.

Sorry, but I think I'm against it because the mechanism that you've chosen seems like an abuse of the !type metadata, since the metadata is meant to be used as a reference point for accesses relative to it, and not as a way of marking up specific fields by itself. As a more practical concern, it has no way of representing the situation where the pointer at the address point is not eligible for VFE.

Here's one alternative proposal: can we make it so that any vtable slots that are eligible for VFE are represented using a new type of Constant (similar to dso_local_equivalent)? This also has the advantage that frontends could also use it for global variables as well as functions.

A potentially more lightweight solution may be providing a way to opt-in to the stricter interpretation. That way, we ensure no existing users are disturbed while not having to add more invasive changes. Do you think the additional complexity a new type of Constant carries its weight if it is only used to support the swift VFE case?

In D108741#3011080, @kubamracek wrote:

It isn't great that VFE apparently relies on the type metadata that we added to support member function pointer CFI in C++. I was unaware that this was the case, and it shouldn't be necessary for frontends that don't have something like C++'s peculiar member function pointer ABI (I assume that Swift doesn't?) It would be better if we designed an independent way of marking these entries as subject to VFE.

As far as I can tell, VFE was added via https://reviews.llvm.org/D63932 and it has since always used type metadata and the !type annotations on vtables. Are you asking for a reversal on how VFE is done at the IR level, including how it's done for C++ in Clang? I'd love to see more thought on that (and what exactly do you think the design should aim to do better), but it seems to me that that's out of scope for this particular small tweak that I'm pursuing.

In D108741#3005803, @fhahn wrote:

In D108741#2998705, @tejohnson wrote:

In D108741#2997844, @fhahn wrote:

As part of codesize optimization work for Swift, we'd like to add Virtual Function Elimination to Swift, very similarly to how GlobalDCE supports C++ VFE.

I think it would be good to drop this part from the description, as the change is not really related to swift; it just changes the code to ignore entries at slots without !type.

It seems sensible to me to ignore entries without corresponding !type metadata and this should be in line with the specification in langref. @pcc, @tejohnson Are there any issues you could think of?

Is this specified in the langref? I looked through the type metadata documentation but don't see it mentioned there that all vfuncs will have type metadata. Looks like that was added a bit later than the original type metadata, in D47567. I think what is specified in langref is that it has type metadata for the address point. Probably the documentation needs updating. But from what I can see, it looks like this should always be emitted now. @pcc ?

Thanks for checking! I think the update to the langref in the latest version should spell out things clearly.

Looks like this change caused this crash, can you please take a look?

$ cat test.ii
class a {
public:
  enum b {};
  struct c {
    b j;
    int *d;
    void *e;
  };
};
class f {
  bool operator==(const f &) const;
  a::c g() const;
};
bool f::operator==(const f &) const {
  a::c h = g(), i = g();
  return h.e == i.e && h.d == i.d && h.j == i.j;
}
$ clang++ -c  -o test.o test.ii -O2
Instruction does not dominate all uses!
  %h = alloca %"struct.a::c", align 8
  %1 = bitcast %"struct.a::c"* %h to i32*
Instruction does not dominate all uses!
  %i = alloca %"struct.a::c", align 8
  %2 = bitcast %"struct.a::c"* %i to i32*
Instruction does not dominate all uses!
  %i = alloca %"struct.a::c", align 8
  %18 = bitcast %"struct.a::c"* %i to i8*
Instruction does not dominate all uses!
  %h = alloca %"struct.a::c", align 8
  %19 = bitcast %"struct.a::c"* %h to i8*
in function _ZNK1feqERKS_
fatal error: error in backend: Broken function found, compilation aborted!

I'm not sure this should be considered an alternative to D69607. Maybe it is if you're not using --gc-sections, but with that flag we really need something like a breadth-first search like in that patch.

LGTM

Move to data_deps

Move to data_deps

In D103127#2985203, @labath wrote:

How exactly are you building this? CMAKE_SYSTEM_NAME is set in the official android cmake toolchain file (https://android.googlesource.com/platform/ndk/+/refs/heads/ndk-release-r23/build/cmake/android.toolchain.cmake#213)

Have you verified that things work at runtime with this change? Because I believe that your change will result in branching to the GOT entry for the runtime functions. Since GOT entries are just addresses and are not executable, I believe that this would result in a segfault.

In D109393#2987973, @kstoimenov wrote:

In D109393#2987961, @pcc wrote:

What's the effect on the relocation types in the object file with this change? I don't think it's valid to have a branch with an @GOT modifier.

Moving from PTL to GOT fixed this errors like this: "error: 32 bit reloc applied to a field with a different size". What do you thing should be the right type for this? Not specifying PTL also had problems with linking so living in blank is not an option.

What's the effect on the relocation types in the object file with this change? I don't think it's valid to have a branch with an @GOT modifier.

In D109185#2987403, @thakis wrote:

• rebase
• add -sectcreate flags for embedding Info.plist files into lldb and lldb-vscode

This is probably in shape enough to iterate in in-tree. I didn't squash in your linux change; let's land that separately :)

Thanks for working on this!

Oh right, I missed that you were emitting the function locally. So then replace "saves a PLT slot" with "saves emitting a trap function".

Maybe you can set the relative offset to 0 in this case? That way, the crashing PC will point to the vtable, which should make it easier to track down the source of any problems. This would also require no runtime support (so doesn't need to be behind a flag) and also saves a PLT slot if the trap function would be in another DSO.

Could we perhaps put something in the binary if it was linked with --no-undefined (e.g. a dynamic table entry)? Then if we're linking against such a binary, ignore any undefined symbols.

I believe that Chromium uses it (or at least it did at the time that I added this, and Chromium has since switched to using libc++ on Windows).

Hi David, it looks like this change caused an assertion failure. Can you please take a look?

> cat test.i
int a;
double b, c;
void d() {
  for (; a; a++) {
    b += c;
    c = a;
  }
}
> clang -O2 test.i --target=aarch64-linux
clang: ../llvm/lib/Transforms/Vectorize/VPlanValue.h:186: llvm::Value *llvm::VPValue::getLiveInIRValue(): Assertion `!getDef() && "VPValue is not a live-in; it is defined by a VPDef inside a VPlan"' failed.

Test this in test/Analysis/StackSafetyAnalysis/lifetime.ll instead

Also libLTO.so

LGTM

LGTM

Note that this is the legacy copy of Scudo. Looks like scudo/standalone has the same problem (see ScopedString::Append and Printf in compiler-rt/lib/scudo/standalone/string_utils.cpp).

0 means when any GV in the third element is alive, mark the first element as used.

LGTM

LGTM

LGTM

Programs must enable the tagged address ABI to
receive these signals and are also opting into the
presence of these tag bits.

LGTM

Yeah, that should be good enough for now. It won't cover users who have the integrated assembler disabled, but it's better than nothing.

This is a known issue with the use of MTE instructions in memtag.h -- they should really be conditional on whether the assembler supports them. For now, please upgrade your assembler.

LGTM

In D105261#2853749, @vitalybuka wrote:

In D105261#2853736, @vitalybuka wrote:

In D105261#2853702, @pcc wrote:

Can we untag BlockEnd in reallocate instead?

Yes, but it looks a little bit inconsistent when untag End here

Can we untag BlockEnd in reallocate instead?

LGTM

LGTM