2. Add a non-GC-root dummy section. This is a bit awkward to implement in MC because we have to pick a section name. The section may be combined with other normal sections with the same name. We need to be careful and not cause an "incompatible section flags" error (lld specific).

LGTM

If we just want a quick fix then I would prefer if we consider strong hidden symbols to be non-preemptible. This just seems like it would cause our assembler to hide bugs of the sort that D72197 was intended to catch, while changing the behavior for hidden symbols would mean that we continue to catch those bugs.

Shouldn't we just implement support for the relocation instead?

In D72197#1825247, @MaskRay wrote:

In D72197#1825012, @pcc wrote:

It looks like this change caused us to start rejecting:

.thumb
.thumb_func
.globl foo
.hidden foo
foo:
adr lr, foo + 1

with:

test.s:6:1: error: unsupported relocation on symbol
adr lr, foo + 1
^

This is exposed by this Android ART code: https://cs.android.com/android/platform/superproject/+/master:art/runtime/arch/arm/quick_entrypoints_arm.S;l=1826

I see a couple of possible fixes for this:

1. We could go back to the previous behaviour for global hidden symbols.
2. We could teach MC and LLD about the R_ARM_THM_ALU_PREL_11_0 relocation required to relocate this instruction. Arguably the ART code shouldn't be adding the 1 for Thumb here (because R_ARM_THM_ALU_PREL_11_0 adds the 1 itself), so ART would then need to be fixed.

foo is a hidden definition in the current object file. Can the ART code be changed to use a local symbol (adr lr, .Lfoo) instead?

In D65653#1825141, @efriedma wrote:

Exception handling has to use a DWARF unwinder, not a "fast" unwinder. So the "fast" unwinder can't actually do anything with the address other than print it out.

In D72197#1825070, @MaskRay wrote:

In D72197#1825012, @pcc wrote:

It looks like this change caused us to start rejecting:

.thumb
.thumb_func
.globl foo
.hidden foo
foo:
adr lr, foo + 1

with:

test.s:6:1: error: unsupported relocation on symbol
adr lr, foo + 1
^

This is exposed by this Android ART code: https://cs.android.com/android/platform/superproject/+/master:art/runtime/arch/arm/quick_entrypoints_arm.S;l=1826

I see a couple of possible fixes for this:

1. We could go back to the previous behaviour for global hidden symbols.
2. We could teach MC and LLD about the R_ARM_THM_ALU_PREL_11_0 relocation required to relocate this instruction. Arguably the ART code shouldn't be adding the 1 for Thumb here (because R_ARM_THM_ALU_PREL_11_0 adds the 1 itself), so ART would then need to be fixed.

% arm-linux-gnueabi-as /tmp/c/a.s -o /tmp/c/a.o
/tmp/c/a.s: Assembler messages:
/tmp/c/a.s:6: Error: invalid Hi register with immediate
/tmp/c/a.s:6: Error: invalid immediate for address calculation (value = 0xFFFFFFFFFFFFFFFE)

How can I make GNU as recognize it?

It looks like this change caused us to start rejecting:

.thumb
.thumb_func
.globl foo
.hidden foo
foo:
adr lr, foo + 1

with:

test.s:6:1: error: unsupported relocation on symbol
adr lr, foo + 1
^

• Address review comments

And I verified that this patch works on Fuchsia (x64 and arm64).

