rnk (Reid Kleckner)
User

Projects

User Details

User Since
Jan 2 2013, 4:34 PM (255 w, 1 d)

Recent Activity

Tue, Nov 21

rnk accepted D39673: Toolchain: Normalize dwarf, sjlj and seh eh.

Yep, looks good

Tue, Nov 21, 1:42 PM
rnk accepted D40278: Object: Improve COFF irsymtab comdat representation..

lgtm

Tue, Nov 21, 12:53 PM
rnk added inline comments to D40278: Object: Improve COFF irsymtab comdat representation..
Tue, Nov 21, 11:33 AM
rnk added a comment to D39673: Toolchain: Normalize dwarf, sjlj and seh eh.

Just as a note there is still a lot to be desired here. I do not particularly like the UseSEHExceptions function default and the actual macro definition guards should be based on the current ExceptionModel because we set that in lib/CodeGen/BackendUtil.cpp. This way we do not need to have some silly default of x64 && windows for UseSEHExceptions and can rely on the llvm backend defaults but override within the driver we want like the apple targets do for sjlj. This does for now keep the current functionality while giving us a flag to override which is the goal of this patch.

Tue, Nov 21, 9:17 AM
rnk accepted D40276: Add -finstrument-function-entry-bare flag.

Yep, looks good. :)

Tue, Nov 21, 8:59 AM

Mon, Nov 20

rnk requested changes to D40278: Object: Improve COFF irsymtab comdat representation..

Didn't mean to push "accept"

Mon, Nov 20, 8:26 PM
rnk accepted D40278: Object: Improve COFF irsymtab comdat representation..
Mon, Nov 20, 8:22 PM
rnk committed rL318723: [MS] Increase default new alignment for win64 and test it.
[MS] Increase default new alignment for win64 and test it
Mon, Nov 20, 5:26 PM
rnk closed D40277: [MS] Increase default new alignment for win64 and test it by committing rL318723: [MS] Increase default new alignment for win64 and test it.
Mon, Nov 20, 5:26 PM
rnk accepted D40271: Split rename_handle out of rename on windows.

lgtm

Mon, Nov 20, 5:03 PM
rnk accepted D40264: COFF: Emit a COFF symbol table if /debug:dwarf is specified..

lgtm

Mon, Nov 20, 5:01 PM
rnk created D40277: [MS] Increase default new alignment for win64 and test it.
Mon, Nov 20, 4:49 PM
rnk added a comment to D40264: COFF: Emit a COFF symbol table if /debug:dwarf is specified..

Got it, just checking my understanding.

Mon, Nov 20, 2:07 PM
rnk committed rL318700: Relax pdb.test checks for debug chunk sizes.
Relax pdb.test checks for debug chunk sizes
Mon, Nov 20, 1:53 PM
rnk added a comment to D40264: COFF: Emit a COFF symbol table if /debug:dwarf is specified..

My hope was that we wouldn't need a flag. We'd just figure out if there were long section names that were marked to not be loaded at runtime (i.e. things that look like DWARF) and if so emit the long section table. Or, does DWARF need the full symbol table too?

Mon, Nov 20, 1:51 PM
rnk committed rL318699: Merge .xdata into .rdata by default.
Merge .xdata into .rdata by default
Mon, Nov 20, 1:50 PM
rnk closed D40197: Merge .xdata into .rdata by default by committing rL318699: Merge .xdata into .rdata by default.
Mon, Nov 20, 1:50 PM
rnk accepted D38445: [x86][inline-asm] allow recognition of MPX regs inside ms inline-asm blob.

Looks good, thanks for the heads up

Mon, Nov 20, 1:42 PM
rnk added a comment to D40249: Copy Function's calling convention by default when creating a new Call.

This wasn't what I had in mind. When a frontend (or any other IR generator) creates a call, it should have a source of type information telling it the function prototype and the calling convention (in clang, the AST). It should also apply any other wacky calling convention flags like -mregparm and other things. In the special case where the frontend is emitting a call to a function defined in the current TU, we should have a check that the conventions match. I was imagining that you'd make IRBuilder implement that assert, and then add an alternative CreateCallWithConvention entry point or something like that to allow the frontend to override the check.

