Page MenuHomePhabricator

rnk (Reid Kleckner)
User

Projects

User Details

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

Recent Activity

Fri, Jan 15

rnk accepted D94761: [DebugInfo][dexter] Add dexter tests for merged values.

lgtm

Fri, Jan 15, 4:35 PM · debug-info, Restricted Project
rnk added inline comments to D94824: [MSVC] Disable <fstream> usage of <filesystem>.
Fri, Jan 15, 3:05 PM · Restricted Project
rnk requested review of D94824: [MSVC] Disable <fstream> usage of <filesystem>.
Fri, Jan 15, 1:58 PM · Restricted Project
rnk committed rG98c89ccfbd74: [MSVC] Don't add -nostdinc++ -isystem to runtimes builds (authored by rnk).
[MSVC] Don't add -nostdinc++ -isystem to runtimes builds
Fri, Jan 15, 1:22 PM
rnk closed D94817: [MSVC] Don't add -nostdinc++ -isystem to runtimes builds.
Fri, Jan 15, 1:22 PM · Restricted Project
rnk added a comment to D90094: [BasicAA] Handle recursive queries more efficiently (NFCI).

I see. Maybe ASLR matters? I have the full stack trace if it helps:
https://reviews.llvm.org/P8253

Fri, Jan 15, 1:14 PM · Restricted Project
rnk updated the diff for D94817: [MSVC] Don't add -nostdinc++ -isystem to runtimes builds.
  • fix misleading comment
Fri, Jan 15, 1:05 PM · Restricted Project
rnk added inline comments to D94817: [MSVC] Don't add -nostdinc++ -isystem to runtimes builds.
Fri, Jan 15, 1:03 PM · Restricted Project
rnk added a comment to D90094: [BasicAA] Handle recursive queries more efficiently (NFCI).

CReduce on plain C code is lightning fast, no template goo grind away. I got this reduced test case that crashes when this patch is applied:

Fri, Jan 15, 12:56 PM · Restricted Project
rnk requested review of D94817: [MSVC] Don't add -nostdinc++ -isystem to runtimes builds.
Fri, Jan 15, 12:38 PM · Restricted Project
rnk added a comment to D90094: [BasicAA] Handle recursive queries more efficiently (NFCI).

I reverted this, it caused crashes while compiling harfbuzz in Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1167305

Fri, Jan 15, 12:30 PM · Restricted Project
rnk added a reverting change for rGa3904cc77f18: [BasicAA] Handle recursive queries more efficiently: rG64db296e5a8c: Revert "[BasicAA] Handle recursive queries more efficiently".
Fri, Jan 15, 12:30 PM
rnk committed rG64db296e5a8c: Revert "[BasicAA] Handle recursive queries more efficiently" (authored by rnk).
Revert "[BasicAA] Handle recursive queries more efficiently"
Fri, Jan 15, 12:30 PM
rnk added a reverting change for D90094: [BasicAA] Handle recursive queries more efficiently (NFCI): rG64db296e5a8c: Revert "[BasicAA] Handle recursive queries more efficiently".
Fri, Jan 15, 12:30 PM · Restricted Project
rnk committed rG4f24d0dd5386: Fix libc++ clang-cl build, swap attribute order (authored by rnk).
Fix libc++ clang-cl build, swap attribute order
Fri, Jan 15, 11:45 AM
rnk closed D94788: Fix libc++ clang-cl build, swap attribute order.
Fri, Jan 15, 11:45 AM · Restricted Project
rnk requested review of D94788: Fix libc++ clang-cl build, swap attribute order.
Fri, Jan 15, 9:00 AM · Restricted Project

Thu, Jan 14

