echristo (Eric Christopher)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 15 2012, 2:12 PM (317 w, 1 d)

Recent Activity

Yesterday

echristo accepted D54378: Add Hurd triplet to LLVMSupport.

I can sign off on this.

Tue, Nov 13, 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.

Tue, Nov 13, 4:24 PM
echristo added inline comments to D54497: [ELF] --gdb-index: extract entities from .debug_info when .debug_gnu_pubnames is absent.
Tue, Nov 13, 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?

Tue, Nov 13, 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.

Tue, Nov 13, 11:45 AM

Mon, Nov 12

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

Fri, Nov 9

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:

Fri, Nov 9, 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.

Fri, Nov 9, 6:02 PM

Thu, Nov 8

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.

Thu, Nov 8, 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 :)

Thu, Nov 8, 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.

Thu, Nov 8, 7:15 PM
echristo added inline comments to D53554: [Argument Promotion] Only promote args when function attributes are compatible.
Thu, Nov 8, 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.

Thu, Nov 8, 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.

Thu, Nov 8, 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.

Thu, Nov 8, 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. :)

Thu, Nov 8, 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 :)

Thu, Nov 8, 4:45 PM

Tue, Nov 6

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

Mon, Nov 5

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.

Mon, Nov 5, 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.

Mon, Nov 5, 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.

Mon, Nov 5, 9:34 AM

Tue, Oct 30

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

Random drive-by bikeshed:

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

Hi Joerg,

Tue, Oct 30, 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

Tue, Oct 30, 5:30 PM

Mon, Oct 29

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

Go ahead and commit for them.

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

Wed, Oct 24

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.

Wed, Oct 24, 11:49 PM
echristo committed rL345240: Revert "Fix use of __libcpp_deallocate in dynarray".
Revert "Fix use of __libcpp_deallocate in dynarray"
Wed, Oct 24, 11:46 PM
echristo committed rCXX345240: Revert "Fix use of __libcpp_deallocate in dynarray".
Revert "Fix use of __libcpp_deallocate in dynarray"
Wed, Oct 24, 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.
Wed, Oct 24, 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.
Wed, Oct 24, 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.

Wed, Oct 24, 2:54 PM

Tue, Oct 23

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.

Tue, Oct 23, 5:20 PM
echristo committed rLLDB345086: Remove unused variable..
Remove unused variable.
Tue, Oct 23, 2:58 PM
echristo committed rL345086: Remove unused variable..
Remove unused variable.
Tue, Oct 23, 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.
Tue, Oct 23, 12:34 AM

Fri, Oct 19

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.

Fri, Oct 19, 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.

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

LGTM.

Fri, Oct 19, 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.
Fri, Oct 19, 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.

Fri, Oct 19, 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.

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

LGTM. Thanks!

Fri, Oct 19, 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.

Fri, Oct 19, 3:17 PM

Thu, Oct 18

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

LGTM.

Thu, Oct 18, 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

Sep 4 2018

echristo added inline comments to D48792: [ARM] Set execute-only flags in .text..
Sep 4 2018, 4:56 PM
echristo added a comment to D46061: [DEBUGINFO, NVPTX] Disable emission of ',debug' option if only debug directives are allowed..

Do you mean basically just line tables or something else? (Minus the need in normal dwarf for a minimal cu to associate the line table with). Also, how does this handle inlining etc?

Sep 4 2018, 4:28 PM
echristo added a reviewer for D51554: [CUDA][OPENMP][NVPTX]Improve logic of the debug info support.: dblaikie.

The change in name here from "line tables" to "directives only" feels a bit confusing. "Limited" seems to be a bit more clear, or even remaining line tables only. Can you explain where you were going with this particular set of changes in a bit more detail please?

Sep 4 2018, 4:27 PM
echristo added a comment to D45822: [DEBUGINFO, NVPTX] Try to pack bytes data into a single string..

Some inline comments.

Sep 4 2018, 4:25 PM
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.

Sep 4 2018, 4:19 PM
echristo added a comment to D47104: Keep underscores in DW_AT_name of DW_TAG_labels.

My *guess* is that the underscore trimming was added to allow setting breakpoints by name on labels in assembler output generated by a compiler, but I'm not sure. Perhaps @echristo knows more?

Sep 4 2018, 4:13 PM

Aug 30 2018

echristo added a comment to D51521: Refactor Addlibgcc to make the when and what logic more straightfoward..

LGTM, but let's get Stephen to ack as well.

Aug 30 2018, 4:04 PM

Aug 28 2018

echristo accepted D51007: Test the cross-product of options that affect how libgcc-related arguments are passed to the linker..

