rnk (Reid Kleckner)
User

Projects

User Details

User Since
Jan 2 2013, 4:34 PM (268 w, 13 m)

Recent Activity

Yesterday

rnk added a comment to D43501: [lit] Implement non-pipelined 'cat' command for internal shell.

With the pipelining restriction, this doesn't seem very useful. Typically this is used to send outputs to FileCheck, although we could rewrite most of those uses to stdin redirects (<).

Tue, Feb 20, 8:03 AM

Fri, Feb 16

rnk accepted D43397: Bring back r323297..

lgtm

Fri, Feb 16, 2:13 PM
rnk added inline comments to D38803: Suppress generation of a deleting destructor for a dllimport record..
Fri, Feb 16, 12:00 PM
rnk accepted D43326: [PDB] Fix emission of PDB string table.

Ship it! =D

Fri, Feb 16, 11:51 AM
rnk committed rC325375: [MS] Make constexpr static data members implicitly inline.
[MS] Make constexpr static data members implicitly inline
Fri, Feb 16, 11:49 AM
rnk committed rL325375: [MS] Make constexpr static data members implicitly inline.
[MS] Make constexpr static data members implicitly inline
Fri, Feb 16, 11:49 AM

Thu, Feb 15

rnk updated the diff for D43093: [FastISel] Sink local value materializations to first use.
  • rebase over prologue/epilogue loc change
Thu, Feb 15, 3:28 PM
rnk added a comment to D43352: Add support for __declspec(code_seg("segname")).

Can you add a test for special member functions? I wasn't able to find one, but maybe there is one already. I tried this and it looks like MSVC uses the class code_seg:

Thu, Feb 15, 3:19 PM
rnk updated the diff for D43320: Allow dllimport non-type template arguments in C++17.
  • revise as discussed
Thu, Feb 15, 2:25 PM
rnk added a comment to D43184: [WIP] [ItaniunCXXABI] Add an option for loading the type info vtable with dllimport.

A related observation on the topic of this: There's a subtle difference between what both GCC and MSVC does here, compared to clang. The case with a single variable works the same in all of them, but if you have a struct with many initialized elements, GCC and MSVC will initialize as many as possible of them statically, and only fill in the dllimport ones via a dynamic initializer. clang, on the other hand, will not initialize anything statically at all if it emits a dynamic initializer.

Thu, Feb 15, 2:21 PM
rnk added a comment to D43093: [FastISel] Sink local value materializations to first use.

I discussed alternative approaches with @echristo, and felt that this would be the best. Eric, do you want to take a look?

Thu, Feb 15, 2:10 PM
rnk added a reviewer for D43093: [FastISel] Sink local value materializations to first use: echristo.
Thu, Feb 15, 2:07 PM
rnk committed rL325296: Don't make PDBs by default in Release mode.
Don't make PDBs by default in Release mode
Thu, Feb 15, 1:28 PM
rnk closed D43156: Allow disabling PDB generation in Release build.
Thu, Feb 15, 1:28 PM
rnk added inline comments to D43313: [DebugInfo] Support parsing DWARF expressions.
Thu, Feb 15, 12:04 PM
rnk commandeered D43156: Allow disabling PDB generation in Release build.

I'll go ahead and make these changes to get us back to our behavior before https://reviews.llvm.org/D42632.

Thu, Feb 15, 11:15 AM
rnk requested changes to D43156: Allow disabling PDB generation in Release build.
Thu, Feb 15, 11:14 AM
rnk added inline comments to D43320: Allow dllimport non-type template arguments in C++17.
Thu, Feb 15, 11:00 AM
rnk added a comment to D42731: [IR] - Make User construction exception safe.

Seems reasonable, a few nits.

Thu, Feb 15, 9:51 AM

Wed, Feb 14

rnk created D43320: Allow dllimport non-type template arguments in C++17.
Wed, Feb 14, 2:55 PM

Tue, Feb 13

