echristo (Eric Christopher)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 15 2012, 2:12 PM (309 w, 6 d)

Recent Activity

Fri, Sep 21

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

Thu, Sep 20

echristo committed rC342668: Add testcases for r342667..
Add testcases for r342667.
Thu, Sep 20, 10:24 AM
echristo committed rL342668: Add testcases for r342667..
Add testcases for r342667.
Thu, Sep 20, 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…
Thu, Sep 20, 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…
Thu, Sep 20, 10:23 AM
echristo closed D52280: Don't trim non-alphanumeric characters in 'file not found' errors for include directives..
Thu, Sep 20, 10:23 AM
echristo closed D52280: Don't trim non-alphanumeric characters in 'file not found' errors for include directives..
Thu, Sep 20, 10:23 AM

Wed, Sep 19

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

LGTM.

Wed, Sep 19, 8:06 PM

Tue, Sep 18

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.

Tue, Sep 18, 4:58 PM

Sun, Sep 16

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

Fri, Sep 14

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

Thu, Sep 6

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
Thu, Sep 6, 3:11 PM
echristo closed D48792: [ARM] Set execute-only flags in .text..
Thu, Sep 6, 3:10 PM

Wed, Sep 5

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

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

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

LGTM

Wed, Sep 5, 6:41 PM
echristo added inline comments to D48792: [ARM] Set execute-only flags in .text..
Wed, Sep 5, 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.

Wed, Sep 5, 2:27 PM

Tue, Sep 4

echristo added inline comments to D48792: [ARM] Set execute-only flags in .text..
Tue, Sep 4, 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?

Tue, Sep 4, 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?

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

Some inline comments.

Tue, Sep 4, 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.

Tue, Sep 4, 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?

Tue, Sep 4, 4:13 PM

Thu, Aug 30

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.

Thu, Aug 30, 4:04 PM

Tue, Aug 28

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 :)

Tue, Aug 28, 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 :)

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

LGTM

Tue, Aug 28, 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

Jul 31 2018

echristo committed rL338469: Simplify selectELFSectionForGlobal by pulling out the entry size.
Simplify selectELFSectionForGlobal by pulling out the entry size
Jul 31 2018, 6:30 PM
echristo committed rL338468: Tidy up logic around unique section name creation and remove a.
Tidy up logic around unique section name creation and remove a
Jul 31 2018, 6:04 PM
echristo committed rL338460: Android is an environment and we were comparing the android triple.
Android is an environment and we were comparing the android triple
Jul 31 2018, 4:53 PM
echristo committed rL338459: Tidy up comment..
Tidy up comment.
Jul 31 2018, 4:53 PM
echristo committed rL338458: Use UnknownVendor rather than UnknownArch since they're in two different enums.
Use UnknownVendor rather than UnknownArch since they're in two different enums
Jul 31 2018, 4:53 PM

Jul 30 2018

echristo committed rL338328: Revert "Add a definition for FieldSize that seems to make sense here.".
Revert "Add a definition for FieldSize that seems to make sense here."
Jul 30 2018, 4:22 PM
echristo committed rC338328: Revert "Add a definition for FieldSize that seems to make sense here.".
Revert "Add a definition for FieldSize that seems to make sense here."
Jul 30 2018, 4:22 PM
echristo committed rL338327: Add a definition for FieldSize that seems to make sense here..
Add a definition for FieldSize that seems to make sense here.
Jul 30 2018, 4:17 PM
echristo committed rC338327: Add a definition for FieldSize that seems to make sense here..
Add a definition for FieldSize that seems to make sense here.
Jul 30 2018, 4:17 PM
echristo updated subscribers of D45784: [DEBUG_INFO, NVPTX] Fix relocation info..

But you have other locations inside of functions so I don't get it.

Jul 30 2018, 3:13 PM
echristo added a comment to D49778: Fix "Q" and "R" inline assembly template modifiers for big-endian Arm.

Couple of comments.

