Page MenuHomePhabricator

mcgrathr (Roland McGrath)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Yesterday

mcgrathr added a comment to D72887: [lsan] Support LeakSanitizer runtime on Fuchsia.

I think I've done everything Vitaly's last review asked for. Thanks.

Thu, Jan 23, 5:00 PM · Restricted Project, Restricted Project
mcgrathr added a parent revision for D72887: [lsan] Support LeakSanitizer runtime on Fuchsia: D73309: [lsan] Factor pthread-specific assumptions out of thread tracking code.
Thu, Jan 23, 5:00 PM · Restricted Project, Restricted Project
mcgrathr added a child revision for D73309: [lsan] Factor pthread-specific assumptions out of thread tracking code: D72887: [lsan] Support LeakSanitizer runtime on Fuchsia.
Thu, Jan 23, 5:00 PM · Restricted Project, Restricted Project
mcgrathr updated the diff for D72887: [lsan] Support LeakSanitizer runtime on Fuchsia.

Split out ThreadContext refactoring into a separate patch.
Updated related changes following review comments.

Thu, Jan 23, 4:52 PM · Restricted Project, Restricted Project
mcgrathr created D73309: [lsan] Factor pthread-specific assumptions out of thread tracking code.
Thu, Jan 23, 4:49 PM · Restricted Project, Restricted Project
mcgrathr updated the diff for D72988: [lsan] Expose Frontier object to OS-specific LockStuffAndStopTheWorld callback.

Rebased.

Thu, Jan 23, 4:49 PM · Restricted Project, Restricted Project
mcgrathr updated the diff for D72886: [sanitizer_common] Implement MemoryMappingLayout for Fuchsia.

Rebased

Thu, Jan 23, 4:40 PM · Restricted Project, Restricted Project

Sat, Jan 18

mcgrathr added a parent revision for D72887: [lsan] Support LeakSanitizer runtime on Fuchsia: D72988: [lsan] Expose Frontier object to OS-specific LockStuffAndStopTheWorld callback.
Sat, Jan 18, 3:23 PM · Restricted Project, Restricted Project
mcgrathr added a child revision for D72988: [lsan] Expose Frontier object to OS-specific LockStuffAndStopTheWorld callback: D72887: [lsan] Support LeakSanitizer runtime on Fuchsia.
Sat, Jan 18, 3:23 PM · Restricted Project, Restricted Project
mcgrathr added a comment to D72887: [lsan] Support LeakSanitizer runtime on Fuchsia.

I've split part of the patch out and done a substantial refactoring of the standalone lsan thread stuff following Vitaly's review.

Sat, Jan 18, 3:23 PM · Restricted Project, Restricted Project
mcgrathr updated the diff for D72887: [lsan] Support LeakSanitizer runtime on Fuchsia.

Generic refactoring patch split out.
Standalone LSan port refactored following review comments.

Sat, Jan 18, 3:21 PM · Restricted Project, Restricted Project
mcgrathr created D72988: [lsan] Expose Frontier object to OS-specific LockStuffAndStopTheWorld callback.
Sat, Jan 18, 3:21 PM · Restricted Project, Restricted Project
mcgrathr updated the diff for D72886: [sanitizer_common] Implement MemoryMappingLayout for Fuchsia.

Rebased.

Sat, Jan 18, 2:16 PM · Restricted Project, Restricted Project
mcgrathr added a child revision for D72886: [sanitizer_common] Implement MemoryMappingLayout for Fuchsia: D72887: [lsan] Support LeakSanitizer runtime on Fuchsia.
Sat, Jan 18, 10:07 AM · Restricted Project, Restricted Project
mcgrathr added a parent revision for D72887: [lsan] Support LeakSanitizer runtime on Fuchsia: D72886: [sanitizer_common] Implement MemoryMappingLayout for Fuchsia.
Sat, Jan 18, 10:07 AM · Restricted Project, Restricted Project
mcgrathr added a reviewer for D72886: [sanitizer_common] Implement MemoryMappingLayout for Fuchsia: vitalybuka.
Sat, Jan 18, 10:04 AM · Restricted Project, Restricted Project
mcgrathr updated the diff for D72886: [sanitizer_common] Implement MemoryMappingLayout for Fuchsia.