rnk committed rL325091: Merging r325085:.
Merging r325085:
Tue, Feb 13, 4:36 PM
rnk committed rL325090: Merging r325049:.
Merging r325049:
Tue, Feb 13, 4:36 PM
rnk committed rL325088: Merging r324449:.
Merging r324449:
Tue, Feb 13, 4:35 PM
rnk committed rL325089: Merging r324645:.
Merging r324645:
Tue, Feb 13, 4:35 PM
rnk committed rL325086: Merging r325085:.
Merging r325085:
Tue, Feb 13, 4:28 PM
rnk committed rL325085: [X86] Remove dead code from retpoline thunk generation.
[X86] Remove dead code from retpoline thunk generation
Tue, Feb 13, 4:26 PM
rnk committed rL325084: Merging r325049:.
Merging r325049:
Tue, Feb 13, 4:24 PM
rnk committed rL325083: Merging r324645:.
Merging r324645:
Tue, Feb 13, 4:21 PM
rnk committed rL325082: Merging r324449:.
Merging r324449:
Tue, Feb 13, 4:20 PM
rnk accepted D43259: Implement function attribute artificial.

Clang's builtin headers use __attribute__((__nodebug__)) for this purpose. Do you think we should follow this up by using artificial instead? It seems like it would be a representational improvement. @aprantl @probinson @dblaikie

Tue, Feb 13, 3:18 PM
rnk committed rL325049: [X86] Use EDI for retpoline when no scratch regs are left.
[X86] Use EDI for retpoline when no scratch regs are left
Tue, Feb 13, 12:50 PM
rnk closed D43214: [X86] Use EDI for retpoline when no scratch regs are left.
Tue, Feb 13, 12:50 PM
rnk added a comment to D43214: [X86] Use EDI for retpoline when no scratch regs are left.

So, if I understand correctly, this prevents the use of -mregparm=3, 32-bit x86, and a caller-save-all calling convention. And the reason this is reliable is because LLVM doesn't have such a calling convention (except maybe AnyReg) and if someone even managed to craft such IR they would just hit the assert and have to extend this? And we believe Clang definitively cannot create such a calling convention?

I'm not doubting anything here, I'm repeating to check my understanding. =]

Tue, Feb 13, 12:48 PM
rnk accepted D43221: Teach Wreturn-type, Wunreachable-code, and alpha.deadcode.UnreachableCode to treat __assume(0) like __builtin_unreachable..

It looks like we already generate llvm.assume() for these and that eventually behaves the same as the unreachable instruction. Neat. :)

Tue, Feb 13, 12:43 PM
rnk committed rLLD325047: [LLD] Implement /guard:[no]longjmp.
[LLD] Implement /guard:[no]longjmp
Tue, Feb 13, 12:35 PM
rnk committed rL325047: [LLD] Implement /guard:[no]longjmp.
[LLD] Implement /guard:[no]longjmp
Tue, Feb 13, 12:35 PM
rnk closed D43217: [LLD] Implement /guard:[no]longjmp.
Tue, Feb 13, 12:35 PM
rnk updated the diff for D43217: [LLD] Implement /guard:[no]longjmp.
  • fixup boolean logic error
Tue, Feb 13, 10:54 AM
rnk added a comment to D43217: [LLD] Implement /guard:[no]longjmp.

Thanks, done

Tue, Feb 13, 10:44 AM
rnk updated the diff for D43217: [LLD] Implement /guard:[no]longjmp.
  • switch enum style and comparisons
Tue, Feb 13, 10:44 AM
rnk accepted D43178: [FunctionAttrs][ArgumentPromotion][GlobalOpt] Disable some optimisations passes for naked functions.

We might want to consider some kind of helper on Function that checks for optnone and naked to block these kinds of prototype-changing optimizations.

Tue, Feb 13, 10:31 AM
rnk added a comment to D43156: Allow disabling PDB generation in Release build.

I don't think we should enable PDBs in release mode by default. One of the main uses cases for building in release mode (without PDBs) is to speed up the build. /Zi creates bottlenecks on machines with many cores, and adding /debug to the final link step really hurts incremental build speed.