Mon, Nov 20, 1:40 PM

Fri, Nov 17

rnk added a comment to D40197: Merge .xdata into .rdata by default.

What are the advantages to the section merging, apart from matching link's ouput? I personally prefer LLD keeping the separate sections, since it's easier to examine them that way.

Fri, Nov 17, 3:55 PM
rnk committed rL318572: [lit] Try to improve Ctrl-C behavior on Windows.
[lit] Try to improve Ctrl-C behavior on Windows
Fri, Nov 17, 3:52 PM
rnk added a comment to D40197: Merge .xdata into .rdata by default.

Do you think we should merge .idata, .edata, and .didata into .rdata as well? That will be more involved than this.

Fri, Nov 17, 3:19 PM
rnk created D40197: Merge .xdata into .rdata by default.
Fri, Nov 17, 2:26 PM
rnk committed rL318559: Fix coverage test on Windows bot.
Fix coverage test on Windows bot
Fri, Nov 17, 1:57 PM
rnk accepted D33675: [DAG] Do MergeConsecutiveStores again before Instruction Selection.

lgtm

Fri, Nov 17, 1:38 PM
rnk accepted D39567: [lit] Implement non-pipelined ‘mkdir’, ‘diff’ and ‘rm’ commands internally.

lgtm

Fri, Nov 17, 1:37 PM
rnk committed rL318556: Loosen -Wempty-body warning.
Loosen -Wempty-body warning
Fri, Nov 17, 1:34 PM
rnk closed D40185: Loosen -Wempty-body warning. by committing rL318556: Loosen -Wempty-body warning.
Fri, Nov 17, 1:33 PM
rnk updated the diff for D40185: Loosen -Wempty-body warning..
  • minor tweak
Fri, Nov 17, 12:57 PM
rnk commandeered D40185: Loosen -Wempty-body warning..
Fri, Nov 17, 12:56 PM
rnk accepted D40185: Loosen -Wempty-body warning..

I'll go ahead and land this as requested to unblock things. Thanks for the patch, sorry for the trouble!

Fri, Nov 17, 12:53 PM
rnk accepted D40191: [X86] Output cfi directives for saved XMM registers even if no GPRs are saved.

Ouch, thanks for the fix, looks good.

Fri, Nov 17, 12:48 PM
rnk committed rL318547: All .xdata sections are eligble for ICF.
All .xdata sections are eligble for ICF
Fri, Nov 17, 11:50 AM
rnk closed D40160: All .xdata sections are eligble for ICF by committing rL318547: All .xdata sections are eligble for ICF.
Fri, Nov 17, 11:50 AM
rnk updated the diff for D40160: All .xdata sections are eligble for ICF.
  • adjust isEligble conditions
Fri, Nov 17, 11:49 AM
rnk added inline comments to D40160: All .xdata sections are eligble for ICF.
Fri, Nov 17, 11:47 AM
rnk accepted D39990: Use TempFile in the implementation of LockFileManager.

lgtm, let's give it a go.

Fri, Nov 17, 11:40 AM
rnk accepted D40189: COFF: Stop emitting a non-standard COFF symbol table into PEs..

lgtm

Fri, Nov 17, 11:30 AM
rnk accepted D40188: Enable PDB generation with lld in asan and cfi tests on Windows..

lgtm

Fri, Nov 17, 11:28 AM
rnk requested changes to D40185: Loosen -Wempty-body warning..

Why does passing the rparen location for the if condition fix the warning for IF_ELSE? I assumed that would refer to the else clause semicolon.

Fri, Nov 17, 10:41 AM

Thu, Nov 16

rnk created D40160: All .xdata sections are eligble for ICF.
Thu, Nov 16, 4:59 PM
rnk committed rL318456: Issue -Wempty-body warnings for else blocks.
Issue -Wempty-body warnings for else blocks
Thu, Nov 16, 1:26 PM
rnk accepted D39737: [MC] Fix regression tests on Windows when git “core.autocrlf” is set to true.

