Page MenuHomePhabricator

mcgrathr (Roland McGrath)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Yesterday

mcgrathr accepted D94362: [scudo][standalone] Enable death tests on Fuchsia.

lgtm.
btw, I think fx test --count will do that for you.

Fri, Jan 22, 12:43 PM · Restricted Project
mcgrathr updated subscribers of D94362: [scudo][standalone] Enable death tests on Fuchsia.
Fri, Jan 22, 12:41 PM · Restricted Project

Thu, Jan 21

mcgrathr added inline comments to D92696: [GWP-ASan] Add inbuilt options parser..
Thu, Jan 21, 7:10 PM · Restricted Project
mcgrathr added a comment to D93668: [clang] Override the Fuchsia platform ABI using the triple environment.

I am also a C++ programmer working on C++ ABI support. I'm glad to be working on a platform that made many intentional design decisions to enable keeping the C++ ABI out of the platform ABI. It makes it vastly easier to do interesting work on C++ ABIs such as what Leo is doing. It also makes it significantly easier to do interesting work on platform ABIs, which I also do.

Thu, Jan 21, 5:30 PM · Restricted Project

Wed, Jan 20

mcgrathr added a comment to D93668: [clang] Override the Fuchsia platform ABI using the triple environment.

The Fuchsia platform ABI includes the SS & SCS ABI. There is no compatibility issue with those. PIC-friendly optimizations are purely an optimization and correctness issue for any PIC/PIE code, and is not an ABI issue.
The only thing where we have an incompatible difference in ABI between our compiler environments is the C++ ABI.

Wed, Jan 20, 1:18 PM · Restricted Project

Fri, Jan 15

mcgrathr added a comment to D94355: [SimplifyCFG] Add relative switch lookup tables.

Many backends generate PIC-friendly jump tables. This is about generating IR initializers that translate to the same kind of backend assembly expressions as those backends use for their jump tables.

Fri, Jan 15, 2:35 PM · Restricted Project, Restricted Project

Thu, Jan 14

mcgrathr committed rGe7228062b2bb: [libc] Use #undef isascii in specific header (authored by mcgrathr).
[libc] Use #undef isascii in specific header
Thu, Jan 14, 1:25 PM
mcgrathr closed D94642: [libc] Use #undef isascii in specific header.
Thu, Jan 14, 1:25 PM · Restricted Project
mcgrathr added a comment to D94642: [libc] Use #undef isascii in specific header.

That plan sounds good to me. There is no way to fold #undef into a macro, so you can't avoid a little bit of boilerplate unless you generate the source code.

Thu, Jan 14, 1:17 PM · Restricted Project

Wed, Jan 13

mcgrathr requested review of D94642: [libc] Use #undef isascii in specific header.
Wed, Jan 13, 5:24 PM · Restricted Project

Mon, Jan 11

mcgrathr added inline comments to D94362: [scudo][standalone] Enable death tests on Fuchsia.
Mon, Jan 11, 11:16 AM · Restricted Project

Fri, Jan 8

mcgrathr added inline comments to D94195: [libc] Switch to use a macro which does not insert a section for every libc function..
Fri, Jan 8, 3:23 PM · Restricted Project
mcgrathr added inline comments to D94195: [libc] Switch to use a macro which does not insert a section for every libc function..
Fri, Jan 8, 2:27 PM · Restricted Project
mcgrathr added a comment to D93668: [clang] Override the Fuchsia platform ABI using the triple environment.

You've said you want to avoid expressing the C++ ABI as a generic switch orthogonal to target because people can use it when it's not what they actually want or isn't actually useful to attempt. This is true of all switches affecting ABI and they haven't all been rejected on this basis. But fine, consistency is the hobgoblin of small minds. If the concern there is either user confusion or support burden for configurations nobody wants to test, then both of those are easily addressed by having -fc++abi= constrain the settings it allows based on target. For Fuchsia targets, we want to maintain and test both "fuchsia" and "compat" (or "itanium") modes. If for other targets the only setting allowed is the one that's the default, or if using non-default settings generates a scare warning or suchlike, that's fine. That seems to be the natural way to constrain the explicit control of the C++ ABI to the supported cases, while not conflating control of the C++ ABI with the language-independent platform ABI being targeted.