So I believe that after this change, we are no longer guaranteed that FP + 16 = SP at exit, correct? I think this would be a requirement for a "fast" unwinder that works by following frame pointers (rather than reading unwind info) and is capable of decrypting return addresses of functions built with -msign-return-addresses (because the AUT instruction would need to know SP at exit). If this is the case, would it be feasible to let a platform opt out of this behaviour? (I realise that Darwin is opted out, but I don't know about the SVE support status there.) I'm not very familiar with SVE, but I would imagine that you could use a different register for this purpose on opted out platforms.

ebase

In D72701#1823025, @sidneym wrote:

In D72701#1822556, @pcc wrote:

I know almost nothing about Hexagon, but shouldn't this be determined by the target triple, or at least a target-feature?

When the target is Linux the clang driver will add this option.

I know almost nothing about Hexagon, but shouldn't this be determined by the target triple, or at least a target-feature?

LGTM

In D71795#1818555, @MaskRay wrote:

In D71795#1818468, @pcc wrote:

Weak undefined symbols are preemptible after D71794.

What about hidden weak undefined symbols? We now reject the following input:

 cat foo.cpp
 __attribute__((weak, visibility("hidden"))) void f();
 
 void g() {
   if (f) f();
 }
 $ clang -shared -fuse-ld=lld foo.cpp
 ld.lld: error: relocation R_X86_64_PLT32 cannot refer to absolute symbol: f()
>>> defined in /tmp/foo-014d89.o
>>> referenced by foo.cpp
>>>               /tmp/foo-014d89.o:(g())
 clang: error: linker command failed with exit code 1 (use -v to see invocation)

This seems like a reasonable thing to want to do. I actually hit this problem in a slightly different case (--exclude-libs demoted the weak undef to hidden) which we could fix like so:

diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 19598f52cd4..54c033303f6 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -1374,7 +1374,7 @@ static void excludeLibs(opt::InputArgList &args) {
     if (!file->archiveName.empty())
       if (all || libs.count(path::filename(file->archiveName)))
         for (Symbol *sym : file->getSymbols())
-          if (!sym->isLocal() && sym->file == file)
+          if (!sym->isUndefined() && !sym->isLocal() && sym->file == file)
             sym->versionId = VER_NDX_LOCAL;
   };

But I'm somewhat more concerned about the explicit hidden case.

This patch looks good. Only defined symbols should have versions. I think the logic will also be closer to GNU ld:

// bfd/elflink.c
	  /* If this symbol has default visibility and the user has
	     requested we not re-export it, then mark it as hidden.  */
	  if (!bfd_is_und_section (sec)
	      && !dynamic
	      && abfd->no_export
	      && ELF_ST_VISIBILITY (isym->st_other) != STV_INTERNAL)
	    isym->st_other = (STV_HIDDEN
			      | (isym->st_other & ~ELF_ST_VISIBILITY (-1)));

If you haven't started working on this yet, I can send a patch.

Weak undefined symbols are preemptible after D71794.

LGTM

• Add memory clobber

This is going in the wrong direction IMO. We already have a way of specifying the size optimization level, which is to specify -Os or -Oz at compile time. Why is that not sufficient?

• Address review comments

• Formatting

LGTM

• Move tag check disablement to the caller

• Only load tags if useMemoryTagging()

• Load tags in malloc_iterate

• Address review comments

• Address review comments

LGTM

• Address review comments

The code now implements UAF checks by retagging on free.

Relanding with a fix for the problem found by @davezarzycki. The test needed to be adjusted to set --sysroot, otherwise it will fail in the case where a directory named /usr/include/c++/v1 or /usr/local/include/c++/v1 exists.

• Address review comments

In D71104#1773289, @cryptoad wrote:

Could you please put a comment somewhere on how to use the benchmark?

In D71131#1773161, @thakis wrote:

Thanks! lg.

I'd probably find it easier to understand, or at least more self-consistent, if this file had an assert(is_linux || is_fuchsia), and llvm/utils/gn/secondary/BUILD.gn had just the dep on //compiler-rt it already had, and the supported_toolchain stuff in this file wasn't here and instead relied on secondary/compiler-rt/BUILD.gn, and secondary/compiler-rt/lib/BUILD.gn would add a dep to scudo only if target_os == linux || fuchsia.

In D71081#1773044, @pcc wrote:

Yes, looking through the code as well as the CMake [1], scudo only supports building on Linux, Android and Fuchsia. Most of the code in linux.cpp looks like it would work on non-Linux POSIX, except for the mutex implementation.

It looks like the non-standalone allocator had some level of support for Windows [2] but that wasn't carried over to the standalone allocator.

So we should probably change the blacklist in the supported platforms to a whitelist for now. I'll go ahead and do that.

[1] http://llvm-cs.pcc.me.uk/projects/compiler-rt/cmake/config-ix.cmake#695
[2] http://llvm-cs.pcc.me.uk/projects/compiler-rt/lib/scudo/scudo_allocator_secondary.h#110

Yes, looking through the code as well as the CMake [1], scudo only supports building on Linux, Android and Fuchsia. Most of the code in linux.cpp looks like it would work on non-Linux POSIX, except for the mutex implementation.

• license header

• Silence sync_source_lists_from_cmake.py

• gn format