mcgrathr (Roland McGrath)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Sat, Jul 14

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?

Sat, Jul 14, 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).

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

Looks great!

Sat, Jul 14, 12:30 AM

Tue, Jul 10

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

Almost there, but some nits.

Tue, Jul 10, 5:12 PM

Tue, Jun 26

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

lgtm

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

lgtm

Tue, Jun 26, 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

Jun 4 2018

mcgrathr accepted D47668: [Driver][Fuchsia] Pass LTO flags to linker.

lgtm

Jun 4 2018, 1:24 PM

Jun 1 2018

mcgrathr accepted D47649: [Fuzzer] Migrate Fuchsia port from launchpad to fdio_spawn.

lgtm modulo unnecessary #include

Jun 1 2018, 12:49 PM

May 30 2018

mcgrathr added a comment to D47208: [profile] Support profiling runtime on Fuchsia.

I don't have enough context on how the profiling runtime works to properly review this implementation strategy for Fuchsia. Perhaps adequate comments here would tell me enough. Certainly it needs a lot more comments.
But probably we just need to talk through the implementation plan in person first.

May 30 2018, 4:50 PM

May 23 2018

mcgrathr accepted D47170: [fuchsia] Add line buffering in RawWrite.

lgtm

May 23 2018, 3:19 PM

May 22 2018

mcgrathr added inline comments to D47170: [fuchsia] Add line buffering in RawWrite.
May 22 2018, 4:43 PM

May 21 2018

mcgrathr added inline comments to D47170: [fuchsia] Add line buffering in RawWrite.
May 21 2018, 5:44 PM

May 9 2018

mcgrathr accepted D46462: [sanitizer] Allow Fuchsia symbolizer to be reused by Myriad RTEMS.