Rebased, added a comment.

Sat, Jan 18, 10:04 AM · Restricted Project, Restricted Project

Thu, Jan 16

mcgrathr created D72887: [lsan] Support LeakSanitizer runtime on Fuchsia.
Thu, Jan 16, 3:34 PM · Restricted Project, Restricted Project
mcgrathr created D72886: [sanitizer_common] Implement MemoryMappingLayout for Fuchsia.
Thu, Jan 16, 3:24 PM · Restricted Project, Restricted Project

Dec 17 2019

mcgrathr accepted D71641: [unwind] Don't link libpthread and libdl on Fuchsia.

lgtm

Dec 17 2019, 5:29 PM · Restricted Project

Oct 16 2019

mcgrathr updated the diff for D66712: [Driver] Enable ShadowCallStack, not SafeStack, by default on AArch64 Fuchsia.

rebased

Oct 16 2019, 11:15 AM · Restricted Project
mcgrathr added a comment to D66712: [Driver] Enable ShadowCallStack, not SafeStack, by default on AArch64 Fuchsia.

It's time to land this now. Can one of you commit it for me?

Oct 16 2019, 11:15 AM · Restricted Project

Aug 24 2019

mcgrathr added reviewers for D66712: [Driver] Enable ShadowCallStack, not SafeStack, by default on AArch64 Fuchsia: phosek, leonardchan, jakehehrlich.

This should land only after we've done our first stages of downstream roll-out and burn-in testing. But setting it up now.

Aug 24 2019, 11:14 PM · Restricted Project
mcgrathr created D66712: [Driver] Enable ShadowCallStack, not SafeStack, by default on AArch64 Fuchsia.
Aug 24 2019, 11:12 PM · Restricted Project

Aug 15 2019

mcgrathr accepted D66323: [Fuchsia] Create the VMO during initialization, not during exit.

lgtm

Aug 15 2019, 4:54 PM · Restricted Project, Restricted Project

Aug 14 2019

mcgrathr accepted D66233: Always print DSO map on Fuchsia libFuzzer launch.

lgtm

Aug 14 2019, 8:46 PM · Restricted Project, Restricted Project

Aug 4 2019

mcgrathr created D65715: [Driver] Don't disable -fsanitizer-coverage for safe-stack or shadow-call-stack.
Aug 4 2019, 1:57 AM · Restricted Project, Restricted Project

Jul 17 2019

mcgrathr added inline comments to D64603: [Target] Use IEEE quad format for long double on Fuchsia x86_64.
Jul 17 2019, 12:50 PM · Restricted Project

Jul 11 2019

mcgrathr accepted D64605: [CMake][Fuchsia] Use RelWithDebInfo to build runtimes.

Lgtm

Jul 11 2019, 4:31 PM · Restricted Project, Restricted Project

Jul 3 2019

mcgrathr accepted D64166: [ASan] Use __sanitizer_fill_shadow for FastPoisonShadow on Fuchsia.

AFAICT the inlining is the whole point of FastPoisonShadow so this LGTM as is.

Jul 3 2019, 5:34 PM · Restricted Project, Restricted Project
mcgrathr accepted D64140: [CMake][Fuchsia] Define asan+noexcept multilib.

lgtm

Jul 3 2019, 10:20 AM · Restricted Project, Restricted Project

Jun 29 2019

mcgrathr added a reviewer for D63695: [sancov] Ignore PC samples with value 0: kcc.
Jun 29 2019, 11:36 AM · Restricted Project
mcgrathr added a comment to D63695: [sancov] Ignore PC samples with value 0.

PTAL

Jun 29 2019, 11:36 AM · Restricted Project

Jun 24 2019

mcgrathr updated the diff for D63695: [sancov] Ignore PC samples with value 0.

