Page MenuHomePhabricator

echristo (Eric Christopher)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 15 2012, 2:12 PM (326 w, 5 d)

Recent Activity

Mon, Jan 14

echristo accepted D56556: [opaque pointer types] Update CallInst creation APIs to consistently accept a callee-type argument..

If whitequark is happy I am :)

Mon, Jan 14, 10:56 AM

Wed, Jan 2

echristo added a comment to D54674: [llvm-objcopy] First bits for MachO .

@lhames , can we proceed here ?

Wed, Jan 2, 11:02 AM

Tue, Jan 1

echristo accepted D56181: [CMake][Fuchsia] Include check-lld in the list of bootstrap targets.

This seems like the sort of thing that you can just commit in the future.

Tue, Jan 1, 9:13 PM

Mon, Dec 31

echristo accepted D28791: [compiler-rt][crt] Simple crtbegin and crtend implementation.

That's fine, it's just not the state of the world and we should support compiling appropriately.

I don't want to pick a fight, but there can be used exactly same argument to defend pulling libgcc here.

Mon, Dec 31, 7:08 PM
echristo accepted D56043: [Driver] Don't pass default value to getCompilerRTArgString.

LGTM, thanks.

Mon, Dec 31, 5:33 PM
echristo accepted D56044: [Driver] Support object files in addition to static and shared libraries in compiler-rt.

OK, that makes sense. I'm not a huge fan of how this set of code looks, but it also seems unfair to require you to extensively refactor it for this.

Mon, Dec 31, 5:27 PM
echristo added a comment to D28791: [compiler-rt][crt] Simple crtbegin and crtend implementation.

Is there intention to support Clang/LLVM by GLIBC/GNU developers? Unlikely (knowing FSF track of reactions against LLVM projects). There are no gcc/libgcc licensing concerns for software hosted on GLIBC (LGPLv3) with libgcc. There already does exist a functional implementation for this OS. There is no need to treat it as an issue on the Clang/LLVM side (maybe except ambitions).

Mon, Dec 31, 5:24 PM
echristo added a comment to D56044: [Driver] Support object files in addition to static and shared libraries in compiler-rt.

Is something passing compiler-rt in as a -r link output or something?

Mon, Dec 31, 12:08 AM

Sun, Dec 30

echristo accepted D56123: [llvm-objdump] - Print symbol addressed when dumping disassembly output (-d).

Sure.

Sun, Dec 30, 10:59 PM

Fri, Dec 21

echristo accepted D55968: Add a doc for missing key function and an error message referencing the doc..

LGTM.

Fri, Dec 21, 11:25 AM
echristo added inline comments to D55968: Add a doc for missing key function and an error message referencing the doc..
Fri, Dec 21, 11:16 AM

Dec 18 2018

echristo accepted D55755: [DebugInfo] Move several private headers to include directory.

Thanks for the split! LGTM.

Dec 18 2018, 2:35 PM · debug-info
echristo accepted D55756: [DebugInfo] Make AsmPrinter struct HandlerInfo and Handlers protected.

Thanks for the split. LGTM!

Dec 18 2018, 2:35 PM · debug-info
echristo accepted D55752: [BPF] Add BTF DebugInfo under BPF target.

Thanks so much for the split!

Dec 18 2018, 2:35 PM · debug-info
echristo added a comment to D54911: [compiler-rt][builtins][PowerPC] Add ___fixunstfti builtin compiler-rt method support for PowerPC .

FWIW once nemanja and Masoud are happy then this can go in.

Dec 18 2018, 10:37 AM

Dec 13 2018

echristo added a comment to D28791: [compiler-rt][crt] Simple crtbegin and crtend implementation.

Looks like this stalled with no feedback outside of Chandler and I.

Dec 13 2018, 5:27 PM

Dec 11 2018

echristo accepted D51554: [CUDA][OPENMP][NVPTX]Improve logic of the debug info support..

LGTM. I'm quite a bit happier with this now. Thanks for going through the back and forth.