LGTM assuming the Fuchsia build still works (I don't see any problems in the source off hand).
@jakehehrlich will verify the Fuchsia Build.

May 9 2018, 11:43 AM
mcgrathr accepted D46609: [CMake] Build shared version of ASan and UBSan for Fuchsia.
May 9 2018, 11:28 AM

May 4 2018

mcgrathr updated subscribers of D46462: [sanitizer] Allow Fuchsia symbolizer to be reused by Myriad RTEMS.

@jakehehrlich is working on an open-source tool at https://fuchsia.googlesource.com/tools that handles this output format as specified in https://fuchsia.googlesource.com/zircon/+/master/docs/symbolizer_markup.md

May 4 2018, 2:34 PM
mcgrathr added a comment to D46462: [sanitizer] Allow Fuchsia symbolizer to be reused by Myriad RTEMS.

I'd prefer to keep all the markup string constants together. Moving them all to a header would be fine, but I think it should be a separate sanitizer_symbolizer_fuchsia.h so that non-Fuchsia builds aren't using symbolizer_fuchsia.h, which is about OS-specific stuff distinct from the symbolizer markup format. But nothing in this patch actually needs the constant to be visible outside sanitizer_symbolizer_fuchsia.cc, so I'd like to see some explanation of the need for that.

May 4 2018, 2:33 PM

May 2 2018

mcgrathr accepted D46345: [Support] Support building LLVM for Fuchsia.

lgtm with a comment

May 2 2018, 10:59 AM
mcgrathr accepted D46361: [CMake][Cache] Stop pretending that Fuchsia is UNIX.

fnu lgtm

May 2 2018, 10:55 AM

May 1 2018

mcgrathr created D46344: [sanitizer] Fix Fuchsia ReadBinaryName not to crash when uninitialized.
May 1 2018, 6:35 PM · Restricted Project

Apr 27 2018

mcgrathr accepted D46215: [AArch64] Support reserving x16 and x17 register.

I don't know the backend or the register allocator at all but this looks like a thorough parallel of the existing model for -ffixed-x18.

Apr 27 2018, 3:31 PM

Apr 25 2018

mcgrathr accepted D46075: [CMake] Enable libc++ for Fuchsia toolchain on Darwin.
Apr 25 2018, 11:17 AM

Apr 24 2018

mcgrathr accepted D45997: [CMake] Pass additional CMake flags in Fuchsia cache files.
Apr 24 2018, 11:04 AM

Apr 19 2018

mcgrathr accepted D45852: [Fuzzer] Add a missing header in Fuchsia implementation.

We should revisit the implicit dependency on fdio. For other compiler-rt libraries' Fuchsia ports, we've kept it at the pure vDSO and minimal libc ABIs.

Apr 19 2018, 5:36 PM

Apr 11 2018

mcgrathr accepted D45525: [sanitizer] Correct name length computation for some Fuchsia vmos.

lgtm

Apr 11 2018, 11:45 AM

Mar 30 2018

mcgrathr accepted D45105: [AArch64] Reserve x18 register on Fuchsia.

lgtm

Mar 30 2018, 1:59 PM

Mar 27 2018

mcgrathr added a comment to D44953: [sanitizer] Remove empty Symbolizer PrepareForSandboxing.

lgtm but I'm not authoritative.

Mar 27 2018, 1:43 PM
mcgrathr accepted D44947: [Driver] Add fuzzer-no-link into the list of supported Fuchsia sanitizers.

lgtm

Mar 27 2018, 1:35 PM

Mar 22 2018

mcgrathr accepted D44770: [sanitizer] zx_vmo_write on Fuchsia takes only 4 arguments now.

Lgtm

Mar 22 2018, 8:37 AM

Mar 20 2018

mcgrathr accepted D44724: [Fuchsia] Don't install libc++, libc++abi or libunwind on Darwin.

Lgtm with some more comments

Mar 20 2018, 11:09 PM
mcgrathr added a comment to D44724: [Fuchsia] Don't install libc++, libc++abi or libunwind on Darwin.

When targetting Darwin we should really not be even compiling libc++/libc++abi and certainly should not have anything else compiling against their headers since at runtime the system libraries will be used and so things better compile against the system headers that match.
Do these settings accomplish that?

Mar 20 2018, 7:32 PM

Mar 18 2018

mcgrathr added a comment to D44605: [Driver] Default to DWARF 5 for Fuchsia.

We aren't actually using DWARF 5 yet AFAICT. Our builds don't pass -gdwarf-5. So I'm not sure we have yet verified that all the DWARF-consuming tools people are using with Fuchsia binaries can handle all of DWARF 5 (which has several major format changes). I'd certainly like 5 to be the default, but I think we need to establish a set of consumers we care about and verify their format version support before we can be sure about this.

Mar 18 2018, 2:35 AM

Mar 14 2018

mcgrathr added a comment to D44349: [WebAssembly] Verify contents of relocations target before writing it.

So Sam and I had dinner together. :-) I don't know a lot about WebAssembly and Sam doesn't know a lot about traditional (e.g. ELF) linkers, so we talked about the parallels in somewhat general terms while explaining the specifics to each other without distracting from our dessert.
In talking about what WebAssembly wants to do with converting some kinds of references to other kinds, I described the obvious analogy to the various kinds of linker relaxation that have been done in traditional machine code linking such as ELF.
There are two precedents that I can cite in detail off hand. In both cases the reloc type is specified in the ABI as for use with certain instruction patterns and the linker is expected to rewrite instructions with complete knowledge of their meaning, rather than just deliver values into bitfields.
One is x86-64's GOTPCREL and GOTPCRELX relocs, where GNU linkers check for expected opcode bytes immediate prior to the r_offset location and silently forego the instruction-rewriting optimization if the reloc is not used in the expected context.
The other is x86-64's TLS relocs, where GNU linkers check the opcode bytes immediatley prior to the r_offset location and diagnose an error if they don't match specific instruction patterns prescribed in the ABI for each reloc that enables linker relaxation (IIRC for some cases the x86 ABI mandates that linkers perform the relaxation, rather than just describing how the optimization is possible).
I don't know off hand how LLD handles these cases, but I would say that matching the GNU linkers' behavior is right--with the possible caveat that perhaps there should be a warning emitting for using GOTPCRELX relocs with instructions the linker doesn't know how to rewrite.
I know that other machines have many more relocs that are prescribed in their ABIs as for use with specific instruction patterns, but I don't know off hand what the details of those are, nor how existing linkers handle situations where such relocs are used with mismatching instructions.

Mar 14 2018, 2:59 PM

Mar 5 2018

mcgrathr accepted D44126: [sanitizer] Fix the return type for GetTid in Fuchsia implementation.

lgtm