Fri, Jan 8, 1:22 PM · Restricted Project

Thu, Jan 7

mcgrathr added inline comments to D94195: [libc] Switch to use a macro which does not insert a section for every libc function..
Thu, Jan 7, 5:59 PM · Restricted Project
mcgrathr added inline comments to D94195: [libc] Switch to use a macro which does not insert a section for every libc function..
Thu, Jan 7, 4:26 PM · Restricted Project
mcgrathr added a comment to D93668: [clang] Override the Fuchsia platform ABI using the triple environment.

It's fine to have a target tuple translate to a default C++ ABI.
But the C++ ABI selection is fundamentally not a target flavor thing. It's just a C++ ABI thing.
So using the target tuple as the sole mechanism to determine C++ ABI is fundamentally wrong.

Thu, Jan 7, 4:24 PM · Restricted Project
mcgrathr added inline comments to D94195: [libc] Switch to use a macro which does not insert a section for every libc function..
Thu, Jan 7, 12:59 PM · Restricted Project

Tue, Jan 5

mcgrathr committed rG90b8fd613607: scudo: Fix compilation for non-Linux aarch64 (authored by mcgrathr).
scudo: Fix compilation for non-Linux aarch64
Tue, Jan 5, 1:22 PM
mcgrathr closed D94108: scudo: Fix compilation for non-Linux aarch64.
Tue, Jan 5, 1:22 PM · Restricted Project
mcgrathr requested review of D94108: scudo: Fix compilation for non-Linux aarch64.
Tue, Jan 5, 11:58 AM · Restricted Project

Dec 16 2020

mcgrathr accepted D93420: [IR] Fixed the typo in attributes test.

lgtm

Dec 16 2020, 2:50 PM · Restricted Project

Dec 4 2020

mcgrathr accepted D92688: [GWP-ASan] IWYU & clang-format.
Dec 4 2020, 2:18 PM · Restricted Project

Dec 2 2020

mcgrathr committed rG70764c02e474: [CMake][Fuchsia] Install llvm-elfabi (authored by mcgrathr).
[CMake][Fuchsia] Install llvm-elfabi
Dec 2 2020, 11:59 AM
mcgrathr closed D92444: [CMake][Fuchsia] Install llvm-elfabi.
Dec 2 2020, 11:59 AM · Restricted Project
mcgrathr committed rG827e075676f6: [lsan] Use final on Fuchsia ThreadContext declaration (authored by mcgrathr).
[lsan] Use final on Fuchsia ThreadContext declaration
Dec 2 2020, 11:58 AM
mcgrathr closed D92442: [lsan] Use final on Fuchsia ThreadContext declaration.
Dec 2 2020, 11:58 AM · Restricted Project

Dec 1 2020

mcgrathr requested review of D92444: [CMake][Fuchsia] Install llvm-elfabi.
Dec 1 2020, 6:43 PM · Restricted Project
mcgrathr requested review of D92442: [lsan] Use final on Fuchsia ThreadContext declaration.
Dec 1 2020, 6:29 PM · Restricted Project
mcgrathr accepted D92415: [GWP-ASan] Fix flaky test on Fuchsia.

lgtm

Dec 1 2020, 2:46 PM · Restricted Project
mcgrathr accepted D92415: [GWP-ASan] Fix flaky test on Fuchsia.

I'm troubled in two ways.

  1. If this test is polluted by global (TLS) state from prior tests, that seems like a non-hermeticity problem for all these tests. Should we maybe be using a test fixture with SetUp/TearDown to wipe the global state between tests?

In other environments (both for upstream testing and Android) we run this test in isolation to avoid this problem.

Dec 1 2020, 1:28 PM · Restricted Project
mcgrathr added a comment to D92415: [GWP-ASan] Fix flaky test on Fuchsia.

I'm troubled in two ways.

  1. If this test is polluted by global (TLS) state from prior tests, that seems like a non-hermeticity problem for all these tests. Should we maybe be using a test fixture with SetUp/TearDown to wipe the global state between tests?
  2. It seems like the code under test actually has semantics that when uninitialized, it will sometimes sample (just rarely). That seems like a problem to me. If the initialized state as configured by the environment will be to never do a gwp allocation, then there should never be a window where one is randomly allowed to happen.