Dec 11 2018, 11:19 PM
echristo accepted D55580: [NVPTX] do not rely on cached subtarget info..

LGTM. Thanks!

Dec 11 2018, 3:37 PM

Dec 5 2018

echristo added inline comments to D51554: [CUDA][OPENMP][NVPTX]Improve logic of the debug info support..
Dec 5 2018, 10:39 PM
echristo accepted D46061: [DEBUGINFO, NVPTX] Disable emission of ',debug' option if only debug directives are allowed..
Dec 5 2018, 10:36 PM
echristo accepted D54320: [DEBUGINFO, NVPTX]Emit last debugging directives..

LGTM.

Dec 5 2018, 10:30 PM
echristo added a comment to D46189: [DEBUGINFO, NVPTX] Enable support for the debug info on NVPTX target..

LGTM still when everything else is approved.

Dec 5 2018, 10:30 PM

Dec 4 2018

echristo added a comment to D54747: Discard debuginfo for object files empty after GC.

But 2GB is perhaps still too big and I guess a large part of it can be for dead sections. If we fix this, I'd like to fix it in a proper way so that we can completely eliminate debug info for dead sections.

Dec 4 2018, 4:18 PM · lld

Dec 3 2018

echristo accepted D55016: Correctly support -shared-libgcc..

LGTM.

Dec 3 2018, 2:08 PM

Nov 28 2018

echristo added a comment to D54747: Discard debuginfo for object files empty after GC.

Thank you for the patch.

What you are doing in this patch is not too complicated and makes sense to me. That said, if actual size saving is not significant as you said in https://github.com/rust-lang/rust/issues/56068#issuecomment-440160568, it may not be worth doing. It seems to me that if debug info is already 2.4GB, shrinking it to 2GB doesn't make much difference. Do you have more convincing examples?

Nov 28 2018, 10:38 PM · lld

Nov 26 2018

echristo accepted D54492: Notify the linker when a TU compiled with split-stack has a functionwithout a prologue.There is more context here:https://go-review.googlesource.com/c/go/+/148819/.

LGTM.

Nov 26 2018, 3:20 PM

Nov 13 2018

echristo accepted D54378: Add Hurd triplet to LLVMSupport.

I can sign off on this.

Nov 13 2018, 4:50 PM
echristo accepted D53554: [Argument Promotion] Only promote args when function attributes are compatible.

This looks better to me. I've got an inline request for some elaboration and you might want to let chandlerc comment, but otherwise OK with me.

Nov 13 2018, 4:24 PM
echristo added inline comments to D54497: [ELF] --gdb-index: extract entities from .debug_info when .debug_gnu_pubnames is absent.
Nov 13 2018, 4:22 PM
echristo added a comment to D54492: Notify the linker when a TU compiled with split-stack has a functionwithout a prologue.There is more context here:https://go-review.googlesource.com/c/go/+/148819/.

Test?

Nov 13 2018, 2:38 PM
echristo added a comment to D53379: GSYM symbolication format.

Unfortunately there's nothing else I can do on this since I don't feel enough ownership over the top level lib/DebugInfo folder to approve a new sub-folder. If you still can't get any traction from someone who does have that kind of ownership, one idea might be to make a post on llvm-dev@ pointing them to this review.

Nov 13 2018, 11:45 AM

Nov 12 2018

echristo added inline comments to D53554: [Argument Promotion] Only promote args when function attributes are compatible.
Nov 12 2018, 2:30 PM

Nov 9 2018

echristo added a comment to D54369: [llvm-size][libobject] Add explicit "inTextSegment" methods similar to "isText" section methods to calculate size correctly..

I think this is the right direction, couple comment:

Nov 9 2018, 6:11 PM
echristo added a comment to D54124: [llvm-readelf] Make llvm-readelf more compatible with GNU readelf..

Okay, from my point of view, LGTM (with a nit). However, we should probably get some more buy-in on the RFC before breaking people's builds etc, so you might want to hold off on the commit for now.