Mar 5 2018, 4:44 PM

Mar 1 2018

mcgrathr accepted D43992: [Frontend] Avoid including default system header paths on Fuchsia.

lgtm

Mar 1 2018, 10:31 PM

Jan 12 2018

mcgrathr added a comment to D42022: Reland "Make TracePcGuardController linker-initialized".

I wonder if cpplint.py is worthwhile any more. Perhaps it should be replaced by a check that clang-format doesn't decide to change anything.

Jan 12 2018, 5:13 PM · Restricted Project
mcgrathr added a comment to D42022: Reland "Make TracePcGuardController linker-initialized".

clang-format and lib/sanitizer_common/scripts/cpplint.py disagree about whitespace before braces.

Jan 12 2018, 5:09 PM · Restricted Project
mcgrathr added a project to D42022: Reland "Make TracePcGuardController linker-initialized": Restricted Project.
Jan 12 2018, 5:09 PM · Restricted Project
mcgrathr added inline comments to D42019: [Driver] Set default sysroot for Fuchsia if none is specified.
Jan 12 2018, 4:58 PM
mcgrathr added a comment to D39348: Implement --just-symbols..

ping. Will this or D41277 land?

Jan 12 2018, 2:51 PM

Dec 21 2017

mcgrathr created D41513: [SanitizerCoverage][Fuchsia] Make TracePcGuardController linker-initialized.
Dec 21 2017, 1:54 PM · Restricted Project

Dec 20 2017

mcgrathr created D41475: [sanitizer] Make function declarations C-compatible.
Dec 20 2017, 6:31 PM · Restricted Project
mcgrathr created D41471: [CMake][Fuchsia] Enable assertions.
Dec 20 2017, 4:47 PM
mcgrathr added a comment to D41277: Implement --just-symbols.

This works fine for me. Perhaps Rui should land D39348 first and then you can rebase this in terms of that to discuss the remaining features (-R alias, no input files case).

Dec 20 2017, 4:38 PM

Dec 19 2017

mcgrathr accepted D41413: [scudo] Add -fsanitize=scudo option to Fuchsia..

Mutual exclusion is not target-specific and is already handled in SanitizerArgs. The test coverage for that looks fine.

Dec 19 2017, 5:12 PM

Dec 18 2017

mcgrathr accepted D39348: Implement --just-symbols..

Works for me. D41277 works just as well. I don't know how you plan to reconcile the two.

Dec 18 2017, 3:42 PM

Dec 1 2017

mcgrathr added a comment to D36493: [MC] Handle unknown literal register numbers in .cfi_* directives.

Great! Please land it for me.

Dec 1 2017, 1:21 PM

Nov 30 2017

mcgrathr added a comment to D40652: [ELF] - Produce relocation section name consistent with output section name when --emit-reloc used with linker script..

This change as is fixes my real-world case.
Thanks! Now I'm just blocked on --just-symbols...

Nov 30 2017, 3:18 PM
mcgrathr added a comment to D40657: [sanitizer] Introduce a vDSO aware time function, and use it in the allocator.

Fuchsia bit LGTM. Linux bit highly suspect.

Nov 30 2017, 11:47 AM

Nov 27 2017

mcgrathr accepted D40329: [CMake][Fuchsia] Disable terminfo database in Fuchsia toolchain.

lgtm

Nov 27 2017, 11:36 AM

Nov 21 2017

mcgrathr accepted D40319: [libcxx] Support getentropy as a source of randomness for std::random_device.

LGTM

Nov 21 2017, 1:34 PM
mcgrathr accepted D39831: [Driver] Make the use of relax relocations a per target option.

LGTM

Nov 21 2017, 12:57 PM
mcgrathr added a comment to D39831: [Driver] Make the use of relax relocations a per target option.

test case?

Nov 21 2017, 12:02 PM

Nov 20 2017

mcgrathr added inline comments to D38595: [fuchsia] Update Fuchsia with a new mmap implementation..
Nov 20 2017, 1:13 PM

Nov 17 2017

mcgrathr accepted D39930: [CMake] Use libc++ and compiler-rt as default libraries in Fuchsia toolchain.

LGTM with comment added but if a different approach will be required for multiple Linux targets (which we all treat as "hostish") then it's worth at least thinking about that now.