Move logic out of RawCoverage constructor.

Jun 24 2019, 12:24 PM · Restricted Project

Jun 23 2019

mcgrathr added a comment to D63695: [sancov] Ignore PC samples with value 0.

https://fuchsia-review.googlesource.com/c/fuchsia/+/295508 has the runtime implementation that motivated this.

Jun 23 2019, 4:03 PM · Restricted Project
mcgrathr created D63695: [sancov] Ignore PC samples with value 0.
Jun 23 2019, 3:58 PM · Restricted Project

May 31 2019

mcgrathr accepted D62770: [libcxx] Use libtool when merging archives on Apple platforms.

lgtm

May 31 2019, 11:35 PM · Restricted Project
mcgrathr accepted D62769: [CMake] Use libtool for runtimes when building for Apple platform.

lgtm

May 31 2019, 11:34 PM · Restricted Project

May 30 2019

mcgrathr accepted D62712: [CMake][Fuchsia] Use libc++ ABI v2 on Darwin as well.

lgtm

May 30 2019, 6:12 PM · Restricted Project, Restricted Project

May 28 2019

mcgrathr accepted D62558: [Driver] Search the toolchain dir with -print-file-name.

lgtm

May 28 2019, 4:51 PM · Restricted Project

May 27 2019

mcgrathr accepted D62469: [Driver] Change layout of per-target runtimes to resemble multiarch.

lgtm

May 27 2019, 12:30 PM · Restricted Project, Restricted Project, Restricted Project

May 24 2019

mcgrathr accepted D62442: [Driver] Update handling of c++ and runtime directories.

lgtm contingent on verifying intended behavior changes and adding comments. The factoring suggestion for the triple,triple searching is preferred but at your discretion, though I'd really like to see at least comments.

May 24 2019, 11:03 PM · Restricted Project

May 21 2019

mcgrathr accepted D62226: [libFuzzer] Ignore synthetic exceptions on Fuchsia.

lgtm with one nit: either change to an assert or improve the comment to clarify what other cases are expected

May 21 2019, 6:36 PM · Restricted Project, Restricted Project

May 14 2019

mcgrathr accepted D61919: [builtins] Deduplicate __eqsf2 and __gtsf2.

Lgtm

May 14 2019, 2:50 PM · Restricted Project, Restricted Project

May 6 2019

mcgrathr added a comment to D61510: [SanitizerCoverage] Use different module ctor names for trace-pc-guard and inline-8bit-counters.

Previously the same local symbol name was being used for both things in an uncoordinated fashion. The logic in places like ValueSymbolTable::createValueName leads to the second one getting renamed with some ".digits" suffix to make them unique.
Then the code that emits the references between the init_array and the actual function and sets the COMDAT group name assumed it could use the original name throughout, but in some places this was replaced with the suffixed name and in some places it was not, leading to the incorrect references.

May 6 2019, 11:58 AM · Restricted Project
mcgrathr added a comment to D61477: [ELF] -z combreloc: sort dynamic relocations by (symbol_index,r_offset).

Sorting R_*_RELATIVE relocs first and setting REL(A)COUNT is a format requirement, period. It's not a debatable question of optimization.

May 6 2019, 11:51 AM · Restricted Project

May 5 2019

mcgrathr created D61572: [libcxxabi] Don't use -fvisibility-global-new-delete-hidden when not defining them.
May 5 2019, 11:56 AM · Restricted Project, Restricted Project
mcgrathr created D61571: [libcxx] Don't use -fvisibility-global-new-delete-hidden when not defining them.
May 5 2019, 11:55 AM · Restricted Project, Restricted Project

May 4 2019

mcgrathr added reviewers for D61510: [SanitizerCoverage] Use different module ctor names for trace-pc-guard and inline-8bit-counters: jakehehrlich, phosek.

This works for my case that led to me to file https://bugs.llvm.org/show_bug.cgi?id=41693 as a linker issue.

May 4 2019, 9:27 PM · Restricted Project

May 3 2019

