Page MenuHomePhabricator

srhines (Stephen Hines)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 23 2012, 7:46 PM (333 w, 6 d)

Recent Activity

Tue, Jan 8

srhines accepted D56456: [Driver] Default to -fno-addrsig on Android..
Tue, Jan 8, 2:28 PM
srhines accepted D55953: Android is not GNU, so don't claim that it is..

Sorry about the delay.

Tue, Jan 8, 1:36 PM

Dec 11 2018

srhines accepted D55587: [hwasan] Verify Android TLS slot at startup..
Dec 11 2018, 4:56 PM

Dec 7 2018

srhines added inline comments to D34796: upporting -f(no)-reorder-functions flag, clang side change.
Dec 7 2018, 10:15 AM

Dec 3 2018

srhines added a comment to D53850: Declares __cpu_model as dso local.

Craig, does this look ok now?

Dec 3 2018, 5:27 PM

Nov 30 2018

srhines updated subscribers of D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.
Nov 30 2018, 11:48 AM

Nov 28 2018

srhines added a comment to D55029: set default max-page-size to 4KB in lld for Android Aarch64.

I wouldn't rush to submit this now, given that this issue is not new at all. Maybe we can just wait for Peter's response?

Nov 28 2018, 3:09 PM
srhines accepted D55029: set default max-page-size to 4KB in lld for Android Aarch64.

Thanks Zhizhou! I'm curious about Peter's answer to the 64KiB default as well, but I think we should move forward with getting this patch submitted.

Nov 28 2018, 2:52 PM

Nov 16 2018

srhines added a comment to D52025: [TargetLowering] Android has sincos functions.

I just wanted to follow up here.

Nov 16 2018, 7:51 PM

Nov 1 2018

srhines added a comment to D53906: [ARM][AArch64] Increase TLS alignment to reserve space for Android's TCB.

Yabin did some of the work to bring TSan to Android. Perhaps he knows whether this is has significant use. Considering that it is meant as a debugging aide, I would actually be comfortable with moving it. You are also correct that we never officially marked it as a feature of the NDK (or platform). That is something we want to do though.

Nov 1 2018, 5:25 PM
srhines updated subscribers of D53906: [ARM][AArch64] Increase TLS alignment to reserve space for Android's TCB.
Nov 1 2018, 5:23 PM

Oct 26 2018

srhines updated subscribers of D53765: [RFC prototype] Implementation of asm-goto support in LLVM.
Oct 26 2018, 11:24 PM

Oct 25 2018

srhines added a comment to D53742: [XRay] Use std::errc::invalid_argument instead of std::errc::bad_message.

LGTM, but please wait for dberris approval.

Oct 25 2018, 9:41 PM

Oct 24 2018

srhines updated subscribers of D53486: [libcxx] Only define __libcpp_is_floating_point<_Float16> for Clang.
Oct 24 2018, 9:19 AM

Oct 22 2018

srhines accepted D53529: [Driver] fix broken test.
Oct 22 2018, 2:25 PM

Oct 19 2018

srhines accepted D53463: [Driver] allow Android triples to alias for non Android targets.

Please remove the reference to b/X in the commit message. LLVM doesn't allow internal bug numbers, and you described the issue well enough without it.

Oct 19 2018, 6:14 PM

Oct 10 2018

srhines accepted D53118: [Driver] Fix --hash-style choice for Android..
Oct 10 2018, 6:28 PM
srhines accepted D53117: [Driver] Default to `-z now` and `-z relro` on Android..
Oct 10 2018, 6:25 PM
srhines added a comment to D53121: [Driver] Add defaults for Android ARM FPUs..

This LGTM, but we should wait to hear from Kristof before submitting.

Oct 10 2018, 6:23 PM
srhines added a reviewer for D53121: [Driver] Add defaults for Android ARM FPUs.: kristof.beyls.
Oct 10 2018, 6:21 PM
srhines accepted D53109: [Driver] Default Android toolchains to libc++..

Really cool! Thanks for making everything easier to use out-of-the-box.

Oct 10 2018, 6:19 PM

Oct 8 2018

srhines added a reviewer for D53003: [ELF] Fix link failure with Android compressed relocation support.: rprichard.
Oct 8 2018, 4:04 PM

Oct 2 2018

