Page MenuHomePhabricator

mcgrathr (Roland McGrath)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Yesterday

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

Move logic out of RawCoverage constructor.

Mon, Jun 24, 12:24 PM · Restricted Project

Sun, Jun 23

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.

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

Fri, May 31

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

lgtm

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

lgtm

Fri, May 31, 11:34 PM · Restricted Project

Thu, May 30

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

lgtm

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

Tue, May 28

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

lgtm

Tue, May 28, 4:51 PM · Restricted Project

Mon, May 27

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

lgtm

Mon, May 27, 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

Sep 26 2018

mcgrathr accepted D52562: [lib/fuzzer] Fix logging for Fuchsia.

lgtm

Sep 26 2018, 11:46 AM

Sep 23 2018

mcgrathr accepted D52402: [CMake][Fuchsia] Use internal_linkage rather than always_inline for libc++.

lgtm as long as we revert when the underlying bugs are resovled

Sep 23 2018, 1:05 AM

Sep 19 2018

mcgrathr accepted D52242: [sanitizer][fuchsia] Fix VMAR leak.

lgtm

Sep 19 2018, 11:59 AM

Sep 18 2018

mcgrathr added inline comments to D52242: [sanitizer][fuchsia] Fix VMAR leak.
Sep 18 2018, 2:12 PM
mcgrathr added inline comments to D52242: [sanitizer][fuchsia] Fix VMAR leak.
Sep 18 2018, 1:03 PM

Sep 17 2018

mcgrathr added a comment to D52202: [ELF] Use the Repl point to avoid the segfault when using ICF.

The test will need to verify that the emitted relocs are correct too.

Sep 17 2018, 8:23 PM

Sep 16 2018

mcgrathr accepted D52155: [sanitizer_common] Fuchsia now supports .preinit_array.

lgtm. Note that most sanitizer runtime ports for Fuchsia will be DSO-only, but I think the #ifndef PIC condition will cover that if/when those cases come up.
For always-static runtimes that can't be statically linked into DSOs (i.e. runtime library is not included in -shared links) this should now be right for Fuchsia.
IIRC profile is always-static but meant to be statically linked separately into each DSO, but presumably that means it's built with PIC defined too.

Sep 16 2018, 2:36 PM

Sep 14 2018

mcgrathr accepted D52132: [CMake] Use cannonical triples for Fuchsia runtimes.
Sep 14 2018, 7:46 PM · Restricted Project

Sep 11 2018

mcgrathr added inline comments to D51961: [objcopy] make objcopy follow program header standards.
Sep 11 2018, 6:41 PM

Sep 8 2018

mcgrathr accepted D51834: [ELF] Don't emit .relr.dyn section if there are no relocs.
Sep 8 2018, 4:03 PM
mcgrathr created D51833: ELF: Add --build-id-link-dir=DIR switch.
Sep 8 2018, 3:20 PM · lld

Sep 4 2018

mcgrathr added inline comments to D51329: [Attribute/Diagnostics] Print macro if definition is an attribute declaration.
Sep 4 2018, 5:16 PM · Restricted Project

Sep 1 2018

mcgrathr accepted D51573: [Driver] Search LibraryPaths when handling -print-file-name.

The log message could give concrete examples.

Sep 1 2018, 5:18 PM

Aug 26 2018

mcgrathr accepted D51266: [sanitizer][fuzzer] Transition back to ZX_TIME_INFINITE.

lgtm

Aug 26 2018, 2:51 PM

Aug 1 2018

mcgrathr added a comment to D49623: Passes for Logging Stackdepth Info.

I thought the plan was to produce JSON, not YAML.

Aug 1 2018, 5:06 PM

Jul 27 2018

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

I think this wants to be a hard error rather than a warning. Though since we use -Werror anyway if others feel strongly contrary I won't object.

Jul 27 2018, 3:52 PM · Restricted Project

Jul 23 2018

mcgrathr accepted D49697: [sanitizer][fuzzer] Temporarily transition to ZX_TIME_INFINITE_OLD.

lgtm

Jul 23 2018, 5:18 PM
mcgrathr accepted D49694: [sanitizer] Transition from _zx_vmar_... to _zx_vmar_..._old calls.

lgtm

Jul 23 2018, 5:14 PM
mcgrathr accepted D47208: [profile] Support profiling runtime on Fuchsia.

lgtm

Jul 23 2018, 5:13 PM
mcgrathr added inline comments to D47208: [profile] Support profiling runtime on Fuchsia.
Jul 23 2018, 4:10 PM

Jul 14 2018

mcgrathr added a comment to D42019: [Driver] Set default sysroot for Fuchsia if none is specified.

Is this still live? Should it be different after all the multiarch stuff?

Jul 14 2018, 2:33 AM
mcgrathr added a comment to D47208: [profile] Support profiling runtime on Fuchsia.

LGTM with these few substantive nits and then a whole lot more comments (mostly a more thorough overview covering linkage and output model).

Jul 14 2018, 2:30 AM
mcgrathr accepted D48509: Improve crash unwinding on Fuchsia.

Looks great!

Jul 14 2018, 12:30 AM

Jul 10 2018

mcgrathr added a comment to D48509: Improve crash unwinding on Fuchsia.

Almost there, but some nits.

Jul 10 2018, 5:12 PM

Jun 26 2018

mcgrathr accepted D48564: [CMake] Support passing FUCHSIA_SDK as the only variable.

lgtm

Jun 26 2018, 8:36 PM
mcgrathr accepted D48563: [CMake] Use explicit targets for building Linux runtimes.

lgtm

Jun 26 2018, 8:35 PM

Jun 18 2018

mcgrathr added inline comments to D48247: lld: add experimental support for SHT_RELR sections..
Jun 18 2018, 3:29 PM

Jun 15 2018

mcgrathr accepted D48208: [Fuchsia] Enable static libc++, libc++abi, libunwind.

lgtm

Jun 15 2018, 1:20 PM

Jun 11 2018

mcgrathr added a comment to D47919: llvm-readobj: add experimental support for SHT_RELR sections..

In response to Jake's comments about GNUStyle:

Jun 11 2018, 7:14 PM

Jun 7 2018

mcgrathr accepted D47866: [Fuzzer] Update the header path for fdio/spawn.h on Fuchsia.

lgtm

Jun 7 2018, 10:44 AM
mcgrathr accepted D47848: [Support] Link libzircon.so when building LLVM for Fuchsia.

lgtm

Jun 7 2018, 10:44 AM

Jun 5 2018

mcgrathr accepted D47753: [Support] Use zx_cache_flush on Fuchsia to flush instruction cache.

lgtm

Jun 5 2018, 1:10 PM
mcgrathr accepted D47758: [Fuchsia] Include install-distribution-stripped in bootstrap targets.

lgtm

Jun 5 2018, 12:35 PM