mcgrathr added a comment to D61477: [ELF] -z combreloc: sort dynamic relocations by (symbol_index,r_offset).

The sorting behavior is entirely an optimization for locality at dynamic reloc time. Both locality of symtab indices and locality of reloc offsets are useful to that goal.

May 3 2019, 10:59 AM · Restricted Project

Apr 30 2019

mcgrathr accepted D61363: [compiler-rt] Pass sysroot and disable pedantic for crtbegin.o/crtend.o.

Lgtm

Apr 30 2019, 8:27 PM · Restricted Project, Restricted Project

Apr 24 2019

mcgrathr added inline comments to D61040: [Fuchsia] Support multilib for -fsanitize=address and -fno-exceptions.
Apr 24 2019, 4:44 PM · Restricted Project
mcgrathr accepted D60990: [Driver] Support priority for multilibs.

lgtm

Apr 24 2019, 4:43 PM · Restricted Project
mcgrathr accepted D61040: [Fuchsia] Support multilib for -fsanitize=address and -fno-exceptions.

lgtm

Apr 24 2019, 4:42 PM · Restricted Project

Apr 3 2019

mcgrathr accepted D60245: [libunwind] Export the weak alias in Mach-O.

Lgtm

Apr 3 2019, 8:26 PM · Restricted Project
mcgrathr accepted D60235: [CMake] Disable libc++ and libc++abi new/delete definitions when built with ASan.

IMHO the library code should use #if !__has_feature(...) to avoid the definitions entirely when built with a sanitizer whose runtime provides them.
But this is a fine way to achieve that while we wait for those libraries to be fixed for sanitized builds.

Apr 3 2019, 3:47 PM · Restricted Project

Apr 2 2019

mcgrathr accepted D60176: [libc++][libc++abi] Don't provide new/delete when built with ASan, HWASan or TSan.

lgtm

Apr 2 2019, 8:44 PM · Restricted Project

Mar 28 2019

mcgrathr added inline comments to D59921: [libunwind] Export the unw_* symbols as weak symbols.
Mar 28 2019, 10:25 AM · Restricted Project

Feb 16 2019

mcgrathr accepted D58325: [Driver][Fuchsia] Support -nolibc flag.

This is only on GCC trunk (i.e. 9), not in 8.2 or even the current gcc-8 branch. So clarify the log entry. We don't know if 8.x,x>2 will add it or only gcc 9.

Feb 16 2019, 4:52 PM · Restricted Project, Restricted Project
mcgrathr added inline comments to D58184: [scudo][standalone] Introduce platform specific code & mutexes.
Feb 16 2019, 1:16 AM · Restricted Project, Restricted Project

Feb 14 2019

mcgrathr added inline comments to D58184: [scudo][standalone] Introduce platform specific code & mutexes.
Feb 14 2019, 10:12 AM · Restricted Project, Restricted Project

Feb 13 2019

mcgrathr added inline comments to D58184: [scudo][standalone] Introduce platform specific code & mutexes.
Feb 13 2019, 1:25 PM · Restricted Project, Restricted Project
mcgrathr added a comment to D58184: [scudo][standalone] Introduce platform specific code & mutexes.

I only really looked at the Fuchsia code.

Feb 13 2019, 11:57 AM · Restricted Project, Restricted Project

Feb 9 2019

mcgrathr accepted D58010: [CodeGen] Set construction vtable visibility after creating initializer.

lgtm with a comment (and perhaps an assertion).

Feb 9 2019, 7:55 PM · Restricted Project, Restricted Project

Jan 30 2019

mcgrathr accepted D57492: [CMake][compiler-rt] Enable statically linking unwinder and c++abi.

lgtm

Jan 30 2019, 6:13 PM

Jan 25 2019

mcgrathr accepted D57262: [libunwind] Drop the dependency on <algorithm>, add placement new inline.

Should you add -nostdinc++ to the compilation flags to drive home that this code can't depend on any library headers?

Jan 25 2019, 3:39 PM

Jan 20 2019