lgtm

Thu, Nov 16, 11:44 AM
rnk committed rL318447: Fix my typo of PDB_TableType.
Fix my typo of PDB_TableType
Thu, Nov 16, 11:41 AM
rnk committed rL318444: Fix -Wreturn-type falling off the end of a function in new DIA code.
Fix -Wreturn-type falling off the end of a function in new DIA code
Thu, Nov 16, 11:33 AM
rnk accepted D39588: Distro: initial support for alpine.

lgtm

Thu, Nov 16, 11:14 AM
rnk committed rL318440: [MS] Apply adjustments after storing 'this'.
[MS] Apply adjustments after storing 'this'
Thu, Nov 16, 11:10 AM
rnk closed D40109: [MS] Apply adjustments after storing 'this' by committing rL318440: [MS] Apply adjustments after storing 'this'.
Thu, Nov 16, 11:10 AM
rnk added a comment to D40109: [MS] Apply adjustments after storing 'this'.
In D40109#926975, @rnk wrote:

This seems to cause a crash on startup in some gtest binaries when I self-host, so I guess I should debug that tomorrow before committing. The rest of clang's tests pass. I guess we don't use virtual inheritance. =S

Thu, Nov 16, 10:55 AM
rnk added inline comments to D40118: [Lint] Don't warn about noalias argument aliasing if other argument is byval.
Thu, Nov 16, 10:06 AM
rnk accepted D39918: [libunwind] Remove a FIXME about truncated section names.

Yep, no need for this.

Thu, Nov 16, 9:57 AM

Wed, Nov 15

rnk added a comment to D40109: [MS] Apply adjustments after storing 'this'.

This seems to cause a crash on startup in some gtest binaries when I self-host, so I guess I should debug that tomorrow before committing. The rest of clang's tests pass. I guess we don't use virtual inheritance. =S

Wed, Nov 15, 5:47 PM
rnk accepted D40102: [coff] correctly emit safeseh entries for handlers defined in dlls.

No yaml support for import libraries, I take it?

Wed, Nov 15, 4:40 PM
rnk committed rL318355: Try to fix WebAssembly build after r318352.
Try to fix WebAssembly build after r318352
Wed, Nov 15, 4:32 PM
rnk created D40109: [MS] Apply adjustments after storing 'this'.
Wed, Nov 15, 4:29 PM
rnk added a comment to D39994: Loosen MSVC 2017 path requirements.

I haven't dug into this code to really understand if this is right and won't change our version detection logic, but yes, broadly I believe we should just consult PATH, find a cl.exe, and check its version. I'd like to reduce this path validation to a minimum.

Wed, Nov 15, 2:26 PM
rnk added reviewers for D39994: Loosen MSVC 2017 path requirements: zturner, hans.
Wed, Nov 15, 2:22 PM
rnk accepted D40014: [LLD] [COFF] Improve the autoexport check for symbols from import libraries with -opt:noref.

lgtm, definitely don't want to auto-export that imported data. The absolute symbol thing can be a follow-up change.

Wed, Nov 15, 2:19 PM
rnk committed rL318320: [InstCombine] Salvage debug info during initial DCE.
[InstCombine] Salvage debug info during initial DCE
Wed, Nov 15, 10:54 AM
rnk added a member for debug-info: rnk.
Wed, Nov 15, 10:32 AM

Tue, Nov 14

rnk accepted D40050: CMake: Turn LLVM_ENABLE_LIBXML2 into a tri-state option.

lgtm

Tue, Nov 14, 2:25 PM
rnk added a comment to D40014: [LLD] [COFF] Improve the autoexport check for symbols from import libraries with -opt:noref.

Do you mind digging a little bit so we can get a test case for it? Is there some way to get symbol definitions from .drectives like ELF with --defsym or something?

