Page MenuHomePhabricator

mcgrathr (Roland McGrath)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 31 2017, 4:46 PM (194 w, 2 d)

Recent Activity

Thu, Oct 8

mcgrathr added inline comments to D77606: [libcxxabi] Add macro for changing functions to support the relative vtables ABI.
Thu, Oct 8, 4:27 PM · Restricted Project

Fri, Oct 2

mcgrathr added a reverting change for rG1c897e9d7297: [lsan] Share platform allocator settings between ASan and LSan: rG5b0cfe93b6cd: Revert "[lsan] Share platform allocator settings between ASan and LSan".
Fri, Oct 2, 6:15 PM
mcgrathr committed rG5b0cfe93b6cd: Revert "[lsan] Share platform allocator settings between ASan and LSan" (authored by mcgrathr).
Revert "[lsan] Share platform allocator settings between ASan and LSan"
Fri, Oct 2, 6:15 PM
mcgrathr added a reverting change for D87795: [lsan] Share platform allocator settings between ASan and LSan: rG5b0cfe93b6cd: Revert "[lsan] Share platform allocator settings between ASan and LSan".
Fri, Oct 2, 6:15 PM · Restricted Project
mcgrathr closed D88768: Revert "[lsan] Share platform allocator settings between ASan and LSan".
Fri, Oct 2, 6:15 PM · Restricted Project
mcgrathr added a reverting change for rG1c897e9d7297: [lsan] Share platform allocator settings between ASan and LSan: D88768: Revert "[lsan] Share platform allocator settings between ASan and LSan".
Fri, Oct 2, 6:14 PM
mcgrathr requested review of D88768: Revert "[lsan] Share platform allocator settings between ASan and LSan".
Fri, Oct 2, 6:13 PM · Restricted Project
mcgrathr added a reverting change for D87795: [lsan] Share platform allocator settings between ASan and LSan: D88768: Revert "[lsan] Share platform allocator settings between ASan and LSan".
Fri, Oct 2, 6:13 PM · Restricted Project
mcgrathr committed rG1c897e9d7297: [lsan] Share platform allocator settings between ASan and LSan (authored by mcgrathr).
[lsan] Share platform allocator settings between ASan and LSan
Fri, Oct 2, 5:56 PM
mcgrathr closed D87795: [lsan] Share platform allocator settings between ASan and LSan.
Fri, Oct 2, 5:55 PM · Restricted Project

Tue, Sep 29

mcgrathr added a comment to D88457: [scudo][standalone] Fix some primary tests.

Two possible approaches to make this kind of problem less likely to come up again:

  1. don't default the template args. might be annoying for this template.
  2. replace bool with an enum class NamedForPurpose bool : { kThis = false, kThat = true }; in the template argument type
Tue, Sep 29, 11:17 AM · Restricted Project

Mon, Sep 28

mcgrathr accepted D88443: [scudo][standalone] Remove unused atomic_compare_exchange_weak.

lgtm

Mon, Sep 28, 2:09 PM · Restricted Project

Fri, Sep 25

mcgrathr added inline comments to D88248: [lsan] On Fuchsia, don't use atexit hook for leak checks.
Fri, Sep 25, 12:13 PM · Restricted Project
mcgrathr updated the diff for D88248: [lsan] On Fuchsia, don't use atexit hook for leak checks.

fix typo and missing #include

Fri, Sep 25, 12:12 PM · Restricted Project

Thu, Sep 24

mcgrathr added a comment to D88248: [lsan] On Fuchsia, don't use atexit hook for leak checks.

This is the same as the reverted change except it always defines InstallAtExitCheckLeaks in asan_posix.cpp, just with an empty body on systems without lsan support.

Thu, Sep 24, 10:43 AM · Restricted Project
mcgrathr requested review of D88248: [lsan] On Fuchsia, don't use atexit hook for leak checks.
Thu, Sep 24, 10:42 AM · Restricted Project
mcgrathr added a comment to D87795: [lsan] Share platform allocator settings between ASan and LSan.

Since everyone approves can someone mark it approved?

Thu, Sep 24, 10:08 AM · Restricted Project

Wed, Sep 23

mcgrathr added a comment to D88184: [lsan] Add interceptor for pthread_detach..