mcgrathr accepted D56972: [CMake][Fuchsia] Drop -DNDEBUG, re-enable modules.

lgtm

Jan 20 2019, 5:01 PM

Jan 14 2019

mcgrathr accepted D56652: [CMake][Fuchsia] Synchronize first and second stage builds.

lgtm

Jan 14 2019, 12:53 PM

Jan 5 2019

mcgrathr accepted D56360: [compiler-rt][Fuchsia] Replace _zx_vmar_allocate_old call.

lgtm

Jan 5 2019, 9:01 PM
mcgrathr accepted D56359: [CMake][Fuchsia] Enable build ID, relaxations for first stage.

lgtm

Jan 5 2019, 7:29 PM

Jan 4 2019

mcgrathr accepted D56349: [CMake][Fuchsia] Enable x86 relaxation by default.

lgtm. Wouldn't hurt to comment that this depends on a linker new enough to support the new reloc types (only reason it's conditional at all) and since we default to lld we don't worry about host system linkers that might be too old to support the new reloc types.

Jan 4 2019, 11:55 PM
mcgrathr accepted D56350: [CMake][Fuchsia] Enable experimental new pass manager by default.

lgtm

Jan 4 2019, 11:55 PM
mcgrathr accepted D56348: [CMake][Fuchsia] Enable --build-id linker flag by default.

lgtm

Jan 4 2019, 11:53 PM

Dec 10 2018

mcgrathr accepted D55405: [CMake] Use hidden visibility for static libc++ in Fuchsia.

lgtm

Dec 10 2018, 12:57 PM

Nov 15 2018

mcgrathr accepted D54613: [CMake] Use the correct spelling for armv7 in Fuchsia's toolchain.

lgtm

Nov 15 2018, 8:04 PM
mcgrathr accepted D54612: [compiler-rt] Use exact spelling when building for default target.

lgtm with a comment in the code

Nov 15 2018, 8:04 PM

Nov 13 2018

mcgrathr accepted D54463: [CMake] Support cross-compiling with Fuchsia toolchain build.

lgtm

Nov 13 2018, 1:25 PM

Nov 10 2018

mcgrathr added inline comments to D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Nov 10 2018, 3:54 PM

Nov 6 2018

mcgrathr accepted D54169: [clang-tidy] Zircon <fbl/limits.h> -> <limits>.

lgtm

Nov 6 2018, 12:03 PM · Restricted Project

Nov 5 2018

mcgrathr added a comment to D53796: [libcxx] Use AS_NEEDED command for linker script inputs.

Is this patch still needed now that Fuchsia uses:

CmdArgs.push_back("--push-state");
CmdArgs.push_back("--as-needed");
if (OnlyLibstdcxxStatic)
  CmdArgs.push_back("-Bstatic");
ToolChain.AddCXXStdlibLibArgs(Args, CmdArgs);
if (OnlyLibstdcxxStatic)
  CmdArgs.push_back("-Bdynamic");
CmdArgs.push_back("-lm");
CmdArgs.push_back("--pop-state");
Nov 5 2018, 6:48 PM
mcgrathr requested changes to D54112: [Driver] Delete redundant -Bdynamic for libc++ on Fuchsia.

This breaks the semantics we want. The -Bdynamic is there to apply to -lm, which is also what --as-needed is there for in this case (it appears earlier because of the conditional logic, since in the other case it applies to -lc++ as well). It's correct as is.

Nov 5 2018, 4:51 PM

Nov 3 2018

mcgrathr accepted D54082: [Driver] Use -Bstatic/dynamic for libc++ on Fuchsia.

lgtm

Nov 3 2018, 9:33 PM
mcgrathr reopened D53854: [Driver] Use -push-/-pop-state and -as-needed for libc++ on Fuchsia.
Nov 3 2018, 6:25 PM
mcgrathr accepted D54064: [Driver] Always match resource dir in Fuchsia driver tests.

lgtm

Nov 3 2018, 5:06 PM
mcgrathr added inline comments to D53854: [Driver] Use -push-/-pop-state and -as-needed for libc++ on Fuchsia.
Nov 3 2018, 5:06 PM