srhines committed rCRT343599: [sanitizer] Use -Wl,-z,global on Android for sanitizers except UBsan.
[sanitizer] Use -Wl,-z,global on Android for sanitizers except UBsan
Oct 2 2018, 9:22 AM
srhines committed rL343599: [sanitizer] Use -Wl,-z,global on Android for sanitizers except UBsan.
[sanitizer] Use -Wl,-z,global on Android for sanitizers except UBsan
Oct 2 2018, 9:22 AM
srhines closed D52770: [sanitizer] Use -Wl,-z,global on Android for sanitizers except UBsan.
Oct 2 2018, 9:21 AM
srhines created D52770: [sanitizer] Use -Wl,-z,global on Android for sanitizers except UBsan.
Oct 2 2018, 12:17 AM

Sep 20 2018

srhines added inline comments to D52248: [SEMA] ignore duplicate declaration specifiers from typeof exprs.
Sep 20 2018, 1:48 PM

Sep 19 2018

srhines accepted D52251: [builtins] Add __emutls_unregister_key function.

+enh and danalbert - in case they have any other concerns about this (Ryan's already on the review). From my perspective, everything here makes sense for resolving this issue.

Sep 19 2018, 3:13 PM
srhines updated subscribers of D52251: [builtins] Add __emutls_unregister_key function.
Sep 19 2018, 3:09 PM

Sep 18 2018

srhines added a comment to D52191: Fix logic around determining use of frame pointer with -pg..

Thanks @dblaikie for the quick fixup. I must have accidentally dropped the '!', because I did run check-all to test the change.

Sep 18 2018, 1:21 PM
srhines committed rC342501: Fix logic around determining use of frame pointer with -pg..
Fix logic around determining use of frame pointer with -pg.
Sep 18 2018, 11:38 AM
srhines committed rL342501: Fix logic around determining use of frame pointer with -pg..
Fix logic around determining use of frame pointer with -pg.
Sep 18 2018, 11:38 AM
srhines closed D52191: Fix logic around determining use of frame pointer with -pg..
Sep 18 2018, 11:38 AM
srhines closed D52191: Fix logic around determining use of frame pointer with -pg..
Sep 18 2018, 11:38 AM
srhines added a comment to D52191: Fix logic around determining use of frame pointer with -pg..

Sure, looks good. Though my other/vague concern is why does this case error about fomit-frame-pointer having no effect, but other things (like using -fomit-frame-pointer on a target that requires frame pointers) that ignore -fomit-frame-pointer don't? Weird. But it probably makes sense somehow.

Sep 18 2018, 11:34 AM

Sep 17 2018

srhines created D52191: Fix logic around determining use of frame pointer with -pg..
Sep 17 2018, 1:58 PM
srhines added a comment to D50297: Align AArch64 and i386 image base to superpage.

+Ryan in case there is anything here that could affect Bionic loading from these pages.

Sep 17 2018, 10:53 AM
srhines added a reviewer for D50297: Align AArch64 and i386 image base to superpage: rprichard.
Sep 17 2018, 10:49 AM
srhines accepted D52163: -S as an alias for --strip-all-gnu.

Thanks again! @alexshap feel free to submit this or I can do it in the morning tomorrow (can't stay up any later to watch for unlikely breakage, etc.).

Sep 17 2018, 2:25 AM
srhines added inline comments to D52163: -S as an alias for --strip-all-gnu.
Sep 17 2018, 1:59 AM
srhines added a comment to D52163: -S as an alias for --strip-all-gnu.

Thanks for noticing this missing feature, as well as for the patch! You really should add a simple test to test/tools/llvm-objcopy/strip-all-gnu.test so that this doesn't ever regress. The test strip-all-and-keep-symbol.test is an example that has multiple RUN lines. Here is what you need to change the strip-all-gnu.test to (I believe) in order to get this working:

Sep 17 2018, 1:35 AM

Sep 13 2018

srhines committed rL342165: Support -fno-omit-frame-pointer with -pg..
Support -fno-omit-frame-pointer with -pg.
Sep 13 2018, 12:51 PM
srhines committed rC342165: Support -fno-omit-frame-pointer with -pg..
Support -fno-omit-frame-pointer with -pg.
Sep 13 2018, 12:51 PM
srhines closed D51713: Support -fno-omit-frame-pointer with -pg..
Sep 13 2018, 12:51 PM
srhines added inline comments to D18086: Fix default processor name for armv6k..
Sep 13 2018, 12:33 PM

Sep 12 2018

srhines added a comment to D51713: Support -fno-omit-frame-pointer with -pg..

What is the call generated with -pg for AMR32, gnu_mcount_nc or _mount? gnu_mcount_nc with "-pg" is known to be broken ( https://bugs.llvm.org/show_bug.cgi?id=33845)

Sep 12 2018, 2:41 PM

Sep 7 2018

srhines accepted D51693: ADT: add <bit> header, implement C++20 bit_cast, use.

Union punning is one of my pet peeves, so I am glad to see this. bit_cast seems generally useful too, so thanks for adding it.

Sep 7 2018, 11:33 AM

Sep 5 2018

srhines added a comment to D51671: [ELF] Set Out::TlsPhdr earlier for encoding packed reloc tables.

Thanks for the update. I'll let the project maintainers give a final LGTM, but I'm happy with it.

Sep 5 2018, 7:55 PM
srhines added a comment to D51713: Support -fno-omit-frame-pointer with -pg..

http://b/32510864 was the internal bug request, so I am noting it here for future reference, but I think that the patch itself is pretty self-explanatory (rather than filing a distinct upstream bug about this issue).

Sep 5 2018, 7:40 PM
srhines added a comment to D51713: Support -fno-omit-frame-pointer with -pg..

This was discovered in the Android build system (which passes -fomit-frame-pointer by default for ARM configurations. This leads to the inability to specify -pg, since there is no way to override the mere presence of -fomit-frame-pointer.

Sep 5 2018, 7:39 PM
srhines updated subscribers of D51713: Support -fno-omit-frame-pointer with -pg..
Sep 5 2018, 7:37 PM
srhines created D51713: Support -fno-omit-frame-pointer with -pg..
Sep 5 2018, 7:37 PM
srhines added inline comments to D51671: [ELF] Set Out::TlsPhdr earlier for encoding packed reloc tables.
Sep 5 2018, 2:35 AM
srhines added a comment to D51671: [ELF] Set Out::TlsPhdr earlier for encoding packed reloc tables.

Thanks for the detailed explanations. I only had one minor nitpick.

Sep 5 2018, 1:04 AM

Aug 31 2018

srhines added inline comments to D51538: Add glibc_prereq to platform limits mmsghdr.
Aug 31 2018, 3:23 PM
srhines added inline comments to D51538: Add glibc_prereq to platform limits mmsghdr.
Aug 31 2018, 3:05 PM
srhines added a comment to D51538: Add glibc_prereq to platform limits mmsghdr.

Please upload patches with context, as it makes it easier to review. https://llvm.org/docs/Phabricator.html has instructions on how to do this (search for "context").

Aug 31 2018, 10:37 AM
srhines accepted D51521: Refactor Addlibgcc to make the when and what logic more straightfoward..

Thanks for cleaning this up and adding better checks for Android. :)

Aug 31 2018, 9:34 AM

Aug 30 2018

srhines added inline comments to D51502: [X86] Fix register resizings for inline assembly register operands..
Aug 30 2018, 1:11 PM

Aug 28 2018

srhines accepted D45588: Start reserving x18 by default on Android targets..

Sorry, this got buried in my inbox back in April right before EuroLLVM.

Aug 28 2018, 6:00 PM

Aug 21 2018

srhines accepted D51068: [Android] Default to -fno-math-errno.
Aug 21 2018, 5:28 PM
srhines added inline comments to D51002: [Tooling] Allow -flto flags and filter out -Wa, flags.
Aug 21 2018, 3:12 PM

Jul 26 2018

srhines committed rL338062: Handle the lack of a symbol table correctly..
Handle the lack of a symbol table correctly.
Jul 26 2018, 1:06 PM
srhines closed D49534: Handle the lack of a symbol table correctly..
Jul 26 2018, 1:05 PM
srhines added a comment to D49534: Handle the lack of a symbol table correctly..

@srhines - is there anything blocking this diff ?

Jul 26 2018, 1:01 PM

Jul 24 2018

srhines added a comment to D49751: Add maybe-unused attribute to a variable..

Definitely deal with the submodule stuff. We don't want a new file checked in for tools/clang.

Jul 24 2018, 1:19 PM

Jul 20 2018

srhines added a comment to D49534: Handle the lack of a symbol table correctly..

I restored the original change, since I had confused things with arcanist before. PTAL

Jul 20 2018, 3:50 PM
srhines updated the diff for D49534: Handle the lack of a symbol table correctly..

Revert back to not layering the positional argument change.

Jul 20 2018, 3:48 PM

Jul 19 2018

srhines abandoned D49537: Fix positional output argument for llvm-strip..

Gah, you are correct. Our Android build system had grown to depend on this behavior from llvm-strip previously. Doing an update now is how I am finding all of these issues. I'll fix it in our build system and abandon this CL. Thanks for the review.

Jul 19 2018, 12:38 PM
srhines updated the diff for D49537: Fix positional output argument for llvm-strip..

Switch to using .back() because I was lazy in the first upload. :)

Jul 19 2018, 4:30 AM
srhines created D49537: Fix positional output argument for llvm-strip..
Jul 19 2018, 4:17 AM
srhines updated the diff for D49534: Handle the lack of a symbol table correctly..
  • Fix positional output argument for llvm-strip.
Jul 19 2018, 4:06 AM
srhines updated the diff for D49534: Handle the lack of a symbol table correctly..

Ran git-clang-format and added rationale to the test.

Jul 19 2018, 1:55 AM
srhines created D49534: Handle the lack of a symbol table correctly..
Jul 19 2018, 1:51 AM

Jul 12 2018

srhines committed rL336921: Add --strip-all option back to llvm-strip..
Add --strip-all option back to llvm-strip.
Jul 12 2018, 10:47 AM
srhines closed D49226: Add --strip-all option back to llvm-strip..
Jul 12 2018, 10:47 AM
srhines created D49226: Add --strip-all option back to llvm-strip..
Jul 12 2018, 2:56 AM

Jul 10 2018

srhines committed rCRT336749: Add libcxxabi option back for sanitizer use..
Add libcxxabi option back for sanitizer use.
Jul 10 2018, 5:55 PM
srhines committed rL336749: Add libcxxabi option back for sanitizer use..
Add libcxxabi option back for sanitizer use.
Jul 10 2018, 5:55 PM
srhines closed D49157: Add libcxxabi option back for sanitizer use..
Jul 10 2018, 5:55 PM
srhines added a comment to D49157: Add libcxxabi option back for sanitizer use..

Thanks for the quick review.

Jul 10 2018, 5:47 PM
srhines added a comment to D49157: Add libcxxabi option back for sanitizer use..

https://reviews.llvm.org/D47100 is the commit where this got dropped.

Jul 10 2018, 3:07 PM
srhines updated subscribers of D49157: Add libcxxabi option back for sanitizer use..
Jul 10 2018, 3:06 PM
srhines created D49157: Add libcxxabi option back for sanitizer use..
Jul 10 2018, 3:05 PM

Jul 6 2018

srhines abandoned D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2..

Removed this in favor of the suggestions here. Setting the CMAKE_FIND_ROOT_PATH_MODE* variables does make this properly hermetic.

Jul 6 2018, 8:58 PM
srhines abandoned D48458: Respect CMAKE_SYSROOT and don't search out libxml2 if it is set..

Removed this in favor of the suggestions in https://reviews.llvm.org/D48459. Setting the CMAKE_FIND_ROOT_PATH_MODE* variables does make this properly hermetic.

Jul 6 2018, 8:58 PM

Jul 3 2018

srhines added a comment to D48538: Make __gcov_flush flush counters for all shared libraries.

On Tue, Jul 3, 2018 at 4:29 PM Marco Castelluccio via Phabricator <reviews@reviews.llvm.org> wrote:

marco-c updated this revision to Diff 154015.
marco-c added a comment.

I've taken another approach which looks simpler to me. Instead of having two lists, I only have a shared list and I use a static variable's address as an identifier for a dynamic object.

I've also added another test (which would have failed with the previous iteration of the patch) which dlopens three libraries.

I will file follow-up bugs for things that don't look right in the llvm-cov output which are not regressions from this patch:

- instrprof-shared-main-gcov-flush_no-writeout.c.gcov: lines 23, 26 and 30 should not be covered; lines 33 and 35 should not be covered (they are considered as uncoverable instead).
- instrprof-dlopen-func{2,3}.c.gcov: the only line in the function should be covered once and not thrice

Regarding removing writeout and only always using __gcov_flush, do you see any disadvantage if we do that?

writeout function only writes the profile for the current dynamic module which is different from the behavior of __gcov_flush. The form is expected when shared lib is unloaded -- i.e., do not dump other load module's profile unless being explicitly told so (to use gcov_flush). The write out is similar to gcc's gcov_exit. In short, we should keep it as it is.

Sorry, what I meant is that we could remove llvm_gcov_writeout and only keep llvm_gcov_flush (they are defined in lib/Transforms/Instrumentation/GCOVProfiling.cpp and passed to llvm_gcov_init). We currently have writeout functions and flush functions, where the difference is just that the flush function is also resetting the counters to 0.
At exit, instead of calling llvm_gcov_writeout, we could call llvm_gcov_flush. Since we are at exit, the fact that we reset the counters to 0 for the current module doesn't matter (as the module is not usable anymore after that).
At exit we would still call llvm_gcov_flush only for the module that is exiting, at gcov_flush we would call __llvm_gcov_flush for all modules.

Regarding llvm_gcov_flush that was recently added, should I remove it since we can now unhide __gcov_flush?

I suggest we keep it -- there is no harm keeping it. llvm_gcov_flush can be documented to be always non hidden. There might also be a use case that user want to discover the dumper using dlsym, but only want to dump profile for that shared library. In that case, a symbol with protected visibility is needed which is a wrapper to the write-out method.

Should we make llvm_gcov_flush hidden and gcov_flush unhidden then? gcov_flush unhidden would be for compatibility with GCC.

Jul 3 2018, 5:31 PM

Jun 29 2018

srhines added inline comments to D48791: [AArch64] Implement execute-only CodeGen Support..
Jun 29 2018, 3:09 PM
srhines added a comment to D48791: [AArch64] Implement execute-only CodeGen Support..

This is really cool. I just had one minor suggestion for testing, and will otherwise defer to the ARM64-related folks to accept the patch. The rest looks quite reasonable to me, though.

Jun 29 2018, 2:51 PM
srhines added a comment to D45454: Add llvm_gcov_flush to be called outside a shared library.

LGTM. Thanks for making the checking more clear. This should help bridge the gap for now, so that we can resume getting per-library profile data in Android.

Jun 29 2018, 2:28 PM
srhines added a comment to D45454: Add llvm_gcov_flush to be called outside a shared library.

Thanks for picking this up again and updating the change to add llvm_gcov_flush().

Jun 29 2018, 11:02 AM

Jun 26 2018

srhines added a comment to D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2..

Actually, I would imagine that if you're cross-compiling or using a custom sysroot, you should probably also specify CMAKE_FIND_ROOT_PATH and set CMAKE_FIND_ROOT_PATH_MODE_PACKAGE to ONLY, to limit all these searches to the desired directories?

(I'm actually playing around with a bunch of this stuff myself right now. I'm curious about your CMake setup for a custom sysroot or cross-compilation and what issues you've come across.)

Jun 26 2018, 9:13 PM
srhines added a comment to D48580: [AArch64] Support reserving x1-7 registers..

x1-x7 are argument registers in the calling convention; what's supposed to happen if there's a call in the code?

I can see two possibilities:

  1. We emit an error.
  2. We change the calling convention so it doesn't use the reserved register.

    Your patch implements neither of these choices; instead, it will just miscompile.
Jun 26 2018, 5:18 PM
srhines added a comment to D45454: Add llvm_gcov_flush to be called outside a shared library.

GCC's behavior is not documented and it also has changed over the years.

Unless there is a bug, changing LLVM's gcov_flush visibility can potentially break clients that depend on this behavior.

Jun 26 2018, 2:43 PM
srhines requested changes to D48580: [AArch64] Support reserving x1-7 registers..
Jun 26 2018, 2:36 PM
srhines updated subscribers of D48580: [AArch64] Support reserving x1-7 registers..
Jun 26 2018, 2:32 PM

Jun 25 2018

srhines accepted D48570: [Driver] Do not add -lpthread & -lrt with -static-libsan on Android.

Agree. This is the first time anyone is linking against static sanitizers on Android, so this is just something that we missed updating in the past.

Jun 25 2018, 4:02 PM
srhines added a comment to D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2..

Rui, I added you to this review (and the other corresponding one), as you reviewed ecbeckmann's original work here. Can you take a quick look at these? Thanks.

Jun 25 2018, 1:45 PM
srhines added a reviewer for D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2.: ruiu.
Jun 25 2018, 1:44 PM
srhines added a reviewer for D48458: Respect CMAKE_SYSROOT and don't search out libxml2 if it is set.: ruiu.
Jun 25 2018, 1:43 PM