Tue, Nov 14, 2:12 PM
rnk accepted D40017: [LLD] [MinGW] Add support for --dynamicbase, ignore --nxcompat, --tsaware and --high-entropy-va.

lgtm

Tue, Nov 14, 2:05 PM
rnk committed rL318203: Make salvageDebugInfo of casts work for dbg.declare and dbg.addr.
Make salvageDebugInfo of casts work for dbg.declare and dbg.addr
Tue, Nov 14, 1:50 PM
rnk closed D40042: Make salvageDebugInfo of casts work for dbg.declare and dbg.addr by committing rL318203: Make salvageDebugInfo of casts work for dbg.declare and dbg.addr.
Tue, Nov 14, 1:50 PM
rnk added a comment to D40017: [LLD] [MinGW] Add support for --dynamicbase, ignore --nxcompat, --tsaware and --high-entropy-va.

I think I'm in favor of ignoring options that reinforce LLD's defaults and don't have negatives. We can wait until a user files a bug and then add a negative option for them, or maybe add an escaping mechanism to let them pass the negative MSVC-style option.

Tue, Nov 14, 1:48 PM
rnk added inline comments to D40042: Make salvageDebugInfo of casts work for dbg.declare and dbg.addr.
Tue, Nov 14, 1:44 PM
rnk accepted D40018: [LLD] [MinGW] Implement support for the --image-base option.

lgtm

Tue, Nov 14, 1:41 PM
rnk accepted D40031: [LLD] [MinGW] Ignore the --build-id, --pie-executable and --disable-auto-image-base options.

lgtm

Tue, Nov 14, 1:40 PM
rnk added a comment to D40017: [LLD] [MinGW] Add support for --dynamicbase, ignore --nxcompat, --tsaware and --high-entropy-va.

Are all of these defaults important for mingw compatibility, or just -dynamicbase?

Tue, Nov 14, 1:40 PM
rnk accepted D40015: [LLD] [MinGW] Handle --large-address-aware.

lgtm

Tue, Nov 14, 12:58 PM
rnk accepted D40019: [LLD] [MinGW] Implement the --[no-]gc-sections and --icf options.

lgtm

Tue, Nov 14, 12:51 PM
rnk accepted D39673: Toolchain: Normalize dwarf, sjlj and seh eh.

Looks good, sorry for the delay!

Tue, Nov 14, 12:45 PM
rnk retitled D40042: Make salvageDebugInfo of casts work for dbg.declare and dbg.addr from [InstCombine] Replace metadata alloca uses without a cast to Make salvageDebugInfo of casts work for dbg.declare and dbg.addr.
Tue, Nov 14, 11:56 AM
rnk accepted D39331: Switch -mcount and -finstrument-functions to emit EnterExitInstrumenter attributes.

Thanks, looks good!

Tue, Nov 14, 11:54 AM
rnk added a comment to D40042: Make salvageDebugInfo of casts work for dbg.declare and dbg.addr.

I rewrote this with salvageDebugInfo, take another look.

Tue, Nov 14, 11:52 AM
rnk updated the diff for D40042: Make salvageDebugInfo of casts work for dbg.declare and dbg.addr.
  • rewrite with salvageDebugInfo
Tue, Nov 14, 11:52 AM
rnk added inline comments to D40042: Make salvageDebugInfo of casts work for dbg.declare and dbg.addr.
Tue, Nov 14, 11:28 AM
rnk added inline comments to D39331: Switch -mcount and -finstrument-functions to emit EnterExitInstrumenter attributes.
Tue, Nov 14, 11:17 AM
rnk created D40042: Make salvageDebugInfo of casts work for dbg.declare and dbg.addr.
Tue, Nov 14, 11:11 AM
rnk added a comment to D40025: [LLD] [COFF] Don't write long section names for sections that will be mapped at runtime.

I wonder if we should limit the long section name extension to sections that aren't mapped at runtime. This makes sense because if some tool cares about unmapped section contents, they're going to need to look at the file on disk, not the image in memory, and that will have a full symbol table.