Nov 17 2017, 11:31 PM
mcgrathr accepted D40201: [llvm-objcopy] Add --strip-all-gnu and change --strip-all.

LGTM. I wonder if it's really worth keeping --strip-non-alloc.

Nov 17 2017, 4:01 PM

Nov 10 2017

mcgrathr added a comment to D39930: [CMake] Use libc++ and compiler-rt as default libraries in Fuchsia toolchain.

In principle this should be OK since we're unconditionally building the runtimes for default.
However, this means that any dynamically-linked host builds will assume our toolchain-provided shared libraries are available on the runtime host in standard library places.
I don't think we want that. Perhaps we could make -static-libstdc++ default in our host targets. But that's not really enough, since compiler-rt has some too. Probably better to just disable building the shared libraries altogether for the host targets.

Nov 10 2017, 5:19 PM

Oct 26 2017

mcgrathr added inline comments to D39348: Implement --just-symbols..
Oct 26 2017, 5:36 PM
mcgrathr added a comment to D39348: Implement --just-symbols..

This doesn't implement the -R switch name, which is the traditional interface from time immemorial. I can use --just-symbols, but you really should handle -R too.

Oct 26 2017, 5:23 PM

Oct 24 2017

mcgrathr accepted D39273: [CMake] Build host builtins in Fuchsia toolchain even on Darwin.

lgtm

Oct 24 2017, 6:22 PM · Restricted Project
mcgrathr accepted D39270: [CMake] Include clang-refactor in Fuchsia toolchain.

lgtm

Oct 24 2017, 6:13 PM · Restricted Project

Oct 23 2017

mcgrathr accepted D39176: [Driver] Use ld.lld directly for Fuchsia rather than passing flavor.

This seems like how it always should have been, since -fuse-ld=lld sets it to ld.lld, not lld.
Looks to me like the existing MipsLinux and WebAssembly implementations are wrong too and only BareMetal is right.

Oct 23 2017, 1:13 PM · Restricted Project

Oct 21 2017

mcgrathr added a comment to D38724: [ELF] - Do not collect SHT_REL[A] sections unconditionally when --gc-sections and --emit-relocs used together..

This change works on my case, but it names the reloc sections it emits based on the first input section in the output section being relocated rather than on the output section itself.
e.g. in my -ffunction-sections --gc-sections --emit-relocs build, the SHT_RELA section with sh_info pointing to the .text section is called .rela.text.first_function_in_text rather than .rela.text.

Oct 21 2017, 3:06 AM

Oct 17 2017

mcgrathr accepted D39017: [CMake] Build Fuchsia toolchain as -O3 .

LGTM
Since we use lld, -Wl,-O2 also helps make the binaries smaller.

Oct 17 2017, 12:41 PM

Oct 15 2017

mcgrathr added a comment to D38907: Give .note.gnu.build-id section alignment 4.

http://www.sco.com/developers/gabi/latest/ch5.pheader.html#note_section describes alignment of the contents of notes. Maintaining that alignment is obviously impossible if the section is not so aligned.

Oct 15 2017, 5:43 PM · lld

Oct 13 2017

mcgrathr created D38907: Give .note.gnu.build-id section alignment 4.
Oct 13 2017, 4:06 PM · lld

Oct 11 2017

mcgrathr added inline comments to D38759: [fuchsia] Add loop to _zx_cprng_draw..
Oct 11 2017, 11:48 AM

Oct 10 2017

mcgrathr accepted D38759: [fuchsia] Add loop to _zx_cprng_draw..

LGTM

Oct 10 2017, 1:46 PM

Oct 9 2017

mcgrathr added inline comments to D38437: Introduce ReservedAddressRange to sanitizer_common..
Oct 9 2017, 2:46 PM

Oct 8 2017

mcgrathr added a comment to D38669: [sanitizer] Don't intercept signal and sigaction on Fuchsia.

I wonder why my builds have never had an undefined symbol HandleDeadlySignal.

Oct 8 2017, 1:13 AM

Oct 7 2017

mcgrathr requested changes to D38669: [sanitizer] Don't intercept signal and sigaction on Fuchsia.

What effect does this have?
Nothing built for Fuchsia ought to be using sanitizer_signal_interceptors.inc or HandleDeadlySignal in the first place.