Tue, Feb 13, 10:24 AM
rnk added a reviewer for D43156: Allow disabling PDB generation in Release build: zturner.
Tue, Feb 13, 10:17 AM

Mon, Feb 12

rnk created D43217: [LLD] Implement /guard:[no]longjmp.
Mon, Feb 12, 5:52 PM
rnk committed rL324978: Add REQUIRES: zlib to gdb-index.s test.
Add REQUIRES: zlib to gdb-index.s test
Mon, Feb 12, 5:22 PM
rnk committed rLLD324978: Add REQUIRES: zlib to gdb-index.s test.
Add REQUIRES: zlib to gdb-index.s test
Mon, Feb 12, 5:21 PM
rnk created D43214: [X86] Use EDI for retpoline when no scratch regs are left.
Mon, Feb 12, 4:08 PM
rnk accepted D43010: Report fatal error in the case of out of memory.

lgtm

Mon, Feb 12, 3:23 PM
rnk added a comment to D43184: [WIP] [ItaniunCXXABI] Add an option for loading the type info vtable with dllimport.

FYI, binutils auto-import actually creates a fake IAT entry rather than using a dynamic initializer. I think it's actually a pretty cute trick. http://blog.omega-prime.co.uk/2011/07/04/everything-you-never-wanted-to-know-about-dlls/#how-auto-import-works has details. That only works when you're referencing an external imported symbol directly though, and breaks when you need an offset from the imported symbol.

Mon, Feb 12, 9:56 AM
rnk committed rC324913: [Sema] Don't mark plain MS enums as fixed.
[Sema] Don't mark plain MS enums as fixed
Mon, Feb 12, 9:39 AM
rnk committed rL324913: [Sema] Don't mark plain MS enums as fixed.
[Sema] Don't mark plain MS enums as fixed
Mon, Feb 12, 9:39 AM
rnk closed D43110: [Sema] Don't mark plain MS enums as fixed.
Mon, Feb 12, 9:39 AM
rnk closed D43110: [Sema] Don't mark plain MS enums as fixed.
Mon, Feb 12, 9:39 AM
rnk added a comment to D43184: [WIP] [ItaniunCXXABI] Add an option for loading the type info vtable with dllimport.

Do I understand correctly that this workarounds a feature missing in lld? Does MinGW emit the same sorts of object files as clang in these scenarios?

Mon, Feb 12, 9:35 AM
rnk added a comment to D43110: [Sema] Don't mark plain MS enums as fixed.

Thanks! I'd noticed this weirdness but wasn't sure what we could do about it without breaking MS compat. I like this approach a lot.

Mon, Feb 12, 9:13 AM

Fri, Feb 9

rnk added a comment to D43110: [Sema] Don't mark plain MS enums as fixed.

Nice!

Test?

Fri, Feb 9, 12:40 PM
rnk updated the diff for D43110: [Sema] Don't mark plain MS enums as fixed.
  • Add test, update comments
Fri, Feb 9, 12:37 PM
rnk accepted D42762: Rewrite the VS Integration Scripts.

lgtm, thanks for revamping the VS integration!

Fri, Feb 9, 9:39 AM

Thu, Feb 8

rnk created D43110: [Sema] Don't mark plain MS enums as fixed.
Thu, Feb 8, 6:22 PM
rnk committed rC324689: [WinEH] Put funclet bundles on inline asm calls.
[WinEH] Put funclet bundles on inline asm calls
Thu, Feb 8, 4:19 PM
rnk committed rL324689: [WinEH] Put funclet bundles on inline asm calls.
[WinEH] Put funclet bundles on inline asm calls
Thu, Feb 8, 4:19 PM
rnk closed D43033: [WinEH] Put funclet bundles on inline asm calls.
Thu, Feb 8, 4:19 PM
rnk created D43093: [FastISel] Sink local value materializations to first use.
Thu, Feb 8, 3:17 PM
rnk accepted D41597: Driver: ignore -mbig-obj like /bigobj.