Any thoughts on how to drum up some more interest?

I'll ping the thread you started, and I'd like to commit on Monday if there are no objections. I'm hoping this change is not so controversial.

Nov 9 2018, 6:02 PM

Nov 8 2018

echristo added a comment to D45784: [DEBUG_INFO, NVPTX] Fix relocation info..

Rather than testing for isNVPTX in AsmPrinter.cpp I'd rather just make a function "emitPreFunctionDebugInfo" and have it do nothing unless it's NVPTX and then define this in the nvptx backend. Easier to update if nvidia ever fixes this weirdness too.

Seems to me you were looking to the very first patch. The last one looks exactly like you want. Switch to the latest revision.

Nov 8 2018, 9:00 PM
echristo accepted D46189: [DEBUGINFO, NVPTX] Enable support for the debug info on NVPTX target..

Once we get everything else handled this is fine :)

Nov 8 2018, 8:44 PM
echristo added a comment to D53990: [MC] Support labels as offsets in .reloc directive.

It'd be really great if we could handle all of this in the mips backend rather than in general code.

Nov 8 2018, 7:15 PM
echristo added inline comments to D53554: [Argument Promotion] Only promote args when function attributes are compatible.
Nov 8 2018, 7:12 PM
echristo added a comment to D51554: [CUDA][OPENMP][NVPTX]Improve logic of the debug info support..

The llvm backend patch here has discussion around debug info kinds that we should iron out first.

Nov 8 2018, 6:46 PM
echristo added a comment to D46061: [DEBUGINFO, NVPTX] Disable emission of ',debug' option if only debug directives are allowed..

Some inline comments - I'm still not happy with some of how this patch is and would like to see some changes and elaborations of how we split things out. Mostly it's bikeshed naming things, but the current state is a bit more confusing than without.

Nov 8 2018, 6:41 PM
echristo added a comment to D53051: [llvm-tapi] initial commit, supports ELF text stubs.

Some drive by comments again. Be good to get @ributzka to take a final look.

Nov 8 2018, 6:33 PM
echristo added a comment to D54243: DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use..

I have no other bikeshed colors here. :)

Nov 8 2018, 4:46 PM
echristo accepted D54242: DebugInfo: Add a CU metadata attribute for use of DWARF ranges base address specifiers.

Naming sounds ok to me :)

Nov 8 2018, 4:45 PM

Nov 6 2018

echristo committed rLLDB346294: Add a break to avoid an unannotated fall-through..
Add a break to avoid an unannotated fall-through.
Nov 6 2018, 9:21 PM
echristo committed rL346294: Add a break to avoid an unannotated fall-through..
Add a break to avoid an unannotated fall-through.
Nov 6 2018, 9:20 PM

Nov 5 2018

echristo added a comment to D52296: [Clang] - Add '-gsplit-dwarf[=split,=single]' version for '-gsplit-dwarf' option..

Nice :)
So seems the last unresolved question left is the naming of the new option?
If we do not want to go with -gsingle-file-split-dwarf then what it should be?

What about -fdebug-fission as an alias for -gsplit-dwarf.
And -fsingle-file-debug-fission for the new option?

Or, may be:

-fdebug-fission for the new option and then
-fdebug-fission -fdebug-split-dwo would work together as -gsplit-dwarf?

Only DWARF supports this, correct?

I am not aware of any kind of debug information splitting except DWARF splitting.

So I would suggest: -fdwarf-fission[={split,single}] where the parameter defaults to split. Is there any particular value in having two separate options?

Probably not. -fdwarf-fission[={split,single}] sounds good for me.

Nov 5 2018, 9:45 AM
echristo added a comment to D53919: [X86] Don't allow illegal vector types to return by direct value on x86-64..