Dec 1 2020, 12:23 PM · Restricted Project

Nov 18 2020

mcgrathr accepted D91575: [GWP-ASan] Port tests to Fuchsia.

lgtm

Nov 18 2020, 10:36 AM · Restricted Project
mcgrathr committed rG7810d8378649: [GWP-ASan] Respect GWP_ASAN_DEFAULT_ENABLED compile-time macro (authored by mcgrathr).
[GWP-ASan] Respect GWP_ASAN_DEFAULT_ENABLED compile-time macro
Nov 18 2020, 10:35 AM
mcgrathr closed D91463: [GWP-ASan] Respect GWP_ASAN_DEFAULT_ENABLED compile-time macro.
Nov 18 2020, 10:34 AM · Restricted Project
mcgrathr added a comment to D91463: [GWP-ASan] Respect GWP_ASAN_DEFAULT_ENABLED compile-time macro.

updated macro

Nov 18 2020, 10:34 AM · Restricted Project
mcgrathr updated the diff for D91463: [GWP-ASan] Respect GWP_ASAN_DEFAULT_ENABLED compile-time macro.

macro nit

Nov 18 2020, 10:33 AM · Restricted Project

Nov 13 2020

mcgrathr committed rG6ef07111a402: [scudo/standalone] Fix leak in ThreadedGlobalQuarantine test (authored by mcgrathr).
[scudo/standalone] Fix leak in ThreadedGlobalQuarantine test
Nov 13 2020, 10:25 PM
mcgrathr closed D91472: [scudo/standalone] Fix leak in ThreadedGlobalQuarantine test.
Nov 13 2020, 10:24 PM · Restricted Project
mcgrathr added inline comments to D91472: [scudo/standalone] Fix leak in ThreadedGlobalQuarantine test.
Nov 13 2020, 10:24 PM · Restricted Project
mcgrathr updated the diff for D91472: [scudo/standalone] Fix leak in ThreadedGlobalQuarantine test.

style nit

Nov 13 2020, 10:23 PM · Restricted Project
mcgrathr requested review of D91472: [scudo/standalone] Fix leak in ThreadedGlobalQuarantine test.
Nov 13 2020, 6:28 PM · Restricted Project
mcgrathr added a comment to D91463: [GWP-ASan] Respect GWP_ASAN_DEFAULT_ENABLED compile-time macro.

I'm not sure adding this complexity here is warranted, why is -DGWP_ASAN_DEFAULT_ENABLED=false better than -DSCUDO_DEFAULT_OPTIONS=GWP_ASAN_Enabled=false?

Nov 13 2020, 5:30 PM · Restricted Project
mcgrathr requested review of D91463: [GWP-ASan] Respect GWP_ASAN_DEFAULT_ENABLED compile-time macro.
Nov 13 2020, 2:38 PM · Restricted Project
mcgrathr added inline comments to D91387: [Driver] Support UBSan multilib.
Nov 13 2020, 11:30 AM · Restricted Project

Nov 10 2020

mcgrathr committed rGcf36142d342a: [clang] Add missing header guard in <cpuid.h> (authored by mcgrathr).
[clang] Add missing header guard in <cpuid.h>
Nov 10 2020, 7:34 PM
mcgrathr closed D91226: [clang] Add missing header guard in <cpuid.h>.
Nov 10 2020, 7:34 PM · Restricted Project
mcgrathr requested review of D91226: [clang] Add missing header guard in <cpuid.h>.
Nov 10 2020, 6:57 PM · Restricted Project

Nov 3 2020

mcgrathr added a comment to D90275: [clang][IR] Add support for leaf attribute.