rnk accepted D94639: [DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types.

(I'm generally good with this - llvm & clang changes should be committed separately & maybe the linkage part too)

I'll leave the rest of the review/final approval up to @rnk

Thu, Jan 14, 5:24 PM · Restricted Project, Restricted Project
rnk added a comment to rG1a92de0064bc: [libc++] NFCI: Remove _LIBCPP_EXTERN_TEMPLATE2.

Before that commit, under debug mode, we'd disable _LIBCPP_EXTERN_TEMPLATE, but not _LIBCPP_EXTERN_TEMPLATE2. After the commit, we disable both (which is your issue).

Thu, Jan 14, 5:22 PM
rnk added a comment to D94718: [libc++] Unbreak the debug mode.

Thanks, I think this will solve the issue.

Thu, Jan 14, 2:29 PM · Restricted Project
rnk accepted D94646: [clang][MSVC] Fix missing MSInheritanceAttr in template specialization..

lgtm

Thu, Jan 14, 10:04 AM · Restricted Project
rnk added a comment to D94646: [clang][MSVC] Fix missing MSInheritanceAttr in template specialization..

Thanks, this seems like the right spot.

Thu, Jan 14, 9:59 AM · Restricted Project

Wed, Jan 13

rnk added inline comments to D43002: [CodeView] Emit S_OBJNAME record.
Wed, Jan 13, 6:36 PM · Restricted Project, Restricted Project
rnk added a comment to D94364: [clang] Allow specifying the aapcs and aapcs-vfp for windows on arm.

I don't see how supporting this attribute would break interop with MSVC. MSVC doesn't support these attributes, only GCC and Clang do. Presumably in the wine environment, some headers using these _GNUC extensions are being included somewhere, and IMO we should accept the attributes and honor them.

Wed, Jan 13, 6:28 PM · Restricted Project
rnk added inline comments to D94639: [DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types.
Wed, Jan 13, 5:48 PM · Restricted Project, Restricted Project
rnk added a comment to D94639: [DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types.

How does any of this deal with overloading? I guess for either solution (qualified name or real scopes) you have to include all the parameter type info too to avoid the functions being treated as identical/duplicate by CV?

Yep, this patch doesn't deal with overloading or lambdas. For lambdas I have a separate patch, and for overloading I haven't really thought much about yet. Seems like we'd have to include all the parameter type info somehow (or include all the parameters in the display name).

Wed, Jan 13, 5:42 PM · Restricted Project, Restricted Project
rnk added a comment to rG1a92de0064bc: [libc++] NFCI: Remove _LIBCPP_EXTERN_TEMPLATE2.

This actually caused https://crbug.com/1166386. The explanation is that defining _LIBCPP_DEBUG causes _LIBCPP_EXTERN_TEMPLATE to be defined to nothing, which disables the extern declarations. Chrome uses _LIBCPP_DEBUG in debug builds. IIUC, locale needs these extern template declarations, because locale uses static data members with hidden visibility (need to confirm). If you remove the extern template declarations, then duplicate static data members may be emitted into the user's DSO, and when they use locale facets, they will be registered with a new id. I need to dig more to confirm, but this is what I'm going on.

Wed, Jan 13, 3:36 PM
rnk added inline comments to D92776: [libc++] ADL-proof <algorithm> by adding _VSTD:: qualification on calls..
Wed, Jan 13, 1:49 PM · Restricted Project
rnk added a comment to D94563: [asan] Add flag (-external_symbolizer_path_from_binary) to find llvm-symbolizer relative to the binary's directory..

Please add a lit test for this. The test could actually copy llvm-symbolizer into a temporary directory, and you could embed the option in __asan_default_options similar to what we want to do in Chromium. I think it's also reasonable to test that if llvm-symbolizer doesn't exist, we get a warning, not an error.

Wed, Jan 13, 11:55 AM · Restricted Project
rnk accepted D94555: [LLD][COFF] Avoid std::vector resizes during type merging.

lgtm

Wed, Jan 13, 9:30 AM · Restricted Project

Tue, Jan 12

rnk committed rG6529d7c5a45b: [PDB] Defer relocating .debug$S until commit time and parallelize it (authored by rnk).
[PDB] Defer relocating .debug$S until commit time and parallelize it
Tue, Jan 12, 5:47 PM
rnk closed D94267: [PDB] Defer relocating .debug$S until commit time and parallelize it.
Tue, Jan 12, 5:47 PM · Restricted Project
rnk added a comment to D94267: [PDB] Defer relocating .debug$S until commit time and parallelize it.

Thanks, I'll push this after a few more tests.

Tue, Jan 12, 2:57 PM · Restricted Project
rnk accepted D94537: [IR] move nomerge attribute from function declaration/definition to callsites.

lgtm, thanks!

Tue, Jan 12, 11:41 AM · Restricted Project
rnk added inline comments to D87805: [PDB] Merge types in parallel when using ghashing.
Tue, Jan 12, 9:31 AM · Restricted Project

Mon, Jan 11

rnk added a comment to D94267: [PDB] Defer relocating .debug$S until commit time and parallelize it.

This is mostly caused by contention when page faulting on either mmap'ed files or zero-pages. KeYieldProcessorEx is spinning while waiting for a lock for bringing pages into the 'working set'.

Mon, Jan 11, 5:21 PM · Restricted Project
rnk accepted D92751: [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate.

lgtm, thanks

Mon, Jan 11, 12:14 PM · Restricted Project, Restricted Project
rnk accepted D94313: [tools] Mark output of tools as text if it is really text.

lgtm

Mon, Jan 11, 12:08 PM · Restricted Project
rnk committed rG5464baaae8c1: Fix minor build issue (NFC) (authored by dstuttard).
Fix minor build issue (NFC)
Mon, Jan 11, 11:24 AM
rnk added a comment to D94415: Fix minor build issue (NFC).

We are also observing this build breakage, so I'm going to go ahead and push this now.

Mon, Jan 11, 11:24 AM · Restricted Project
rnk closed D94415: Fix minor build issue (NFC).
Mon, Jan 11, 11:24 AM · Restricted Project

Fri, Jan 8

rnk added a comment to D94154: Unique Internal Linkage Name suffixes must be demangler friendly.

@rnk so I think this ^ leads us back to the "maybe this should be done in clang where we can enable mangling for C static functions when needed/when this feature is enabled". (I don't mean that we should add this scheme into the mangling scheem proper - I'm still OK with it being a ".part.N" suffix - but that that suffix should only be added to a mangled name, not to an unmangled name like a plain C function name) - not sure how that works for Windows (does Windows mangling scheme have a notion of suffixes, etc - I guess this system as implemented assumes Windows is cool with ".foo.N" suffixes?).

We are going in circles. For C functions, there is no linkage name added and it will not be newly introduced either, Hoy's patch specifically updates linkage name only when it is not null. When "overloadable" is used, linkage name is added as a mangled name for C functions too and the suffix will also make it demangleable. I don't see any issues.

Fri, Jan 8, 3:41 PM · Restricted Project, Restricted Project
rnk added a comment to D94325: [DebugInfo][CodeView] Use a fancier function display name when using line tables only to better differentiate between functions..

I put some comments on https://bugs.llvm.org/show_bug.cgi?id=48432#c4 about this. I'm not sure we should keep going this direction. Thanks for getting numbers, though.

Fri, Jan 8, 3:21 PM · Restricted Project, Restricted Project
rnk added a comment to D94313: [tools] Mark output of tools as text if it is really text.

Please fix the two test failures observed in the harbormaster output. Your change will cause these tools to produce CRLF output, which tests must account for.

Fri, Jan 8, 11:16 AM · Restricted Project
rnk added a comment to D93668: [clang] Override the Fuchsia platform ABI using the triple environment.

It's fine to have a target tuple translate to a default C++ ABI.
But the C++ ABI selection is fundamentally not a target flavor thing. It's just a C++ ABI thing.
So using the target tuple as the sole mechanism to determine C++ ABI is fundamentally wrong.

Fri, Jan 8, 11:05 AM · Restricted Project

Thu, Jan 7

rnk accepted D93930: [NewPM][NVPTX] Port NVPTX opt passes.

lgtm with nit

Thu, Jan 7, 2:33 PM · Restricted Project
rnk accepted D93929: [NewPM][Hexagon] Fix HexagonVectorLoopCarriedReusePass position in pipeline.

lgtm

Thu, Jan 7, 2:26 PM · Restricted Project
rnk updated the diff for D94267: [PDB] Defer relocating .debug$S until commit time and parallelize it.
  • formatting
Thu, Jan 7, 2:20 PM · Restricted Project
rnk requested review of D94267: [PDB] Defer relocating .debug$S until commit time and parallelize it.
Thu, Jan 7, 2:19 PM · Restricted Project
rnk accepted D91529: [llvm-readobj] [ARMWinEH] Clearly print an invalid case of packed unwind info as such.

lgtm

Thu, Jan 7, 1:06 PM · Restricted Project
rnk accepted D94258: [CoroSplit][NewPM] Don't call LazyCallGraph functions to split when no clones.

lgtm

Thu, Jan 7, 1:02 PM · Restricted Project
rnk committed rGad55d5c3f32f: Simplify vectorcall argument classification of HVAs, NFC (authored by rnk).
Simplify vectorcall argument classification of HVAs, NFC
Thu, Jan 7, 11:14 AM
rnk added inline comments to D92751: [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate.
Thu, Jan 7, 10:01 AM · Restricted Project, Restricted Project

Wed, Jan 6

rnk added a comment to D93668: [clang] Override the Fuchsia platform ABI using the triple environment.

I guess a triple of -fuchsia-itanium would be a reasonable way of expressing this.

Wed, Jan 6, 1:58 PM · Restricted Project
rnk added a comment to D93668: [clang] Override the Fuchsia platform ABI using the triple environment.

Generally makes sense, but I had a concern.

Wed, Jan 6, 11:21 AM · Restricted Project
rnk added inline comments to D94153: [AMDGPU][Inliner] Remove amdgpu-inline and add new TTI inline hooks.
Wed, Jan 6, 10:59 AM · Restricted Project
rnk accepted D94182: [NFC] Don't copy MachineFrameInfo on each invocation of HasAlias.

lgtm

Wed, Jan 6, 10:00 AM · Restricted Project
rnk committed rG08e5e91e45af: [X86] Remove [ER]SP from all CSR lists (authored by rnk).
[X86] Remove [ER]SP from all CSR lists
Wed, Jan 6, 9:51 AM
rnk closed D94118: [X86] ESP should not be in the regcall CSR list.
Wed, Jan 6, 9:50 AM · Restricted Project
rnk added a comment to D94154: Unique Internal Linkage Name suffixes must be demangler friendly.

This is only demangler friendly if the name is already an itanium mangled name, right? (ie: the name starts with _Z) So it wouldn't work for C code?

Wed, Jan 6, 9:37 AM · Restricted Project, Restricted Project

Tue, Jan 5

rnk added inline comments to D94118: [X86] ESP should not be in the regcall CSR list.
Tue, Jan 5, 7:33 PM · Restricted Project
rnk added a comment to D94118: [X86] ESP should not be in the regcall CSR list.

I saw the document mentions ESP/RSP should be preserve for regcall. But I cannot think out a situation that ESP/RSP need to preserve either.

Tue, Jan 5, 7:22 PM · Restricted Project
rnk updated the diff for D94118: [X86] ESP should not be in the regcall CSR list.
  • Remove RSP from CSR lists too
Tue, Jan 5, 7:19 PM · Restricted Project
rnk committed rGf4bcbdf9cea1: Suppress GCC Wdangling-else warning on gtest macros (authored by rnk).
Suppress GCC Wdangling-else warning on gtest macros
Tue, Jan 5, 5:36 PM
rnk added a comment to D83892: [clang][cli] Port CodeGen option flags to new option parsing system.

Reposting my comment here, since this is where the discussion was:

Tue, Jan 5, 5:22 PM · Restricted Project, Restricted Project
rnk added a comment to rG741978d727a4: [clang][cli] Port CodeGen option flags to new option parsing system.

I did some comparisons of the sanitizer object files, and I think this change broke -gline-tables-only functionality. The new object file had a large .debug_loc section, which is only present when full debug info is enabled, and is not desired in this case.

Tue, Jan 5, 5:03 PM
rnk accepted D93732: [LLD][COFF] When using PCH.OBJ, ensure func_id records indices are remapped under /DEBUG:GHASH.

The Harbormaster precommit test stuff is complaining, but maybe it's just because of the binary files in the patch.

Tue, Jan 5, 5:00 PM · Restricted Project
rnk accepted D93720: [llvm-pdbutil] Don't crash when printing unknown CodeView type records.

It seems like Harbormaster / Jenkins didn't like the new test, so double check that it passes before landing.

Tue, Jan 5, 4:47 PM · Restricted Project
rnk accepted D93772: [Clang][Driver] Fix read-after-free when using /clang:.

lgtm

Tue, Jan 5, 4:11 PM · Restricted Project
rnk accepted D93828: [CGSCC][Coroutine][NewPM] Properly support function splitting/outlining.

I'm not an SCC expert, but Arthur explained to me how this works, and I trust the explanation.

Tue, Jan 5, 4:09 PM · Restricted Project
rnk accepted D93950: [LLD] [MinGW] Pass the --demangle and --no-demangle options to the COFF linker.

lgtm

Tue, Jan 5, 3:51 PM · Restricted Project
rnk requested review of D94118: [X86] ESP should not be in the regcall CSR list.
Tue, Jan 5, 1:48 PM · Restricted Project
rnk accepted D93946: [FuncAttrs] Infer noreturn.

lgtm

Tue, Jan 5, 11:36 AM · Restricted Project

Mon, Jan 4

rnk added a comment to D92751: [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate.

Thank for the update, apologies for not providing these suggestions the first time.

Mon, Jan 4, 12:12 PM · Restricted Project, Restricted Project

Sat, Dec 19

rnk committed rG0985a8bfea46: Fix left shift overflow UB in PPC backend on LLP64 platforms (authored by rnk).
Fix left shift overflow UB in PPC backend on LLP64 platforms
Sat, Dec 19, 5:46 PM

Dec 17 2020

rnk committed rZORG9b0551343174: Try to fix Windows stage 2 build issue (authored by rnk).
Try to fix Windows stage 2 build issue
Dec 17 2020, 1:31 PM
rnk committed rZORGb474bb2325bd: Fix next Python 3 Unicode issue, the gift that will keep giving for the next 20… (authored by rnk).
Fix next Python 3 Unicode issue, the gift that will keep giving for the next 20…
Dec 17 2020, 12:15 PM
rnk committed rZORG0ef61a790bbe: Put back missing strip in annotated script (authored by rnk).
Put back missing strip in annotated script
Dec 17 2020, 12:13 PM
rnk committed rZORG4cf14ad11aa3: Attempt to fix Python 3 issues in annotated scripts (authored by rnk).
Attempt to fix Python 3 issues in annotated scripts
Dec 17 2020, 12:06 PM
rnk accepted D93434: [test] Factor out creation of copy of SCC Nodes into function.

lgtm

Dec 17 2020, 11:32 AM · Restricted Project
rnk accepted D93458: clang-cl: Remove /Zd flag.

lgtm

Dec 17 2020, 10:57 AM · Restricted Project

Dec 16 2020

rnk added inline comments to D93427: [test] Cleanup some CGSCCPassManager tests.
Dec 16 2020, 4:58 PM · Restricted Project
rnk accepted D93427: [test] Cleanup some CGSCCPassManager tests.

lgtm, the comment is a suggestion

Dec 16 2020, 4:22 PM · Restricted Project
rnk added inline comments to D93427: [test] Cleanup some CGSCCPassManager tests.
Dec 16 2020, 4:22 PM · Restricted Project
rnk added a comment to D91659: Allow enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode.

@rnk Your thoughts on this would be appreciated.

Dec 16 2020, 2:11 PM · Restricted Project
rnk committed rGb7905e81fc3d: Fix split-debug.c test on Windows (authored by rnk).
Fix split-debug.c test on Windows
Dec 16 2020, 1:49 PM
rnk committed rG15ca54525d6c: Fix XCore test on Windows, the register order is reversed, for reasons unknown (authored by rnk).
Fix XCore test on Windows, the register order is reversed, for reasons unknown
Dec 16 2020, 1:33 PM
rnk committed rZORGccffa7a21c8d: Start Docker-izing sanitizer-windows (authored by rnk).
Start Docker-izing sanitizer-windows
Dec 16 2020, 10:51 AM
rnk closed D93340: Start Docker-izing sanitizer-windows.
Dec 16 2020, 10:51 AM
rnk accepted D93350: [Test] Fix undef var in catch-undef-behavior.c.

lgtm

Dec 16 2020, 9:52 AM · Restricted Project
rnk accepted D93306: [llvm-symbolizer][Windows] Add start line when searching in line table sections..

lgtm

Dec 16 2020, 9:50 AM · Restricted Project

Dec 15 2020

rnk committed rGb0b5d389635a: Document that AlignedCharArrayUnion exists to work around an MSVC bug (authored by rnk).
Document that AlignedCharArrayUnion exists to work around an MSVC bug
Dec 15 2020, 4:03 PM
rnk closed D93355: Document that AlignedCharArrayUnion exists to work around an MSVC bug.
Dec 15 2020, 4:03 PM · Restricted Project
rnk requested review of D93355: Document that AlignedCharArrayUnion exists to work around an MSVC bug.
Dec 15 2020, 4:00 PM · Restricted Project
rnk added a comment to D92509: ADT: Remove redundant `alignas` from IntervalMap, NFC.

I missed this email yesterday, but your solution SGTM.

It looks like you did not revert 65c5c9f92ec514ae41c8ea407d1c885737d699ec (ADT: Rely on std::aligned_union_t for math in AlignedCharArrayUnion, NFC), which changed it to rely on the math in std::aligned_union_t:

template <typename T, typename... Ts> struct AlignedCharArrayUnion {
  using AlignedUnion = std::aligned_union_t<1, T, Ts...>;
  alignas(alignof(AlignedUnion)) char buffer[sizeof(AlignedUnion)];
};

Should that be reverted too, or if not, why does alignof(std::aligned_union_t<...>) give us the right answer?

Dec 15 2020, 3:53 PM · Restricted Project
rnk requested review of D93340: Start Docker-izing sanitizer-windows.
Dec 15 2020, 2:42 PM
rnk added inline comments to D93306: [llvm-symbolizer][Windows] Add start line when searching in line table sections..
Dec 15 2020, 12:04 PM · Restricted Project
rnk added a comment to D92751: [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate.

Thanks for the fix. At first, misunderstood, I expected that an aggregate containing non-aggregates should be returned indirectly, and that the fix would be in the C++ ABI codepath. However, I see that is not the case. An aggregate may contain non-aggregates, and MSVC will in fact return such a type directly in X0/X1.

Dec 15 2020, 11:44 AM · Restricted Project, Restricted Project

Dec 14 2020

rnk added a reverting change for rG4b5dc150b986: ADT: Change AlignedCharArrayUnion to an alias of std::aligned_union_t, NFC: rGd2ed9d6b7ec6: Revert "ADT: Migrate users of AlignedCharArrayUnion to std::aligned_union_t….
Dec 14 2020, 5:06 PM
rnk added a reverting change for rGd10f9863a5ac: ADT: Migrate users of AlignedCharArrayUnion to std::aligned_union_t, NFC: rGd2ed9d6b7ec6: Revert "ADT: Migrate users of AlignedCharArrayUnion to std::aligned_union_t….
Dec 14 2020, 5:06 PM