Jul 30 2018, 3:08 PM
echristo added inline comments to D45822: [DEBUGINFO, NVPTX] Try to pack bytes data into a single string..
Jul 30 2018, 3:05 PM
echristo added a comment to D45784: [DEBUG_INFO, NVPTX] Fix relocation info..

Can you explain this a bit more with some examples of wrong relocations?

Jul 30 2018, 2:40 PM
echristo added a comment to D46021: [DEBUGINFO] Disable emission of the dwarf sections, but allow directives..

Sorry, I'm a bit confused.

  1. I'd expect /this/ behavior (line table directives, but no debug_info, etc) to be controlled via a DICompileUnit emission kind (a value between "NoDebug" and "LineTablesOnly" as Paul mentioned).
  2. I'd expect the "no ', debug'" flag (which I don't see in this change? Is it missing? is it elsewhere?) to be controlled via an MCOption - because it must be global.

    Now, arguably, if we have the global "no ', debug'" flag we maybe might as well use it to implement this behavior too since we don't have any other use for it? So maybe (2) invalidates (1).

I thought, that it is not very useful to have an option that, actually, controls only single target and I decided instead to add the option that just disables emission of the DWARF sections. In this case we don't need the special debug info mode, we can use this option.

Jul 30 2018, 2:32 PM
echristo added a comment to D49930: [DebugInfo][OpenCL] Generate correct block literal debug info for OpenCL.

Sorry, I didn't see the additional comments until after I committed. I will make those changes; is it OK to update this review, or should I create a new one?

Jul 30 2018, 1:58 PM
echristo added a comment to D49930: [DebugInfo][OpenCL] Generate correct block literal debug info for OpenCL.

The patch is fine, in general, couple of comments:

Jul 30 2018, 1:11 PM

Jul 25 2018

echristo accepted D49828: [libc++] Add hack to allow ubsan to work w/o compiler-rt (__muloti4 is undefined).

Bit of a hack, but I'm ok with it.

Jul 25 2018, 5:29 PM
echristo accepted D45963: [DEBUGINFO, NVPTX] Emit correct debug information for local variables..

LGTM.

Jul 25 2018, 2:48 PM
echristo accepted D45785: [DEBUGINFO, NVPTX] Set `DW_AT_frame_base` to `DW_OP_call_frame_cfa`..

LGTM.

Jul 25 2018, 2:48 PM
echristo added inline comments to D49148: [DEBUGINFO] Disable unsupported debug info options for NVPTX target..
Jul 25 2018, 1:40 PM

Jul 24 2018

echristo accepted D49663: [x86/SLH] Teach the x86 speculative load hardening pass to harden against v1.2 BCBS attacks directly..

Couple of small nits, but that's it. Still LGTM.

Jul 24 2018, 3:24 PM

Jul 23 2018

echristo accepted D49663: [x86/SLH] Teach the x86 speculative load hardening pass to harden against v1.2 BCBS attacks directly..

Couple of small nits, ctopper's suggestion seems reasonable too. Only "better" way I had of dealing with this would be not folding the load at all in the first place, but it seems harder to justify given the existing unfolding machinery.

Jul 23 2018, 5:42 PM

Jul 20 2018

echristo accepted D49583: [x86/SLH] Rename and comment the main hardening function. NFC..

LGTM.

Jul 20 2018, 4:29 PM

Jul 19 2018

echristo added a comment to D49532: [DebugInfoMetadata] Added endianity field in DIBasicType to hold DW_AT_endianity attribute for DW_TAG_basic_type..

Probably squirrel it away in the generic 'flags' bucket?

Jul 19 2018, 4:13 PM
echristo added reviewers for D49532: [DebugInfoMetadata] Added endianity field in DIBasicType to hold DW_AT_endianity attribute for DW_TAG_basic_type.: dblaikie, aprantl, dexonsmith.

I don't necessarily think this should be part of DIBasicType since it'll be used almost never. Adding other reviewers to get thoughts on location.

Jul 19 2018, 3:49 PM
echristo added a comment to D49148: [DEBUGINFO] Disable unsupported debug info options for NVPTX target..

Getting close, one inline comment.