Oct 7 2017, 8:24 PM

Oct 5 2017

mcgrathr added inline comments to D38437: Introduce ReservedAddressRange to sanitizer_common..
Oct 5 2017, 3:27 PM
mcgrathr added inline comments to D38437: Introduce ReservedAddressRange to sanitizer_common..
Oct 5 2017, 3:26 PM
mcgrathr added inline comments to D38595: [fuchsia] Update Fuchsia with a new mmap implementation..
Oct 5 2017, 3:25 PM
mcgrathr added a comment to D38592: Update sanitizer_allocator to use new API..

Mostly LGTM

Oct 5 2017, 3:19 PM

Sep 12 2017

mcgrathr created D37785: [Fuchsia] Set ENABLE_X86_RELAX_RELOCATIONS for Fuchsia builds.
Sep 12 2017, 7:01 PM
mcgrathr accepted D37767: [llvm-objcopy] Add e_machine validity check for reserved section indexes.

LGTM

Sep 12 2017, 4:21 PM
mcgrathr added inline comments to D37767: [llvm-objcopy] Add e_machine validity check for reserved section indexes.
Sep 12 2017, 3:56 PM
mcgrathr created D37770: [Fuchsia] Magenta -> Zircon.
Sep 12 2017, 1:54 PM · Restricted Project
mcgrathr created D37763: [Fuchsia] Magenta -> Zircon.
Sep 12 2017, 1:36 PM

Sep 11 2017

mcgrathr created D37723: [Driver] Fuchsia targets default to -fasynchronous-unwind-tables.
Sep 11 2017, 4:56 PM

Sep 2 2017

mcgrathr accepted D37391: [yaml2obj][ELF] Add support for symbol indexes greater than SHN_LORESERVE.

Not being expert in the yaml2obj code base, this looks good to me with the caveats below.

Sep 2 2017, 1:21 AM
mcgrathr accepted D37393: [llvm-objcopy] Add support for special section indexes in symbol table greater than SHN_LORESERVE.

I'm not closely familiar with this code base, so it should probably still get some review from someone who has reviewed the objcopy implementation so far.
But just looking at the apparent logic in this change, it looks OK to me with the caveats noted below.

Sep 2 2017, 1:04 AM

Sep 1 2017

mcgrathr added inline comments to D37391: [yaml2obj][ELF] Add support for symbol indexes greater than SHN_LORESERVE.
Sep 1 2017, 2:05 PM
mcgrathr added inline comments to D37391: [yaml2obj][ELF] Add support for symbol indexes greater than SHN_LORESERVE.
Sep 1 2017, 1:05 PM
mcgrathr requested changes to D37393: [llvm-objcopy] Add support for special section indexes in symbol table greater than SHN_LORESERVE.
Sep 1 2017, 12:57 PM
mcgrathr requested changes to D37391: [yaml2obj][ELF] Add support for symbol indexes greater than SHN_LORESERVE.
Sep 1 2017, 12:39 PM

Aug 29 2017

mcgrathr updated subscribers of D37229: When built with ASan, __cxa_throw calls __asan_handle_no_return.
Aug 29 2017, 1:47 PM · Restricted Project
mcgrathr updated subscribers of D37273: [sanitizer_common][Fuchsia] Update Fuchsia sanitizer markup.
Aug 29 2017, 1:47 PM · Restricted Project
mcgrathr created D37273: [sanitizer_common][Fuchsia] Update Fuchsia sanitizer markup.
Aug 29 2017, 1:46 PM · Restricted Project

Aug 28 2017

mcgrathr added a comment to D37229: When built with ASan, __cxa_throw calls __asan_handle_no_return.

Re-land of https://reviews.llvm.org/D36599
Actually builds now that compiler-rt has been updated.

Aug 28 2017, 1:11 PM · Restricted Project
mcgrathr created D37229: When built with ASan, __cxa_throw calls __asan_handle_no_return.
Aug 28 2017, 1:10 PM · Restricted Project
mcgrathr accepted D37218: Enable GetRandom for Fuchsia sanitizer..

LGTM

Aug 28 2017, 1:04 PM
mcgrathr requested changes to D37218: Enable GetRandom for Fuchsia sanitizer..
Aug 28 2017, 12:16 PM