Tue, Nov 14, 9:32 AM
rnk accepted D40009: [Lint] Don't warn about passing alloca'd value to tail call if using byval.

lgtm with suggestion

Tue, Nov 14, 9:26 AM
rnk added inline comments to D40019: [LLD] [MinGW] Implement the --[no-]gc-sections and --icf options.
Tue, Nov 14, 9:22 AM

Mon, Nov 13

rnk accepted D39373: Reorder Value.def to optimize code size.

I checked, and we wrap this enum in our C API, so there should be no ABI concerns here. See LLVMValueKind LLVMGetValueKind(LLVMValueRef Val) in IR/Core.cpp.

Mon, Nov 13, 12:25 PM
rnk added inline comments to D39331: Switch -mcount and -finstrument-functions to emit EnterExitInstrumenter attributes.
Mon, Nov 13, 10:54 AM
rnk accepted D39287: Use CountingFunctionInserter both mcount and cygprofile calls, before and after inlining.
In D39287#922534, @hans wrote:

Rebasing and addressing comments.

I've also added the pass in PassManagerBuilder::populateFunctionPassManager() because I realized that's how Clang sets up the passes.

One thing that worries me is that this set-up is pretty fragile against how the passes are run. I think it will currently work, but if the pass were to somehow run more than once, the instrumentation could get inserted multiple times. One way to avoid that would be to remove the attribute (or probably faster, give it an empty value?) when the pass "consumes" the attribute. Does that sound reasonable?

Mon, Nov 13, 10:51 AM
rnk committed rL318073: Fix clang -Wsometimes-uninitialized warning in SCEV code.
Fix clang -Wsometimes-uninitialized warning in SCEV code
Mon, Nov 13, 10:43 AM
rnk committed rL318072: Remove a std::map and std::set that show up in LLD profiles.
Remove a std::map and std::set that show up in LLD profiles
Mon, Nov 13, 10:39 AM
rnk closed D39609: Remove a std::map and std::set that show up in LLD profiles by committing rL318072: Remove a std::map and std::set that show up in LLD profiles.
Mon, Nov 13, 10:39 AM
rnk committed rL318071: Disable GC and ICF when /debug is present.
Disable GC and ICF when /debug is present
Mon, Nov 13, 10:38 AM
rnk closed D39885: Disable GC and ICF when /debug is present by committing rL318071: Disable GC and ICF when /debug is present.
Mon, Nov 13, 10:38 AM
rnk accepted D39941: Set hasSideEffects=0 for TargetOpcode::{CFI_INSTRUCTION,EH_LABEL,GC_LABEL,ANNOTATION_LABEL}.

Looks good. I think MachineInstr::isPosition is a more accurate way to identify these types of label instructions. If there is any code that relies on hasUnmodeledSideEffects, we can update it to check isPosition instead.

Mon, Nov 13, 10:26 AM

Sat, Nov 11

rnk accepted D39922: Create a TempFile class.

Looks good

Sat, Nov 11, 6:49 AM

Fri, Nov 10

rnk added inline comments to D39922: Create a TempFile class.
Fri, Nov 10, 2:46 PM
rnk updated the diff for D39885: Disable GC and ICF when /debug is present.

One last attempt at simplification

Fri, Nov 10, 2:36 PM
rnk updated the diff for D39885: Disable GC and ICF when /debug is present.

Rewrite one more time with fewer bools. The tricky case is /opt:noicf,ref.

Fri, Nov 10, 2:17 PM
rnk updated the diff for D39885: Disable GC and ICF when /debug is present.
  • make opt:ref imply ICF unless it's explicitly disabled
Fri, Nov 10, 1:23 PM
rnk added inline comments to D39885: Disable GC and ICF when /debug is present.
Fri, Nov 10, 1:20 PM
rnk accepted D39892: [llvm-cvtres] Add support for ARM64.

lgtm

Fri, Nov 10, 1:18 PM
rnk committed rL317910: Use DenseMap instead of std::map in fixupExports.
Use DenseMap instead of std::map in fixupExports
Fri, Nov 10, 11:13 AM