Jul 19 2018, 3:45 PM
echristo accepted D49571: [x86/SLH] Clean up helper naming for return instruction handling and remove dead declaration of a call instruction handling helper..

LGTM. The comment is particularly helpful, thanks!

Jul 19 2018, 3:21 PM

Jul 18 2018

echristo added a comment to D46230: For x86_64, gcc 7.2 under Amazon Linux AMI sets its paths to x86_64-amazon-linux.

LGTM.

-eric

Hi Eric, I do not have commit access to trunk. Could you commit the change for me? Thanks.

Jul 18 2018, 12:27 PM · Restricted Project
echristo accepted D46230: For x86_64, gcc 7.2 under Amazon Linux AMI sets its paths to x86_64-amazon-linux.

LGTM.

Jul 18 2018, 11:30 AM · Restricted Project
echristo added a comment to D49148: [DEBUGINFO] Disable unsupported debug info options for NVPTX target..

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

  1. I can split it, no problems.
  2. Hmm, actually this what the warning already says. If the option is not supported it says 'debug option '-xxx' is not supported for target 'xxx-xxx-xxx''. It does not seem to me like a warning on non-standard option.

Let me try to elaborate a bit, I agree that I'm not very clear above.

I'm not a fan of the generic "non default debug options". It feels misleading. I think we probably want to separate it by "dwarf extensions", and "dwarf version".

As far as the error message itself: "debug option" sounds like an option to debug clang rather than a debug information option. Perhaps say "debug information option" rather than "debug option"?

Jul 18 2018, 11:09 AM
echristo added a comment to D46230: For x86_64, gcc 7.2 under Amazon Linux AMI sets its paths to x86_64-amazon-linux.

It's usually preferred to submit patches with full context - it'll make this a bit easier if you do please?

Jul 18 2018, 10:57 AM · Restricted Project
echristo added a comment to D49148: [DEBUGINFO] Disable unsupported debug info options for NVPTX target..

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

  1. I can split it, no problems.
  2. Hmm, actually this what the warning already says. If the option is not supported it says 'debug option '-xxx' is not supported for target 'xxx-xxx-xxx''. It does not seem to me like a warning on non-standard option.
Jul 18 2018, 10:55 AM
echristo added a comment to D49434: Put "built-in" function definitions in global Used list, for LTO. (fix bug 34169).
In D49434#1165381, @pcc wrote:

I don't think that globalopt is the right place for this because there is no guarantee that the pass is run. If we wanted to do something about this, it seems like a better place would be in lib/Analysis/ModuleSummaryAnalysis.cpp where we decide which functions are GC roots.

Also this is only a subset of the functions that the code generator can generate calls to. There is a list of runtime functions in llvm/include/llvm/CodeGen/RuntimeLibcalls.def, but as far as I know it isn't 100% comprehensive. Using that list would probably be better than just 3 functions though.

Jul 18 2018, 9:38 AM
echristo accepted D49043: [llvm-objdump] Add -demangle (-C) option.

Concerned about why... but let's just do it for now and figure out the test problem later. It's not a problem with the code as far as I can tell.

Jul 18 2018, 9:23 AM

Jul 17 2018

echristo accepted D49427: [x86/SLH] Major refactoring of SLH implementaiton. There are two big changes that are intertwined here:.

LGTM. Looks like a nice set of API simplifications.

Jul 17 2018, 3:47 PM
echristo added a comment to D49148: [DEBUGINFO] Disable unsupported debug info options for NVPTX target..

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Jul 17 2018, 3:14 PM
echristo accepted D48146: ELF: Implement --icf=safe using address-significance tables..

You'll want to wait on Rui for an actual LGTM, but this looks good to me and will match the llvm support so we can try this out in more detail and see where we should move the proposals.