With both 3.3 and trunk (I don't have a 7.0 handy; I can build it if it would be helpful):

Please try clang 2.6 on both testcases.

Nov 5 2018, 9:41 AM
echristo added a comment to D53379: GSYM symbolication format.

README.md has complete file details now for anyone wanting to understand the entire file format.

Nov 5 2018, 9:34 AM

Oct 30 2018

echristo added a comment to D53051: [llvm-tapi] initial commit, supports ELF text stubs.

Random drive-by bikeshed:

Oct 30 2018, 6:18 PM
echristo added a comment to D28791: [compiler-rt][crt] Simple crtbegin and crtend implementation.

Hi Joerg,

Oct 30 2018, 6:15 PM
echristo added a comment to D52296: [Clang] - Add '-gsplit-dwarf[=split,=single]' version for '-gsplit-dwarf' option..

Do we generate the .dwo file directly these days? If not, I can imagine wanting to avoid the overhead of the objcopy hack; as long as the linker is smart enough not to bother with the .debug_*.dwo sections this seems like a build-time win.

We do generate them generically with no objcopy hack.

Eric, could you elaborate for me your position, please

Oct 30 2018, 5:30 PM

Oct 29 2018

echristo updated subscribers of D53652: [PPC64] Handle powerpc64 in OUTPUT_FORMAT.

Go ahead and commit for them.

Oct 29 2018, 10:46 PM
echristo added inline comments to D53545: [DWARF][NFC] NFC patch for reverted r342218 (refactoring rangelist handling).
Oct 29 2018, 3:46 PM
echristo added inline comments to D53545: [DWARF][NFC] NFC patch for reverted r342218 (refactoring rangelist handling).
Oct 29 2018, 3:38 PM

Oct 24 2018

echristo added a comment to D47811: [PATCH] [DWARF] Kotlin language.

I've put a note out to the DWARF committee to see if we can streamline the assignment of language codes. As long as there is a source of truth for these, we shouldn't have to wait for DWARF 6 to be published.
The idea has come up before, and now with a motivating case I think it's likely to happen. So if you could hold off a little longer (probably a few days) I can have an answer for you on that point.

Ok I'll wait as long as required, for now I've updated diff regarding my understanding of our yesterday conversation. Thank you for cooperation.

Out of all the current requests to the DWARF committee for a new language code, Kotlin is the only one to request a particular number, so there's a very high probability that you will actually get the requested number.
That said, the DWARF committee does not currently have a process for guaranteeing a language code, and it may be a little while (more than days, less than years) before that's in place.
Therefore, the current state of the patch LGTM, and sorry the DWARF committee can't be more responsive at this time.

Oct 24 2018, 11:49 PM
echristo committed rL345240: Revert "Fix use of __libcpp_deallocate in dynarray".
Revert "Fix use of __libcpp_deallocate in dynarray"
Oct 24 2018, 11:46 PM
echristo committed rCXX345240: Revert "Fix use of __libcpp_deallocate in dynarray".
Revert "Fix use of __libcpp_deallocate in dynarray"
Oct 24 2018, 11:46 PM
echristo committed rL345239: Temporarily Revert "Implement sized deallocation for std::allocator and friends..
Temporarily Revert "Implement sized deallocation for std::allocator and friends.
Oct 24 2018, 11:22 PM
echristo committed rCXX345239: Temporarily Revert "Implement sized deallocation for std::allocator and friends..
Temporarily Revert "Implement sized deallocation for std::allocator and friends.
Oct 24 2018, 11:22 PM
echristo accepted D53586: Implement Function Multiversioning for Non-ELF Systems..

All of the target specific stuff looks fine to me. I'm going to defer to rnk about the windows side of things and aaron for the attributes.

Oct 24 2018, 2:54 PM

Oct 23 2018

echristo accepted D52441: [CodeGen] Update min-legal-vector width based on function argument and return types.
In D52441#1271545, @rnk wrote:

Address Reid's comments. Add a comment with a list of all things that currently effect the vector width attribute emitted in IR.

For inlining, we update the caller's attribute during merging to ensure it is at least as large as the callee that is being inlined. This is required for always_inline of the intrinsics. We probably want a way to limit inlining, but that would effect the inlining decision. If the decision has been made to inline we have to take the max.

For LTO I don't have an answer. What do we do for things like target features and cpu today?

I think your comments about the behavior w.r.t. inlining are enough to describe what happens during LTO. I don't want to speak for Eric, but I think you've answered his questions.

Oct 23 2018, 5:20 PM
echristo committed rLLDB345086: Remove unused variable..
Remove unused variable.
Oct 23 2018, 2:58 PM
echristo committed rL345086: Remove unused variable..
Remove unused variable.
Oct 23 2018, 2:57 PM
echristo added a comment to D53364: [llvm-dwarfdump] - Add the support of parsing .debug_loclists..

I just wanted to let you know that I am currently working on a patch to refactor some of the rangelists code (see: https://reviews.llvm.org/D51081) with the objective to

  1. fold the pre-v5 functionality (.debug_ranges) into DWARF5 so we can get rid of DebugRangelists.{cpp,.h}
  2. templatize more of the DWARFListTable usage so it could be also used for location lists

    Unfortunately the first submission attempt caused some problems with TSan tests that I don't have access to, so there is still some work to be done. Once I'm past this I would like to suggest basing the location list table handling on the same framework, if you are OK with that.
Oct 23 2018, 12:34 AM

Oct 19 2018

echristo accepted D53364: [llvm-dwarfdump] - Add the support of parsing .debug_loclists..

LGTM. This code needs a lot of cleanup, but that's for another day.

Oct 19 2018, 5:29 PM
echristo accepted D53462: [X86] Add additional CPUs and features to Host.cpp and X86TargetParser.def to match compiler-rt and enable __builtin_cpu_supports/__builtin_cpu_is support in clang.

LGTM.

Oct 19 2018, 5:18 PM
echristo accepted D53461: [X86][compiler-rt] Add additional CPUs and features to the cpu detection to match libgcc.

LGTM.

Oct 19 2018, 5:17 PM
echristo accepted D53460: [X86] When checking the bits in cpu_features for function multiversioning dispatcher in the resolver, make sure all the required bits are set. Not just one of them.
Oct 19 2018, 5:16 PM
echristo accepted D53458: [X86] Add support for more than 32 features for __builtin_cpu_is.

Feel free to make any follow on changes you need here.

Oct 19 2018, 4:00 PM
echristo added a reviewer for D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used: aprantl.

I think Adrian has looked at this more recently than I have. Adding him here.

Oct 19 2018, 3:24 PM
echristo accepted D46878: Add DW_AT_linkage_name for DW_TAG_labels.

LGTM. Thanks!

Oct 19 2018, 3:22 PM
echristo accepted D52027: [llvm-readobj] Print ELF header flags names in GNU output.

The specific use of ENUM_ENT is also somewhat confusing, but you didn't add that so OK.

Oct 19 2018, 3:17 PM

Oct 18 2018

echristo accepted D45822: [DEBUGINFO, NVPTX] Try to pack bytes data into a single string..

LGTM.

Oct 18 2018, 6:12 PM

Oct 13 2018

echristo accepted D53248: [Driver] Support direct split DWARF emission for Fuchsia.

I'm ok with the "here are a bunch of fuchsia tests" file :)

Oct 13 2018, 5:23 PM
echristo added a comment to D52920: Introduce code_model macros.

LGTM as well. Thanks!

Oct 13 2018, 1:44 PM · Restricted Project

Oct 9 2018

echristo added a comment to D52703: Allow ifunc resolvers to accept arguments.

SGTM.

Oct 9 2018, 12:07 PM

Oct 1 2018

echristo added a comment to D52759: [AMDGPU] Rename pass "isel" to amdgpu-isel.

LGTM.

Oct 1 2018, 6:17 PM
echristo added a comment to D52441: [CodeGen] Update min-legal-vector width based on function argument and return types.

Couple of comments/questions:

Oct 1 2018, 1:43 PM
echristo committed rL343522: Temporarily revert "[GVNHoist] Re-enable GVNHoist by default".
Temporarily revert "[GVNHoist] Re-enable GVNHoist by default"
Oct 1 2018, 11:59 AM

Sep 24 2018

echristo accepted D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible..

Sounds like everyone is in favor. Hitting the accept button for Jordan :)

Sep 24 2018, 11:28 AM
echristo added a comment to D52296: [Clang] - Add '-gsplit-dwarf[=split,=single]' version for '-gsplit-dwarf' option..

Do we generate the .dwo file directly these days? If not, I can imagine wanting to avoid the overhead of the objcopy hack; as long as the linker is smart enough not to bother with the .debug_*.dwo sections this seems like a build-time win.

Sep 24 2018, 10:24 AM
echristo added a comment to D52296: [Clang] - Add '-gsplit-dwarf[=split,=single]' version for '-gsplit-dwarf' option..

Do we generate the .dwo file directly these days? If not, I can imagine wanting to avoid the overhead of the objcopy hack; as long as the linker is smart enough not to bother with the .debug_*.dwo sections this seems like a build-time win.

Sep 24 2018, 10:24 AM

Sep 21 2018

echristo accepted D52383: Fix codemodels.c test case (only test mcmodel-kernel on x86).
Sep 21 2018, 4:20 PM

Sep 20 2018

echristo committed rC342668: Add testcases for r342667..
Add testcases for r342667.
Sep 20 2018, 10:24 AM
echristo committed rL342668: Add testcases for r342667..
Add testcases for r342667.
Sep 20 2018, 10:24 AM
echristo committed rL342667: r342177 introduced a hint in cases where an #included file is not found. It….
r342177 introduced a hint in cases where an #included file is not found. It…
Sep 20 2018, 10:23 AM
echristo committed rC342667: r342177 introduced a hint in cases where an #included file is not found. It….
r342177 introduced a hint in cases where an #included file is not found. It…
Sep 20 2018, 10:23 AM
echristo closed D52280: Don't trim non-alphanumeric characters in 'file not found' errors for include directives..
Sep 20 2018, 10:23 AM
echristo closed D52280: Don't trim non-alphanumeric characters in 'file not found' errors for include directives..
Sep 20 2018, 10:23 AM

Sep 19 2018

echristo committed rL342616: Temporarily Revert "[New PM] Introducing PassInstrumentation framework".
Temporarily Revert "[New PM] Introducing PassInstrumentation framework"
Sep 19 2018, 10:18 PM
echristo updated subscribers of D52287: [unittests] Do not use llvm::sort in googlemock.

LGTM.

Sep 19 2018, 8:06 PM

Sep 18 2018

echristo added a reviewer for D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible.: scanon.

Adding Steve here in case he wants to opine.

Sep 18 2018, 4:58 PM

Sep 16 2018

echristo accepted D52159: [Lexer] Add xray_instrument feature.
Sep 16 2018, 6:42 PM

Sep 14 2018

echristo accepted D52129: [llvm-readobj] Make some commonly used short options visibile in -help.
Sep 14 2018, 11:49 PM

Sep 6 2018

echristo committed rL341593: The initial .text section generated in object files was missing the.
The initial .text section generated in object files was missing the
Sep 6 2018, 3:11 PM
echristo closed D48792: [ARM] Set execute-only flags in .text..
Sep 6 2018, 3:10 PM

Sep 5 2018

echristo added a comment to D48792: [ARM] Set execute-only flags in .text..

I'll get it tomorrow if no one does before.

Sep 5 2018, 7:38 PM
echristo added a comment to D48792: [ARM] Set execute-only flags in .text..

LGTM

Sep 5 2018, 6:41 PM
echristo added inline comments to D48792: [ARM] Set execute-only flags in .text..
Sep 5 2018, 2:45 PM
echristo added a comment to D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used.

Meta comment: please split the rename out. I don't mind the rename, but it makes the change much harder to review.

Sep 5 2018, 2:27 PM