LGTM but I'll leave approval to one of the maintainers.
Thanks for the fix!

Wed, Sep 23, 8:11 PM · Restricted Project
mcgrathr added reviewers for D88184: [lsan] Add interceptor for pthread_detach.: eugenis, vitalybuka.
Wed, Sep 23, 8:09 PM · Restricted Project
mcgrathr committed rGc96d0cceb684: asan: Use `#if` to test CAN_SANITIZE_LEAKS (authored by mcgrathr).
asan: Use `#if` to test CAN_SANITIZE_LEAKS
Wed, Sep 23, 11:59 AM
mcgrathr closed D88173: asan: Use `#if` to test CAN_SANITIZE_LEAKS.
Wed, Sep 23, 11:59 AM · Restricted Project
mcgrathr updated the diff for D88173: asan: Use `#if` to test CAN_SANITIZE_LEAKS.

fix comment typo in earlier change

Wed, Sep 23, 11:41 AM · Restricted Project
mcgrathr requested review of D88173: asan: Use `#if` to test CAN_SANITIZE_LEAKS.
Wed, Sep 23, 11:30 AM · Restricted Project
mcgrathr accepted D88170: [scudo][standalone] Fix tests under ASan/UBSan.

LGTM modulo clang-format. Thanks for the quick fixes!

Wed, Sep 23, 11:14 AM · Restricted Project
mcgrathr committed rG0caad9fe441d: [lsan] On Fuchsia, don't use atexit hook for leak checks (authored by mcgrathr).
[lsan] On Fuchsia, don't use atexit hook for leak checks
Wed, Sep 23, 11:11 AM
mcgrathr closed D86171: [lsan] On Fuchsia, don't use atexit hook for leak checks.
Wed, Sep 23, 11:11 AM · Restricted Project
mcgrathr added a comment to D87795: [lsan] Share platform allocator settings between ASan and LSan.

Please don’t do this. You’re adding a dependency between asan and lsan, and

This dependency already exists in asan_allocator.cpp

Also I think that duplication of these constants is not rely a problems and it's fine as-is without the patch.

Wed, Sep 23, 10:55 AM · Restricted Project
mcgrathr added a comment to D87795: [lsan] Share platform allocator settings between ASan and LSan.

Please don’t do this. You’re adding a dependency between asan and lsan, and
making it impossible to support platforms without lsan.