Feel free to add any tests like this that test existing behavior without review in the future. If we want to change the behavior we should probably review that though :)

Aug 28 2018, 6:15 PM
echristo accepted D51377: [NFC] Make getPreferredAlignment honor section markings..

Yeah, that works for me. If we start getting something wrong it's an edge case we should probably document :)

Aug 28 2018, 12:52 PM
echristo added a comment to D51358: [driver] Do not pass "-flavor old-gnu" option to LLD linker.

LGTM

Aug 28 2018, 12:42 PM

Aug 24 2018

echristo added a comment to D48964: Add support for writing HermitCore (https://hermitcore.org) ELF binaries..

Going to guess you need someone to commit this for you.

Aug 24 2018, 6:09 PM
echristo committed rL340675: This patch adds support to LLVM for writing HermitCore (https://hermitcore.org)….
This patch adds support to LLVM for writing HermitCore (https://hermitcore.org)…
Aug 24 2018, 6:09 PM
echristo added a comment to D51177: [DEBUGINFO] Add support for emission of the debug directives only..

Should we just have them mean the same thing and change it based on target?

Aug 24 2018, 3:37 PM

Aug 22 2018

echristo accepted D51150: [x86/retpoline] Split the LLVM concept of retpolines into separate subtarget features for indirect calls and indirect branches..

LGTM. Thanks.

Aug 22 2018, 9:04 PM
echristo added a comment to D48964: Add support for writing HermitCore (https://hermitcore.org) ELF binaries..

Oh, you probably want to fix the commit message here.

Aug 22 2018, 9:27 AM
echristo accepted D48964: Add support for writing HermitCore (https://hermitcore.org) ELF binaries..

SGTM.

Aug 22 2018, 9:26 AM

Aug 21 2018

echristo accepted D50989: Remove Darwin support from POWER backend..
Aug 21 2018, 4:33 PM
echristo committed rL340315: Temporarily Revert "[PowerPC] Generate Power9 extswsli extend sign and shift….
Temporarily Revert "[PowerPC] Generate Power9 extswsli extend sign and shift…
Aug 21 2018, 11:35 AM

Aug 7 2018

echristo accepted D50099: [DebugInfo][OpenCL] Address post-commit review of D49930.

LGTM. Thanks.

Aug 7 2018, 10:10 PM

Aug 6 2018

echristo added inline comments to D50289: [ELF] Don't copy STT_TLS in copy relocation.
Aug 6 2018, 3:23 PM
echristo accepted D50219: [ADT] Normalize empty triple components.
Aug 6 2018, 1:13 PM
echristo added a comment to D50219: [ADT] Normalize empty triple components.

LGTM. One inline comment.

Aug 6 2018, 1:07 PM
echristo added a comment to D50099: [DebugInfo][OpenCL] Address post-commit review of D49930.

When I went to mark these as static I noticed they use CGDebugInfo::CreateMemberType which uses a couple other non-static member functions, and it starts to become difficult to tease things out into nice clean static functions.

Aug 6 2018, 11:16 AM

Aug 5 2018

echristo added a comment to D50099: [DebugInfo][OpenCL] Address post-commit review of D49930.

Looks pretty good. Could you pass CGM in and just make the functions static I couldn't see any other class variables, but might have missed something. One inline comment as well.

Aug 5 2018, 7:44 AM
echristo committed rL338968: Revert "Add a warning if someone attempts to add extra section flags to….
Revert "Add a warning if someone attempts to add extra section flags to…
Aug 5 2018, 7:24 AM

Aug 1 2018

echristo added a comment to D49148: [DEBUGINFO] Disable unsupported debug info options for NVPTX target..
In D49148#1184957, @tra wrote:

I wonder, what's the right thing to do to silence the warnings. For instance, we compile everything with -Werror and the warnings result in build breaks.

Easy way out is to pass -Wno-unsupported-target-opt. It works, but it does not really solve anything. It also may mask potential other problems.

Another alternative is to change clang driver and filter out unsupported options so they are not passed to cc1. That will also work, but it looks wrong, because now we have two patches that effectively cancel each other for no observable benefit.

Third option is to grow a better way to specify target-specific sub-compilation options and then consider fancy debug flags to be attributable to host compilation only. Anything beyond the "safe" subset, would have to be specified explicitly. This also sounds awkward -- I don't really want to replicate bunch of options times number of GPUs I'm compiling for. That may be alleviated by providing more coarse way to group options. E.g. we could say "these are the options for *all* non-host compilations, and here are few specifically for sm_XY". I think @echristo and I had discussed something like this long time ago.

Aug 1 2018, 2:42 PM