The GCC documentation specifically gives the example of the standard C function qsort as one that does not qualify as __attribute__((leaf)) because it uses a callback function (that presumably might be from the caller's own TU). AIUI the claim the attribute makes is that no normal control flow (i.e. excluding only signal handlers) would call any function in the caller's TU by any means, nor longjmp to any state captured by a setjmp call in the caller's TU, and thus only normal return or normal C++ exception handling return/unwind would reach the caller's TU again after the jump (call) to this entry point. The caller is thus presumed not to share potentially-called functions with the callee by any means, whether direct symbol references or function pointers passed in the call or function pointers previously stored somewhere (e.g. a C++ vtable). The compiler can make whatever assumptions about potential dataflow/side-effect and the like are implied by this exclusion of TU-local code paths from the otherwise unknown call graph of the external call.

Nov 3 2020, 5:47 PM · Restricted Project, Restricted Project

Nov 2 2020

mcgrathr accepted D90537: [GWP-ASan] Stub out backtrace/signal functions on Fuchsia.

lgtm

Nov 2 2020, 11:10 AM · Restricted Project

Oct 30 2020

mcgrathr added inline comments to D90483: [GWP-ASan] Fuchsia specific mapping & utilities functions.
Oct 30 2020, 10:14 PM · Restricted Project
mcgrathr accepted D90483: [GWP-ASan] Fuchsia specific mapping & utilities functions.

LGTM with a few minor changes.
Thanks!

Oct 30 2020, 3:54 PM · Restricted Project

Oct 29 2020

mcgrathr accepted D90351: [GWP-ASan] Add mutexes for Fuchsia.

lgtm

Oct 29 2020, 10:55 AM · Restricted Project
mcgrathr committed rG5a3077f3a7b3: [sanitizer][fuchsia] Avoid deprecated syscall. (authored by jsankey).
[sanitizer][fuchsia] Avoid deprecated syscall.
Oct 29 2020, 10:52 AM
mcgrathr closed D90169: [sanitizer][fuchsia] Avoid deprecated syscall..
Oct 29 2020, 10:52 AM · Restricted Project
mcgrathr committed rGddfe4784cc6e: [Support] Make Support/SwapByteOrder.h compile on Fuchsia (authored by mcgrathr).
[Support] Make Support/SwapByteOrder.h compile on Fuchsia
Oct 29 2020, 10:49 AM
mcgrathr closed D90279: [Support] Make Support/SwapByteOrder.h compile on Fuchsia.
Oct 29 2020, 10:49 AM · Restricted Project

Oct 27 2020

mcgrathr requested review of D90279: [Support] Make Support/SwapByteOrder.h compile on Fuchsia.
Oct 27 2020, 6:31 PM · Restricted Project
mcgrathr added a comment to D90275: [clang][IR] Add support for leaf attribute.

There is an RFC going out with this prototype as reference. When there is consensus on the RFC, this will get in shape for landing with complete tests and all.

Oct 27 2020, 5:32 PM · Restricted Project, Restricted Project
mcgrathr added a comment to D90169: [sanitizer][fuchsia] Avoid deprecated syscall..

I already approved this as is. I just think it's dead code.

Oct 27 2020, 4:13 PM · Restricted Project
mcgrathr added inline comments to D90195: [GWP-ASan] Abstract the thread local variables access.
Oct 27 2020, 2:52 PM · Restricted Project
mcgrathr added a comment to D90169: [sanitizer][fuchsia] Avoid deprecated syscall..

I think NanoTime is actually not reached at all. We can probably remove it.

Oct 27 2020, 1:47 PM · Restricted Project
mcgrathr accepted D90169: [sanitizer][fuchsia] Avoid deprecated syscall..

This is fine. If we wanted to avoid wider use of _zx_utc_reference_get, I think it would also be fine for this one call to use the C11 timespec_get API rather than a pure Zircon API.

Oct 27 2020, 12:26 PM · Restricted Project

Oct 26 2020

mcgrathr accepted D90195: [GWP-ASan] Abstract the thread local variables access.

This is fine as is but there's really no need to carefully pack it into a uint64_t. It would be fine for Fuchsia's special header to have to meet some less trivial API. The main complexity there would be just figuring out the header arrangement so that Fuchsia's header could #include something to define the types it's supposed to use. But AFAICT it doesn't really need to be in guarded_pool_allocator.h like this. That generic header could just declare static ThreadLocalPackedVariables *getThreadLocals();. Then the platform header can provide the inline defn outside the class scope, and that header only needs to be used in guarded_pool_allocator.cpp itself.

Oct 26 2020, 4:23 PM · Restricted Project
mcgrathr accepted D89993: [GWP-ASan] Refactor memory mapping functions.

Thanks!

Oct 26 2020, 1:06 PM · Restricted Project

Oct 24 2020

mcgrathr committed rG1e09dbb6a942: [asan] Fix stack-use-after-free checks on non-main thread on Fuchsia (authored by zarvox).
[asan] Fix stack-use-after-free checks on non-main thread on Fuchsia
Oct 24 2020, 2:29 PM
mcgrathr closed D89607: [asan] Fix stack-use-after-free checks on non-main thread on Fuchsia.
Oct 24 2020, 2:29 PM · Restricted Project
mcgrathr committed rG29480c6c746c: [asan][fuchsia] set current thread before reading thread state (authored by zarvox).
[asan][fuchsia] set current thread before reading thread state
Oct 24 2020, 2:23 PM
mcgrathr closed D89606: [asan][fuchsia] set current thread before reading thread state.
Oct 24 2020, 2:23 PM · Restricted Project

Oct 23 2020

mcgrathr accepted D89993: [GWP-ASan] Refactor memory mapping functions.

LGTM. Different names and more comments would be preferable.

Oct 23 2020, 2:26 PM · Restricted Project
mcgrathr added a comment to D89607: [asan] Fix stack-use-after-free checks on non-main thread on Fuchsia.

I don't really understand why the whole "lazy" logic exists. I can't tell what case it would ever be used in as the code stands. Every thread will pass through ASanThread::Init before it would ever just call fake_stack() and hit the "lazy" case. Is it intended that one could set __asan_option_detect_stack_use_after_return to false (either from startup or explicitly clear it), then create a thread, then set the flag explicitly on later?

Oct 23 2020, 11:58 AM · Restricted Project
mcgrathr accepted D89606: [asan][fuchsia] set current thread before reading thread state.

Thanks for the fix!

Oct 23 2020, 10:58 AM · Restricted Project
mcgrathr accepted D89432: [llvm-elfabi] Emit ELF .dynsym and .dynamic sections.

LGTM. The section ordering details are conventional but not supposed to be semantically meaningful anyway.

Oct 23 2020, 10:43 AM · Restricted Project
mcgrathr accepted D88347: [tsan] Refactor Memory Release Functions..

I'm not sure this is the optimal refactor ultimately, but it seems like a safe and clean pure refactor that's at least an incremental cleanup in expressing what the APIs are being used for.

Oct 23 2020, 10:28 AM · Restricted Project

Oct 8 2020

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

Oct 2 2020

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".
Oct 2 2020, 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"
Oct 2 2020, 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".
Oct 2 2020, 6:15 PM · Restricted Project
mcgrathr closed D88768: Revert "[lsan] Share platform allocator settings between ASan and LSan".
Oct 2 2020, 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".
Oct 2 2020, 6:14 PM
mcgrathr requested review of D88768: Revert "[lsan] Share platform allocator settings between ASan and LSan".
Oct 2 2020, 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".
Oct 2 2020, 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
Oct 2 2020, 5:56 PM
mcgrathr closed D87795: [lsan] Share platform allocator settings between ASan and LSan.
Oct 2 2020, 5:55 PM · Restricted Project

Sep 29 2020

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
Sep 29 2020, 11:17 AM · Restricted Project

Sep 28 2020

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

lgtm

Sep 28 2020, 2:09 PM · Restricted Project

Sep 25 2020

mcgrathr added inline comments to D88248: [lsan] On Fuchsia, don't use atexit hook for leak checks.
Sep 25 2020, 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

Sep 25 2020, 12:12 PM · Restricted Project

Sep 24 2020

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.

Sep 24 2020, 10:43 AM · Restricted Project
mcgrathr requested review of D88248: [lsan] On Fuchsia, don't use atexit hook for leak checks.
Sep 24 2020, 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?

Sep 24 2020, 10:08 AM · Restricted Project

Sep 23 2020

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!

Sep 23 2020, 8:11 PM · Restricted Project
mcgrathr added reviewers for D88184: [lsan] Add interceptor for pthread_detach.: eugenis, vitalybuka.
Sep 23 2020, 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
Sep 23 2020, 11:59 AM
mcgrathr closed D88173: asan: Use `#if` to test CAN_SANITIZE_LEAKS.
Sep 23 2020, 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

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

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

Sep 23 2020, 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
Sep 23 2020, 11:11 AM
mcgrathr closed D86171: [lsan] On Fuchsia, don't use atexit hook for leak checks.
Sep 23 2020, 11:11 AM · Restricted Project