It’s a layering violation. I guess you can hack it if you add a *very
minimal * “allocator for asan and/or lsan” in sanitizer common. That
wouldn’t be ideal (a header shared between only two Sanitizers, but at
least it would be in the right place.

Wed, Sep 23, 10:52 AM · Restricted Project

Sep 22 2020

mcgrathr added a comment to D87795: [lsan] Share platform allocator settings between ASan and LSan.

I redid this to put it in the __lsan namespace in lsan_common.h, which is definitely simpler all around.

Sep 22 2020, 6:39 PM · Restricted Project
mcgrathr updated the diff for D87795: [lsan] Share platform allocator settings between ASan and LSan.

update

Sep 22 2020, 6:36 PM · Restricted Project
mcgrathr updated the diff for D87795: [lsan] Share platform allocator settings between ASan and LSan.

Moved to lsan_common.h.

Sep 22 2020, 6:36 PM · Restricted Project
mcgrathr updated the diff for D86171: [lsan] On Fuchsia, don't use atexit hook for leak checks.

fix typo in windows stub

Sep 22 2020, 6:03 PM · Restricted Project

Sep 19 2020

mcgrathr committed rGf5fa5b9fe3b0: [scudo/standalone] Fix undefined behavior in checksum test (authored by mcgrathr).
[scudo/standalone] Fix undefined behavior in checksum test
Sep 19 2020, 12:28 PM
mcgrathr closed D87973: [scudo/standalone] Fix undefined behavior in checksum test.
Sep 19 2020, 12:28 PM · Restricted Project
mcgrathr updated the diff for D87973: [scudo/standalone] Fix undefined behavior in checksum test.

formatting

Sep 19 2020, 11:45 AM · Restricted Project
mcgrathr requested review of D87973: [scudo/standalone] Fix undefined behavior in checksum test.
Sep 19 2020, 11:43 AM · Restricted Project

Sep 17 2020

mcgrathr updated the diff for D86171: [lsan] On Fuchsia, don't use atexit hook for leak checks.

add windows stub

Sep 17 2020, 5:53 PM · Restricted Project
mcgrathr abandoned D87793: [lsan] On Fuchsia, don't use atexit hook for leak checks.

bad upload

Sep 17 2020, 5:50 PM · Restricted Project
mcgrathr updated the diff for D87793: [lsan] On Fuchsia, don't use atexit hook for leak checks.

add windows stub

Sep 17 2020, 5:48 PM · Restricted Project
mcgrathr committed rG27f34540ea56: [scudo/standalone] Don't define test main function for Fuchsia (authored by mcgrathr).
[scudo/standalone] Don't define test main function for Fuchsia
Sep 17 2020, 5:43 PM
mcgrathr closed D87809: [scudo/standalone] Don't define test main function for Fuchsia.
Sep 17 2020, 5:43 PM · Restricted Project
mcgrathr added inline comments to D87809: [scudo/standalone] Don't define test main function for Fuchsia.
Sep 17 2020, 1:06 PM · Restricted Project
mcgrathr updated the diff for D87809: [scudo/standalone] Don't define test main function for Fuchsia.

update msg

Sep 17 2020, 1:05 PM · Restricted Project
mcgrathr updated the diff for D87809: [scudo/standalone] Don't define test main function for Fuchsia.

simplified

Sep 17 2020, 1:03 PM · Restricted Project

Sep 16 2020

mcgrathr updated the diff for D87795: [lsan] Share platform allocator settings between ASan and LSan.

rebase

Sep 16 2020, 6:38 PM · Restricted Project
mcgrathr added a comment to D87795: [lsan] Share platform allocator settings between ASan and LSan.

can you please clang format this?

Sep 16 2020, 6:26 PM · Restricted Project
mcgrathr requested review of D87809: [scudo/standalone] Don't define test main function for Fuchsia.
Sep 16 2020, 6:14 PM · Restricted Project
mcgrathr added reviewers for D87795: [lsan] Share platform allocator settings between ASan and LSan: cryptoad, phosek, eugenis, vitalybuka.
Sep 16 2020, 2:20 PM · Restricted Project
mcgrathr updated the diff for D86171: [lsan] On Fuchsia, don't use atexit hook for leak checks.

rebased

Sep 16 2020, 2:17 PM · Restricted Project
mcgrathr requested review of D87795: [lsan] Share platform allocator settings between ASan and LSan.
Sep 16 2020, 2:14 PM · Restricted Project
mcgrathr abandoned D87793: [lsan] On Fuchsia, don't use atexit hook for leak checks.

bad upload

Sep 16 2020, 2:12 PM · Restricted Project
mcgrathr requested review of D87793: [lsan] On Fuchsia, don't use atexit hook for leak checks.
Sep 16 2020, 2:06 PM · Restricted Project
mcgrathr added a comment to D86171: [lsan] On Fuchsia, don't use atexit hook for leak checks.

ping

Sep 16 2020, 1:18 PM · Restricted Project

Sep 9 2020

mcgrathr added a comment to D87420: scudo: Introduce a new mechanism to let Scudo access a platform-specific TLS slot.

Looks fine to me. Thanks!

Sep 9 2020, 6:59 PM · Restricted Project

Sep 8 2020

mcgrathr added inline comments to D86171: [lsan] On Fuchsia, don't use atexit hook for leak checks.
Sep 8 2020, 5:22 PM · Restricted Project
mcgrathr updated the diff for D86171: [lsan] On Fuchsia, don't use atexit hook for leak checks.

refactored per review

Sep 8 2020, 5:22 PM · Restricted Project

Aug 28 2020

mcgrathr accepted D86826: compiler-rt: Remove unnecesary typedefs in tsan_interface.

They don't appear anywhere in this file, and presumably if you wanted those types you should use <stdint.h> from the compiler anyway.

Aug 28 2020, 5:55 PM · Restricted Project
mcgrathr accepted D86822: [clang] Enable -fsanitize=thread on Fuchsia..

I'm not 100% sure we don't need more fiddles in the driver, but we can iterate from here.

Aug 28 2020, 4:19 PM · Restricted Project

Aug 27 2020

mcgrathr added a comment to D77248: [llvm][IR] Add dso_local_equivalent Constant.

Rather than "equivalent to the resolved function at runtime", I'd say "equivalent to the resolved function as a call target". It's not equivalent to the function at runtime when the runtime use is "ptr == &func".

Aug 27 2020, 4:44 PM · Restricted Project

Aug 19 2020

mcgrathr added a comment to D86175: [profile][Fuchsia] Use build ID for the raw profile name.

Even with the bugs fixed, I'm not very sanguine about truncating to 31 chars.

Aug 19 2020, 12:22 PM

Aug 18 2020

mcgrathr requested review of D86171: [lsan] On Fuchsia, don't use atexit hook for leak checks.
Aug 18 2020, 2:40 PM · Restricted Project

Aug 14 2020

mcgrathr retitled D85930: [lsan] Share platform allocator settings between ASan and LSan from [lsan] Follow SANITIZER_CAN_USE_ALLOCATOR64 for LSan allocator to [lsan] Share platform allocator settings between ASan and LSan.
Aug 14 2020, 5:37 PM · Restricted Project
mcgrathr added inline comments to D85930: [lsan] Share platform allocator settings between ASan and LSan.
Aug 14 2020, 3:02 PM · Restricted Project
mcgrathr updated the diff for D85930: [lsan] Share platform allocator settings between ASan and LSan.

Refactor to share code directly with asan.

Aug 14 2020, 3:01 PM · Restricted Project
mcgrathr accepted D85924: [clang][feature] Add cxx_abi_relative_vtable feature.

LGTM but I'm not sure who should sign off on new __has_feature symbols.

Aug 14 2020, 1:05 PM · Restricted Project, Restricted Project

Aug 13 2020

mcgrathr added inline comments to D85924: [clang][feature] Add cxx_abi_relative_vtable feature.
Aug 13 2020, 2:03 PM · Restricted Project, Restricted Project
mcgrathr added inline comments to D85924: [clang][feature] Add cxx_abi_relative_vtable feature.
Aug 13 2020, 1:49 PM · Restricted Project, Restricted Project
mcgrathr requested review of D85930: [lsan] Share platform allocator settings between ASan and LSan.
Aug 13 2020, 1:42 PM · Restricted Project

Aug 12 2020

mcgrathr added a comment to D77606: [libcxxabi] Add macro for changing functions to support the relative vtables ABI.

Long-term, I think we want this to be keyed entirely on a compiler predefine and not chosen in cmake.

Aug 12 2020, 6:03 PM · Restricted Project
mcgrathr added inline comments to D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use.
Aug 12 2020, 11:25 AM · Restricted Project

Aug 5 2020

mcgrathr accepted D85362: [CMake] Print the autodetected host linker version.

That's a great start. IMHO we should give the driver a way to print out its embedded value too.

Aug 5 2020, 2:12 PM · Restricted Project

Jul 30 2020

mcgrathr added a comment to D76482: [lld][ELF] Provide optional hidden symbols for build ID.

The proposed solution with an input linker script is a clever kludge, but it's a kludge. There's no way to make that actually general, since there is no particular named section that you can correctly assume is present in all kinds of binaries you might be linking. The kludge could be locally tailored for different cases, but that is even worse. Since the section and layout we're talking about here is entirely synthesized by the linker, it makes a lot more sense to have the linker support convenience features for using that format directly than to require users to build detailed kludges baking in the details of the linker's built-in synthesizing behavior.

Jul 30 2020, 1:40 PM · Restricted Project

Jul 28 2020

mcgrathr abandoned D79664: [ELF] Support --pack-dyn-relocs=rel+relr.

obviated by -z rel

Jul 28 2020, 10:40 AM · Restricted Project

Jul 23 2020

Herald added a project to D84482: [Fuchsia] Use -z dead-reloc-in-nonalloc=*=0 for Fuchsia targets: Restricted Project.
Jul 23 2020, 6:52 PM · Restricted Project

Jun 15 2020

mcgrathr added a comment to D55682: [ELF] Add -z start-stop-visibility= to set __start_/__stop_ symbol visibility.

Both GNU linkers now implement -z start-stop-visibility=... and binutils 2.35 will include that feature.

Jun 15 2020, 1:46 PM · Restricted Project

Jun 5 2020

mcgrathr added inline comments to D77647: [ELF][AArch64] Add R_AARCH64_PLT32 relocation type..
Jun 5 2020, 4:48 PM · Restricted Project

Jun 4 2020

mcgrathr added a comment to D81184: [lld][ELF][AArch64] Handle R_AARCH64_PLT32 relocation.

First implement the lld change with standalone tests covering all cases; that's one change.
Then do the assembly bits. As others have said, follow the x86 model where @plt is required to request this reloc type.
The assembler doesn't do its own logic based on the symbols to decide what reloc types to use. The assembly syntax has to request it directly.
When the compiler is producing a reference, it explicitly uses the symbol@plt syntax (or internal equivalent for integrated-as) when a PLT entry might be required.

Jun 4 2020, 7:51 PM · Restricted Project

May 29 2020

mcgrathr added a reviewer for D80605: [AddressSanitizer] Don't use weak linkage for __{start,stop}_asan_globals: Restricted Project.
May 29 2020, 1:40 PM · Restricted Project
mcgrathr updated the diff for D79835: [Fuchsia] Rely on linker switch rather than dead code ref for profile runtime.

comment update

May 29 2020, 1:06 PM · Restricted Project, Restricted Project
mcgrathr added a comment to D80496: [ELF] Add -z rel and -z rela.

LGTM. It might be worth mentioning in the doc/comment that REL format cannot pack the full range of addend values for all reloc types, but this only affects reloc types that lld doesn't support emitting as dynamic relocs since they are only used for TEXTRELs.

My understanding is that text relocations describes dynamic relocations in non-SHF_WRITE sections. I do not say text relocations because we disallow such dynamic relocations from SHF_WRITE sections as well.

May 29 2020, 12:35 PM · Restricted Project

May 28 2020

mcgrathr accepted D80496: [ELF] Add -z rel and -z rela.

LGTM. It might be worth mentioning in the doc/comment that REL format cannot pack the full range of addend values for all reloc types, but this only affects reloc types that lld doesn't support emitting as dynamic relocs since they are only used for TEXTRELs.

May 28 2020, 11:33 AM · Restricted Project

May 26 2020

mcgrathr created D80605: [AddressSanitizer] Don't use weak linkage for __{start,stop}_asan_globals.
May 26 2020, 8:10 PM · Restricted Project

May 20 2020

mcgrathr added inline comments to D80355: compiler-rt: decommit asan memory for unmaps in fuchsia..
May 20 2020, 10:33 PM · Restricted Project

May 12 2020

mcgrathr created D79835: [Fuchsia] Rely on linker switch rather than dead code ref for profile runtime.
May 12 2020, 6:54 PM · Restricted Project, Restricted Project
mcgrathr added a comment to D79822: [AArch64] Emit CFI instruction for updating x18 when using ShadowCallStack with exception unwinding.

glibc on Linux and perhaps other platforms has long implemented pthread_cancel via unwinding, and long supported asynchronous unwinding generally for this. If LLVM intends to support the Linux glibc ABI, then it must support -fasynchronous-unwind-tables generally.

May 12 2020, 5:15 PM · Restricted Project
mcgrathr added a comment to D79822: [AArch64] Emit CFI instruction for updating x18 when using ShadowCallStack with exception unwinding.

This is a direct revert of https://reviews.llvm.org/D54988. The rationale stated in that change was incorrect IMHO. When CFI is enabled, correct CFI is required, period. Making assumptions about how CFI will be used and thus when incomplete or incorrect CFI is adequate is a bad idea. The only proper distinctions are between no CFI at all, correct CFI at call return sites (-funwind-tables), and correct CFI at every instruction (-fasynchronous-unwind-tables). If generating CFI at all, then all CFI required to recover all caller registers not explicitly call-clobbered is required from at least every call return site in the function. As SCS CFI is necessary after the first or second prologue instruction, there is effectively no situation in which CFI omitting the x18 register is valid.

May 12 2020, 5:15 PM · Restricted Project
mcgrathr added a comment to D79664: [ELF] Support --pack-dyn-relocs=rel+relr.

If people are agreed, I think separate -z rel, -z relr, and -z android-relocs flags that operate independent makes the most sense. The existing --pack-dyn-relocs can be accepted for compatibility, but need not get any new supported values.

May 12 2020, 4:43 PM · Restricted Project

May 9 2020

mcgrathr added a comment to D79664: [ELF] Support --pack-dyn-relocs=rel+relr.

I haven't thought much on this yet. First impression: why isn't the RELA->REL toggle a new option? It does not seem to fit with the rest of --pack-dyn-relocs which selects Android packed dynamic relocations or RELR or both.

May 9 2020, 2:22 PM · Restricted Project
mcgrathr created D79667: [Clang] Pass -z max-page-size to linker for Fuchsia.
May 9 2020, 2:37 AM · Restricted Project
mcgrathr created D79665: [Clang] Pass --pack-dyn-relocs=relr to lld for Fuchsia.
May 9 2020, 2:05 AM · Restricted Project

May 8 2020

mcgrathr added a comment to D79664: [ELF] Support --pack-dyn-relocs=rel+relr.

Also note that compatible dynamic linkers support not just either, but both in the same executable (though the format requires that only one or the other be the format for PLT relocs, but those never need addends anyway). So for the large model case one could theoretically produce RELA for any relocs whose addend is too large for the bits available in the relocated entity while still producing REL for everything that fits. (The need ever to support large addends in the real world seems unlikely, but just saying.)

May 8 2020, 11:25 PM · Restricted Project
mcgrathr created D79664: [ELF] Support --pack-dyn-relocs=rel+relr.
May 8 2020, 11:04 PM · Restricted Project
mcgrathr added a comment to D79664: [ELF] Support --pack-dyn-relocs=rel+relr.

I'm not sure what all tests this should have. It was surprisingly easy to implement and seems to work perfectly. I've done a Fuchsia build with it that shows binaries that look as they should by eyeball, though I haven't yet tested it at runtime.

May 8 2020, 11:04 PM · Restricted Project
mcgrathr added a comment to D79664: [ELF] Support --pack-dyn-relocs=rel+relr.

Oh, and perhaps I'll update that comment since we do know why ELF specified RELA in the first place, which is for general-case reloc types for machines not like x86, where some reloc types don't apply to locations that store as many bits as the full address size that the addend can be in the general case. When you have TEXTRELs, then any dynamic reloc can also be such a type. For newer ABIs that never allowed TEXTRELs, I think people just assumed that the encoding for dynamic relocs should match the encoding for ET_REL relocs without thinking about it deeply. In the ABIs we're concerned with today, the only narrow reloc type that might appear as a dynamic reloc is ABS32 in ELFCLASS64, but limiting addends to 32 bits is no more limiting than the rules of the small memory model, so you just wouldn't use rela->rel encoding with large memory model (or you could punt only for specific programs when you hit an out of range addend for real, which seems unlikely). (A similar constraint has always been tolerated for R_ARM_MOV[WT]* relocs where EM_ARM is canonically REL rather than RELA and so addends used in relocs for movw/movt instructions are limited to 16 bits though the address computation is always a full 32 bit computation before the reloc stores the high or low 16 bits.)

May 8 2020, 11:04 PM · Restricted Project

Apr 22 2020

mcgrathr accepted D78687: [Fuchsia] Build compiler-rt builtins for 32-bit x86.

lgtm

Apr 22 2020, 5:25 PM · Restricted Project

Apr 7 2020

mcgrathr accepted D77647: [ELF][AArch64] Add R_AARCH64_PLT32 relocation type..

Thanks!

Apr 7 2020, 12:31 PM · Restricted Project

Apr 2 2020

mcgrathr added inline comments to D72959: Relative VTables ABI on Fuchsia.
Apr 2 2020, 11:55 AM · Restricted Project, Restricted Project, Restricted Project

Mar 30 2020

mcgrathr accepted D76963: [profile] Avoid duplicating or leaking VMO.

lgtm

Mar 30 2020, 2:11 PM · Restricted Project

Mar 24 2020

mcgrathr accepted D76750: [profile] Move RuntimeCounterRelocation and ProfileDumped into a separate file.

lgtm

Mar 24 2020, 8:24 PM · Restricted Project
mcgrathr accepted D76556: [profile] Make atexit hook a no-op on Fuchsia.

lgtm with comment and nits

Mar 24 2020, 6:36 PM · Restricted Project