This works even though we pass -Wa,-mbig-obj?

Thu, Feb 8, 9:39 AM

Wed, Feb 7

rnk abandoned D43046: [Windows] Set the console output page to UTF-8.

Maybe we shouldn't do this, I wrote in the bug:

Unfortunately, it sounds like forcibly setting the user's console to the UTF8 codepage is hostile: https://github.com/stedolan/jq/issues/1121 Apparently it can cause window resizing, and I was able to confirm that the setting persists after exiting clang, which is really unclean. I want a solution where clang just prints UTF-8 and it works.

Wed, Feb 7, 2:40 PM
rnk created D43046: [Windows] Set the console output page to UTF-8.
Wed, Feb 7, 2:11 PM
rnk created D43033: [WinEH] Put funclet bundles on inline asm calls.
Wed, Feb 7, 11:19 AM
rnk added a comment to D42512: [X86] When using Win64 ABI, exit with error if SSE is disabled for varargs.

I wonder if it would be more general to handle this stuff earlier in the generated calling convention code, so that the use of XMM registers is conditionalized on the subtarget support. In other words, if SSE isn't available, we'd end up assigning FP values to stack locations, just as if all SSE registers had already been consumed. We could diagnose the error at that point. I think all this code is table-generated, which makes it hard to see and edit with these fixes.

Wed, Feb 7, 10:55 AM
rnk accepted D43016: Fix for #31362 - ms_abi is implemented incorrectly for larger values (>=16 bytes)..

These ABIInfo classes don't have any state, so just building one on the stack as needed is the way to go. Thanks for the fix!

Wed, Feb 7, 10:49 AM
rnk accepted D43017: Clean up use of C allocation functions.

lgtm

Wed, Feb 7, 10:43 AM
rnk accepted D42924: Don't pass ForDefinition_t in places it is redundant..

I'm guessing things were structured this way so that a function definition could have its visibility set before giving it a body, i.e. it would look like a declaration, but the visibility should be "for a definition". If everything works I suppose we don't do that anymore.

Wed, Feb 7, 10:35 AM

Tue, Feb 6

rnk added a comment to D43002: Emit S_OBJNAME symbol in CodeView.

I'd back out the C API changes, but otherwise this makes sense to me. Let's check with @aprantl, though.

Tue, Feb 6, 8:37 PM
rnk added a reviewer for D43002: Emit S_OBJNAME symbol in CodeView: aprantl.
Tue, Feb 6, 8:35 PM
rnk accepted D42998: [x86/retpoline] Make the external thunk names exactly match the names that happened to end up in GCC..

Looks good!

Tue, Feb 6, 6:47 PM
rnk accepted D42768: AST: support SwiftCC on MS ABI.

lgtm, ship it :)

Tue, Feb 6, 5:53 PM
rnk added a reviewer for D42968: Fix for PR32992. Static const classes not exported.: hans.
Tue, Feb 6, 3:42 PM
rnk added inline comments to D41950: Fix for Bug 8446. Template instantiation without a 'typename' keyword..
Tue, Feb 6, 12:12 PM
rnk added inline comments to D42762: Rewrite the VS Integration Scripts.
Tue, Feb 6, 11:43 AM

Mon, Feb 5

rnk committed rLLD324306: [COFF] Add minimal support for /guard:cf.
[COFF] Add minimal support for /guard:cf
Mon, Feb 5, 6:03 PM
rnk committed rL324306: [COFF] Add minimal support for /guard:cf.
[COFF] Add minimal support for /guard:cf
Mon, Feb 5, 6:03 PM
rnk closed D42592: [COFF] Add minimal support for /guard:cf.
Mon, Feb 5, 6:03 PM
rnk added a comment to D42592: [COFF] Add minimal support for /guard:cf.

Thanks!

Mon, Feb 5, 5:59 PM
rnk added a comment to D42926: [CodeView] Initial support for emitting S_BLOCK32 symbols for lexical scopes.