Nov 2 2018

mcgrathr accepted D54026: [CMake][Fuchsia] Set -fuse-ld=lld explicitly for Linux runtimes.

So the default is not per-target? It should be.

Nov 2 2018, 11:20 AM

Nov 1 2018

mcgrathr accepted D53970: [CMake][Fuchsia] Don't restrict Linux runtimes to UNIX.

lgtm

Nov 1 2018, 12:43 PM

Oct 31 2018

mcgrathr added a comment to D53787: [Sema] Provide -fvisibility-global-new-delete-hidden option.

Other symbols must have exactly one definition (modulo the permission for duplicate identical definitions for some cases), but these ones have a default definition that is designed to be overridable by a different definition appearing anywhere in the program.

I don't understand this claim. These are symbols like any others at link time. A single definition must be supplied as for any other function.

The program has two choices: either it provides a definition, and that gets used everywhere, or it does not, and the default version (provided by the toolchain) gets used everywhere.

Oct 31 2018, 5:10 PM
mcgrathr added a comment to D53787: [Sema] Provide -fvisibility-global-new-delete-hidden option.

These symbols really are special. Other symbols are introduced explicitly by a declaration, whereas these are declared implicitly by the compiler.

Oct 31 2018, 3:36 PM
mcgrathr added a comment to D53787: [Sema] Provide -fvisibility-global-new-delete-hidden option.

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

If you really want to do this weird thing, and you understand what you're getting yourself into, I don't see a problem with having a dedicated flag for it, but don't break all existing users of -fvisibility=.

Oct 31 2018, 2:48 PM

Oct 30 2018

mcgrathr accepted D53854: [Driver] Use -push-/-pop-state and -as-needed for libc++ on Fuchsia.
Oct 30 2018, 4:37 PM

Oct 28 2018

mcgrathr accepted D53801: [libc++abi] Provide __cxa_thread_atexit on Fuchsia.

lgtm

Oct 28 2018, 4:39 PM
mcgrathr added a comment to D53801: [libc++abi] Provide __cxa_thread_atexit on Fuchsia.

I don't know what's affected by the presence of that declaration. Certainly it should be defined.
AFAICT this has nothing to do with solving the problem of the symbol not existing in the library.
That needs CMakeLists.txt:34 changed so it compiles cxa_thread_atexit.cpp for Fuchsia.
We also need to make sure that the cmake check for presence of __cxa_thread_atexit_impl in libc is passing correctly.

Oct 28 2018, 4:11 PM

Oct 23 2018

mcgrathr accepted D53487: [Driver] Support sanitized libraries on Fuchsia.

lgtm

Oct 23 2018, 12:46 PM

Oct 19 2018

mcgrathr accepted D52162: [XRay] Support for Fuchsia.

lgtm

Oct 19 2018, 5:53 PM
mcgrathr added inline comments to D52162: [XRay] Support for Fuchsia.
Oct 19 2018, 11:51 AM

Oct 17 2018

mcgrathr added inline comments to D52162: [XRay] Support for Fuchsia.
Oct 17 2018, 12:26 PM

Oct 3 2018

mcgrathr added a comment to D49511: [Sema/Attribute] Check for noderef attribute.

The motivating use case always pairs noderef with an address_space attribute, and it's already invalid to convert between pointer types in different address spaces without a cast.
So I don't think we have a strong opinion one way or the other. In the abstract, I'd say noderef feels like a "constraining" qualifier a la const/volatile so that going from unconstrained to constrained implicitly is OK but not vice versa.

Oct 3 2018, 7:07 PM · Restricted Project

Sep 28 2018

mcgrathr accepted D52660: [CMake][Fuchsia] Use unstable libc++ ABI for Fuchsia toolchain.

lgtm

Sep 28 2018, 10:17 AM
mcgrathr added inline comments to D52162: [XRay] Support for Fuchsia.
Sep 28 2018, 9:50 AM