Jul 17 2018, 3:02 PM
echristo added a comment to D48964: Add support for writing HermitCore (https://hermitcore.org) ELF binaries..

Checking out the archives for this list might come in handy:

Jul 17 2018, 2:58 PM
echristo accepted D48143: CodeGen: Add a target option for emitting .addrsig directives for all address-significant symbols..

LGTM. Not a fan of adding more stuff to TargetOptions, but for now it'll do.

Jul 17 2018, 2:56 PM
echristo accepted D47744: MC: Implement support for new .addrsig and .addrsig_sym directives..

LGTM. Thanks and sorry for the delay.

Jul 17 2018, 2:54 PM
echristo added a comment to D48964: Add support for writing HermitCore (https://hermitcore.org) ELF binaries..

Has ELFOSABI_HERMITCORE been officially allocated?

I'm not aware of any official entity managing these ELFOSABI numbers, and have neither found references to one at the Binutils and LLVM websites.

Jul 17 2018, 2:18 PM
echristo added a comment to D49043: [llvm-objdump] Add -demangle (-C) option.

LGTM, with the caveat that I don't know for certain if that was the original issue, without looking at the failing job and what was going on.

If you need to make minor changes to get the build bots working, no need for further review from my point of view.

I just asked @echristo if he could give this patch a try on non-x86 architecture, to be sure it was the problem ! :)

Jul 17 2018, 2:09 PM
echristo accepted D49433: [x86/SLH] Add the design document for Speculative Load Hardening, a Spectre v1 mitigation..

LGTM. Small organizational thoughts, but that's it.

Jul 17 2018, 1:59 PM
echristo added a comment to D48663: [Power9] Code Cleanup - Remove needsAggressiveScheduling().
Jul 17 2018, 1:28 PM
echristo committed rC337321: Remove unnecessary trailing ; in macro intrinsic definition..
Remove unnecessary trailing ; in macro intrinsic definition.
Jul 17 2018, 1:27 PM
echristo committed rL337321: Remove unnecessary trailing ; in macro intrinsic definition..
Remove unnecessary trailing ; in macro intrinsic definition.
Jul 17 2018, 1:27 PM

Jul 16 2018

echristo added a comment to D49378: [x86/SLH] Completely rework how we sink post-load hardening past data invariant instructions to be both more correct and much more powerful..

As a quick change this looks ok, some thoughts for cleanup:

Jul 16 2018, 4:12 PM

Jul 12 2018

echristo added inline comments to D44824: [Spectre] Introduce a new pass to do speculative load hardening to mitigate Spectre variant #1 for x86..
Jul 12 2018, 10:17 PM
echristo accepted D44824: [Spectre] Introduce a new pass to do speculative load hardening to mitigate Spectre variant #1 for x86..

This is obviously good to start and to iterate on.

Jul 12 2018, 10:09 PM
echristo accepted D49220: [x86] Teach the EFLAGS copy lowering to handle much more complex control flow patterns including forks, merges, and even cyles..

LGTM, a couple of inline requests. One small and one maybe less so.

Jul 12 2018, 9:19 PM

Jul 11 2018

echristo committed rL336885: Remove the unused m_signal member variable, but leave the code that gets it out….
Remove the unused m_signal member variable, but leave the code that gets it out…
Jul 11 2018, 8:58 PM
echristo committed rL336884: Remove unused variable m_header as it hasn't been used since it was.
Remove unused variable m_header as it hasn't been used since it was
Jul 11 2018, 8:57 PM
echristo committed rL336877: Temporarily revert "Recommit r328307: [IPSCCP] Use constant range information….
Temporarily revert "Recommit r328307: [IPSCCP] Use constant range information…
Jul 11 2018, 6:58 PM
echristo accepted D49211: [x86] Fix EFLAGS copy lowering to correctly handle walking past uses in multiple successors where some of the uses end up killing the EFLAGS register..

The change looks obvious, I think the test is fine too, but MIR test cases aren't fun to read. :)

Jul 11 2018, 5:51 PM
echristo committed rL336870: Add -allow-deprecated-dag-overlap to one of the experimental webassembly target….
Add -allow-deprecated-dag-overlap to one of the experimental webassembly target…
Jul 11 2018, 5:06 PM

Jul 10 2018

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

Has ELFOSABI_HERMITCORE been officially allocated?

Jul 10 2018, 3:31 PM