Thanks! So, I guess nesting S_DEFRANGE inside S_BLOCK32 works in VS? I thought we did experiments to show that it didn't, but hey, if it works, great.

Mon, Feb 5, 5:58 PM
rnk updated the diff for D42592: [COFF] Add minimal support for /guard:cf.
  • address feedback
Mon, Feb 5, 5:47 PM
rnk added inline comments to D42592: [COFF] Add minimal support for /guard:cf.
Mon, Feb 5, 5:37 PM
rnk committed rL324304: Update test expectations after reverting PLT change.
Update test expectations after reverting PLT change
Mon, Feb 5, 4:58 PM
rnk committed rL324301: Revert "Don't assume a null GV is local for ELF and MachO.".
Revert "Don't assume a null GV is local for ELF and MachO."
Mon, Feb 5, 4:49 PM
rnk updated the diff for D42592: [COFF] Add minimal support for /guard:cf.
  • address feedback
Mon, Feb 5, 4:41 PM
rnk added inline comments to D42592: [COFF] Add minimal support for /guard:cf.
Mon, Feb 5, 4:41 PM
rnk committed rL324297: Fix LLD wasm error check on Windows, which prints "lld.EXE: error: ...".
Fix LLD wasm error check on Windows, which prints "lld.EXE: error: ..."
Mon, Feb 5, 4:09 PM
rnk committed rLLD324297: Fix LLD wasm error check on Windows, which prints "lld.EXE: error: ...".
Fix LLD wasm error check on Windows, which prints "lld.EXE: error: ..."
Mon, Feb 5, 4:09 PM
rnk accepted D42925: Call FlushFileBuffers on readwrite file mappings..
In D42925#998354, @rnk wrote:

One concern I have is that this is a common pattern on Linux:

int fd = open(...);
void *mem = mmap(fd, ...);
close(fd);
// use mem

Are we reasonably confident that LLVM doesn't have this pattern anywhere in it? i.e. we never store the FD and then try to use it after closing it?

We don't do that anywhere *at the moment* that I can find, but I suppose it's a concern for future proofing this code. Someone could come along and add code like this after the fact. Currently the only place where we open a mapped_file_region for readwrite is in OnDiskBuffer, which stores a TempFile and then renames it atomically to the final path. We could put the flush logic inside of TempFile::keep() so that it flushes it before renaming. That might be a better option, although it wouldn't catch the case where someone comes along later and tries to use a mapped_file_region for something else. Perhaps it's ok for the time being though?

Mon, Feb 5, 3:41 PM
rnk added a comment to D42925: Call FlushFileBuffers on readwrite file mappings..

One concern I have is that this is a common pattern on Linux:

int fd = open(...);
void *mem = mmap(fd, ...);
close(fd);
// use mem
Mon, Feb 5, 3:21 PM
rnk added inline comments to D42762: Rewrite the VS Integration Scripts.
Mon, Feb 5, 3:06 PM
rnk added inline comments to D42762: Rewrite the VS Integration Scripts.
Mon, Feb 5, 2:27 PM

Thu, Feb 1

rnk added inline comments to D42830: Add and link to a testing philosophy document in the developer documentation..
Thu, Feb 1, 6:12 PM
rnk added a comment to D42768: AST: support SwiftCC on MS ABI.

No, I mean things like void foo(__attribute__((swiftcall)) void (*fnptr)());.

Thu, Feb 1, 4:52 PM
rnk committed rL324026: Merging r323288:.
Merging r323288:
Thu, Feb 1, 2:43 PM
rnk committed rL324025: Merging r323155 in LLD, with modifications to handle int3 fill.
Merging r323155 in LLD, with modifications to handle int3 fill
Thu, Feb 1, 2:40 PM
rnk accepted D42641: [MinGW] Emit typeinfo locally for dllimported classes without key functions.

This is actually consistent with what Microsoft does for dllimport classes. They don't have key functions, but they do import vftables when a class is dllimport and the constructor is inline. They never import RTTI and always emit it locally.

Thu, Feb 1, 2:06 PM