thakis (Nico Weber)Email Not Verified
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 10 2013, 2:43 PM (297 w, 21 h)

Recent Activity

Yesterday

thakis added inline comments to D52266: [clang-cl] Provide separate flags for all the /O variants.
Thu, Sep 20, 11:09 AM
thakis added a comment to D52266: [clang-cl] Provide separate flags for all the /O variants.

Sorry, I didn't realize we both set off to do this in parallel. I've incorporated your changes into my patch.

Thu, Sep 20, 8:46 AM

Wed, Sep 19

thakis added a comment to D52193: RFC: [clang] Multithreaded compilation support.

Since it helps existing msbuild configs, adding this seems like a good thing to me.

Wed, Sep 19, 8:54 AM
thakis added a comment to D52266: [clang-cl] Provide separate flags for all the /O variants.

FWIW the recommendation against /Ox in my version is because of https://github.com/ulfjack/ryu/pull/70#issuecomment-412168459

Wed, Sep 19, 8:14 AM
thakis added inline comments to D52266: [clang-cl] Provide separate flags for all the /O variants.
Wed, Sep 19, 8:10 AM

Mon, Sep 17

thakis created D52184: Remove dead function user_cache_directory().
Mon, Sep 17, 12:24 PM
thakis added a comment to D52143: Make initializeOutputStream() return false on error and true on success..

Not formally in any kind of mailing list discussion

Mon, Sep 17, 10:59 AM
thakis added a comment to D52145: lld-link: Also demangle undefined dllimported symbols..

Thanks! Changed the comment; landing.

Mon, Sep 17, 9:32 AM

Sat, Sep 15

thakis added a comment to D52145: lld-link: Also demangle undefined dllimported symbols..

Thanks!

Sat, Sep 15, 6:39 PM
thakis updated the diff for D52145: lld-link: Also demangle undefined dllimported symbols..

fix case on variable

Sat, Sep 15, 5:40 PM
thakis created D52145: lld-link: Also demangle undefined dllimported symbols..
Sat, Sep 15, 5:39 PM
thakis abandoned D51887: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..
Sat, Sep 15, 4:42 PM
thakis abandoned D51957: https://reviews.llvm.org/D51951, naive parallel hash variant.
Sat, Sep 15, 4:42 PM
thakis created D52143: Make initializeOutputStream() return false on error and true on success..
Sat, Sep 15, 4:37 PM
thakis added a comment to rL342148: Renovate CMake files in the `llvm-(cfi-verify|exegesis|mca)` tools..

The revert helped, see e.g. http://lab.llvm.org:8011/builders/lld-x86_64-darwin13/builds/25686 and http://lab.llvm.org:8011/builders/lld-x86_64-freebsd/builds/23378 (compare to previous build)

Sat, Sep 15, 12:33 PM
thakis added a comment to rL342148: Renovate CMake files in the `llvm-(cfi-verify|exegesis|mca)` tools..

I reverted this (and follow-on fix attempts r342154 r342180 r342182 r342193) in r342336 in an attempt to heal the lld bots.

Sat, Sep 15, 12:07 PM
thakis added a comment to D52104: lld-link: print demangled symbol names for "undefined symbol" diagnostics.

Ah, looks like all changes are mentioned at the top, just not as comments further down.

Sat, Sep 15, 11:41 AM
thakis added a comment to D51956: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

https://reviews.llvm.org/rLLD342334 too. Thanks!

Sat, Sep 15, 11:41 AM
thakis added a comment to D52104: lld-link: print demangled symbol names for "undefined symbol" diagnostics.

Also https://reviews.llvm.org/rL342331 and https://reviews.llvm.org/rLLD342332

Sat, Sep 15, 11:35 AM
thakis added a comment to D52104: lld-link: print demangled symbol names for "undefined symbol" diagnostics.

Thanks! Submitting...

Sat, Sep 15, 11:23 AM

Fri, Sep 14

thakis added inline comments to D52104: lld-link: print demangled symbol names for "undefined symbol" diagnostics.
Fri, Sep 14, 12:59 PM
thakis added a comment to D51956: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

The symptoms of a collision are just going to be you can’t debug the
program, so not very severe imo, especially since it would almost certainly
be resolved on the next incremental build

Fri, Sep 14, 12:06 PM
thakis added a comment to D52104: lld-link: print demangled symbol names for "undefined symbol" diagnostics.

Thanks!

Fri, Sep 14, 11:40 AM
thakis updated the diff for D52104: lld-link: print demangled symbol names for "undefined symbol" diagnostics.

ruiu comments

Fri, Sep 14, 11:40 AM
thakis added inline comments to D51956: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..
Fri, Sep 14, 11:25 AM
thakis updated the diff for D51956: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

improve comment

Fri, Sep 14, 10:38 AM
thakis closed D52095: Introduce explicit add_file_reading_unittest target for tests that use llvm::getInputFileDirectory().

r342248, thanks!

Fri, Sep 14, 10:37 AM
thakis created D52104: lld-link: print demangled symbol names for "undefined symbol" diagnostics.
Fri, Sep 14, 10:31 AM
thakis added inline comments to D51956: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..
Fri, Sep 14, 10:27 AM
thakis updated the diff for D51956: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

rebase, minor cleanups

Fri, Sep 14, 8:13 AM
thakis added a comment to D51561: [CMake] Add support for unittests that have input files.

All of the lit tests have this restriction, is this something you would do
with unit tests but not lit tests?

Fri, Sep 14, 8:00 AM
thakis created D52095: Introduce explicit add_file_reading_unittest target for tests that use llvm::getInputFileDirectory().
Fri, Sep 14, 7:54 AM
thakis added a comment to D51561: [CMake] Add support for unittests that have input files.

Unrelatedly, another problem with the current design is that llvm.srcdir.txt must contain an absolute path, which makes it impossible to build on one machine and then copy artifacts and test inputs to another machine and run tests on another machine. DebugInfoPDBTests is the only test in all llvm, lld, clang tests that has this restriction. Requiring some defined fixed pwd for unit tests with inputs would address this issue as well. (As would finding a way to not require reading a file off disk for this test.)

Fri, Sep 14, 7:41 AM
thakis added a comment to D51561: [CMake] Add support for unittests that have input files.

These aren't demangle tests fwiw. But that just complicates the cmake
logic, and I don't see the benefit of doing so. Writing 3k of data to the
disk is hardly going to be a noticeable part of the configure step.

The benefits are:

  1. Unit tests reading inputs are bad form; doing that should require an explicit opt-in.
  2. While writing 3k of data to disk isn't causing measurable perf issues, writing a text file next to every test binary just because one test binary needs it is still plain ugly.

    And not doing it is super easy to do. I don't understand the pushback here.
Fri, Sep 14, 6:04 AM

Thu, Sep 13

thakis added a comment to D51956: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

Actually, this replaces https://reviews.llvm.org/D51887

Thu, Sep 13, 11:17 AM
thakis added a comment to D51956: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

This replaces https://reviews.llvm.org/D51957.

Thu, Sep 13, 10:52 AM
thakis retitled D51956: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence. from https://reviews.llvm.org/D51951, naive sequential hash variant to lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..
Thu, Sep 13, 10:52 AM
thakis added a comment to D51887: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

The situation on Windows is like on Linux. I built chrome.dll with our pinned lld (which was compiled with clang), with a locally-built lld (built by msvc 2017), and then with this patch and the other two patches linked from here. Min-of-5 times are comparable (full data in parens):

Thu, Sep 13, 10:50 AM
thakis added a comment to D51887: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

Did some measurements on my linux box (didn't have access to my win box). There, just naively calling xxHash64 on the 1.4GB chrome.dll.pdb file after creating it, doing the parallel xxHas64 computation, this patch, and the old nondeterministic guid code all take about the same amount of time (around 32s, min-of-4 links with each approach is within 0.2s of that). I'll try this on Windows tomorrow.

(Naive: https://reviews.llvm.org/D51956 Parallel: https://reviews.llvm.org/D51957)

I'm a little surprised it's this fast. Are you sure it's even doing anything? What kind of hard drive are you using? A SATA III bus interface, which is still probably the single most common interface used by SSDs, has a theoretical maximum transfer rate of 600MB/s, so you'd be adding at least 2.5 seconds to the link, and that's under optimal circumstances. A SATA II interface, which is also still common enough, is 300MB/s. nVME and PCIe interfaces can reach gigabytes / second but we definitely shoudln't be basing measurements off of those. I don't really have a super strong opinion, but it's something to think about.

Thu, Sep 13, 9:46 AM

Wed, Sep 12

thakis added a comment to D51981: lld-link: If an input file doesn't exist, don't print diagnostics after "input file doesn't exist"..

(Forcing an email to be sent since I accidentally clicked "save" before adding llvm-commits)

Wed, Sep 12, 7:01 AM
thakis updated the summary of D51981: lld-link: If an input file doesn't exist, don't print diagnostics after "input file doesn't exist"..
Wed, Sep 12, 6:54 AM
thakis created D51981: lld-link: If an input file doesn't exist, don't print diagnostics after "input file doesn't exist"..
Wed, Sep 12, 6:51 AM

Tue, Sep 11

thakis added a comment to D51887: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

Did some measurements on my linux box (didn't have access to my win box). There, just naively calling xxHash64 on the 1.4GB chrome.dll.pdb file after creating it, doing the parallel xxHas64 computation, this patch, and the old nondeterministic guid code all take about the same amount of time (around 32s, min-of-4 links with each approach is within 0.2s of that). I'll try this on Windows tomorrow.

Tue, Sep 11, 4:24 PM
thakis created D51957: https://reviews.llvm.org/D51951, naive parallel hash variant.
Tue, Sep 11, 4:20 PM
thakis created D51956: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..
Tue, Sep 11, 4:20 PM
thakis updated the diff for D51887: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

tests pass

Tue, Sep 11, 7:34 AM
thakis added a comment to D51887: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

(I don't understand yet how /pdbaltpath: makes it into the hash)

Tue, Sep 11, 7:27 AM
thakis added a comment to D51887: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

(Landed r341945 to address the 2 differing bytes that caused the guid for rsds.test to be different. It's still different due to different /pdbaltpath: flags, but the pdb contents are now identical except for guid and timestamp. I don't understand yet how /pdbaltpath: makes it into the hash, but for the same /pdbaltpath: it seems to be deterministic now. Looking more…)

Tue, Sep 11, 7:15 AM
thakis added a comment to D50658: Hot cold splitting pass.

The new test fails like so on my mac laptop:

Tue, Sep 11, 7:03 AM

Mon, Sep 10

thakis added a comment to D51887: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..

I wonder if you can just compute a hash for a mmap'ed PDB file instead of defining a new type of Stream. That's what we do for ELF when computing a build-id, and I found that's easy to do. It is also easy to parallelize hash computation by making it a tree hash.

Mon, Sep 10, 4:24 PM
thakis created D51887: lld-link: Set PDB GUID to hash of PDB contents instead of to a random byte sequence..
Mon, Sep 10, 12:49 PM

Sat, Sep 8

thakis added inline comments to rL341729: [PDB] Support pointer types in the native reader..
Sat, Sep 8, 2:07 PM

Fri, Sep 7

thakis added inline comments to D51784: ms: Insert $$Z in mangling between directly consecutive parameter packs..
Fri, Sep 7, 6:35 AM
thakis created D51784: ms: Insert $$Z in mangling between directly consecutive parameter packs..
Fri, Sep 7, 6:30 AM
thakis added a comment to D51561: [CMake] Add support for unittests that have input files.

These aren't demangle tests fwiw. But that just complicates the cmake
logic, and I don't see the benefit of doing so. Writing 3k of data to the
disk is hardly going to be a noticeable part of the configure step.

Fri, Sep 7, 5:54 AM

Thu, Sep 6

thakis added a comment to D51561: [CMake] Add support for unittests that have input files.

Doesn’t mean it’s a good or bad pattern, just that “only 1 file will use
it” is already false, because we can make lldb use it as a followup and
that will get many more.

I did think about alternative solutions, If this CL wasn’t here, the
alternatives i came up would be to either not write this test, or spend
several days/weeks writing a tool whose only purpose was to make it
possible to write this test.

Thu, Sep 6, 3:22 PM
thakis added a comment to D51561: [CMake] Add support for unittests that have input files.

We may call these "unit" tests, but they're really gtests, and developers everywhere use gtest for more than just unit tests. We already have tests that create temp files. I don't see why reaching back into the source directory for inputs is too heavyweight for us.

Thu, Sep 6, 1:32 PM
thakis added a comment to D51561: [CMake] Add support for unittests that have input files.

Also also, opening a file from a unit test kind of makes it not a unit test, since those are supposed to not hit the disk etc. Maybe that test wants to be its own binary instead that's called from a lit script, instead of being a unit test?

Thu, Sep 6, 11:08 AM
thakis added a comment to D51561: [CMake] Add support for unittests that have input files.

Sorry, didn't see this earlier. Writing a llvm.srcdir.txt next to every single unit test binary seems like a pretty roundabout way of doing this. How about instead passing the src dir in a runtime flag in http://llvm-cs.pcc.me.uk/utils/lit/lit/formats/googletest.py#107 if the lit config asks for it? Then you don't need to touch the disk to get the src dir path, it's not passed to _all_ binaries, and cmake doesn't have to write a file to disk for every test binary.

Thu, Sep 6, 11:07 AM
thakis added inline comments to D50659: [CodeGen] Fix remaining zext() assertions in SelectionDAG.
Thu, Sep 6, 10:55 AM
thakis added a comment to D45179: [libc++] Add _LIBCPP_ENABLE_NODISCARD and _LIBCPP_NODISCARD_EXT to allow pre-C++2a [[nodiscard]].

What's the status here? Did this land?

Thu, Sep 6, 7:06 AM

Wed, Sep 5

thakis added inline comments to D51652: lld-link: Write an empty "repro" debug directory entry if /Brepro is passed.
Wed, Sep 5, 5:28 AM
thakis updated the diff for D51652: lld-link: Write an empty "repro" debug directory entry if /Brepro is passed.
In D51652#1224017, @pcc wrote:

I wasn't imagining that we'd write 32 bits of hash to the repro directory but rather the 64-bit xxhash that we currently truncate to 32 bits to get it to fit into the timestamp field. Without that, isn't there a high probability of collisions after 65536 files (which could be small depending on the project)?

Wed, Sep 5, 5:28 AM

Tue, Sep 4

thakis added a comment to D51652: lld-link: Write an empty "repro" debug directory entry if /Brepro is passed.

Quoting from the PE spec (see bug for more): "The raw data of this debug entry may be empty". So I don't think it was a bug. Maybe tools had problems with the zero-sized entries, but they didn't bother to update the spec. So I'd suggest to go with this and only add more code if needed (?). We could add a size and just add the 4-byte hash to TimeDateStamps to fill it in at the end, but while it's easy to do it feels a bit cargo-culty.

Tue, Sep 4, 3:42 PM
thakis added a comment to D51652: lld-link: Write an empty "repro" debug directory entry if /Brepro is passed.

MSVC on my laptop has the same behavior as on my desktop:

Tue, Sep 4, 3:06 PM
thakis added a comment to D51652: lld-link: Write an empty "repro" debug directory entry if /Brepro is passed.

pcc: what if you do cl /Zi /Brepro foo.c?

Tue, Sep 4, 1:31 PM
thakis created D51652: lld-link: Write an empty "repro" debug directory entry if /Brepro is passed.
Tue, Sep 4, 12:48 PM
thakis added a comment to D51354: Fix the -print-multi-directory flag to print the selected multilib..

The test fails on my system like so:

Tue, Sep 4, 11:02 AM
thakis created D51635: clang-cl: Pass /Brepro to linker if it was passed to the compiler.
Tue, Sep 4, 8:18 AM

Mon, Sep 3

thakis closed D51579: Rename a few unittests/.../Foo.cpp files to FooTest.cpp.
Mon, Sep 3, 6:09 AM
thakis added a comment to D51579: Rename a few unittests/.../Foo.cpp files to FooTest.cpp.

r341313, thanks! I had svn mvd the files. Diffs for mv'd files look a bit weird; phab is probably doing the best it can do here.

Mon, Sep 3, 5:45 AM

Sun, Sep 2

thakis added a comment to D51545: Enable -Wtautological-unsigned-zero-compare under -Wextra.

We don't match gcc's -Wextra behvior. We generally try to not put a ton of stuff in Wextra that isn't in -Wall. (We also generally don't put a lot of stuff in -Wall that isn't enabled by default.) So I don't think we want this.

Sun, Sep 2, 4:22 PM
thakis created D51579: Rename a few unittests/.../Foo.cpp files to FooTest.cpp.
Sun, Sep 2, 4:21 PM

Thu, Aug 30

thakis closed D51357: Remove LIT_SITE_CFG_IN_FOOTER..

r341130 (clang-tools-extra), r341132 (clang), r341134 (lld), and r341135 (llvm). Thanks!

Thu, Aug 30, 3:15 PM

Tue, Aug 28

thakis created D51357: Remove LIT_SITE_CFG_IN_FOOTER..
Tue, Aug 28, 7:43 AM

Mon, Aug 27

thakis added a comment to D51174: Move Microsoft Demangler to Support.

What if we produced a .lib file that had only the minimal set of object
files needed to link this, and make MicrosoftDemangle.h not include other
llvm headers, then you could just link against that object file and it
should be effectively independent.

Would that work?

Mon, Aug 27, 5:25 AM

Thu, Aug 23

thakis added a comment to D51174: Move Microsoft Demangler to Support.

FWIW I'd like to use the new demangler in demumble [1], but if I need to pull in most of Support of it then I probably won't do that. I agree this shouldn't impact LLVM much (or at all), but having stand-alone-ish demangler code is probably nice for several other projects too. So since the itanium demangler is still in the other place and likely will stay there, maybe it's nice to keep the ms demangler next to it. (But as I said, don't weigh this feedback heavily.)

Thu, Aug 23, 5:32 PM

Wed, Aug 22

thakis closed D51134: win: Omit ".exe" from clang and clang-cl driver-level diagnostics..

r340498, thanks!

Wed, Aug 22, 4:54 PM
thakis closed D51133: win: Omit ".exe" from lld warning and error messages..

r340487, thanks!

Wed, Aug 22, 4:54 PM
thakis closed D51117: lld-link: Separate 'undefined symbol' errors with just one newline, not two..

r340482, thanks!

Wed, Aug 22, 4:46 PM
thakis closed D51140: Fix two RUN: lines that were unintentionally spelled "RN:"..

r340481, thanks!

Wed, Aug 22, 4:45 PM
thakis created D51140: Fix two RUN: lines that were unintentionally spelled "RN:"..
Wed, Aug 22, 4:13 PM
thakis updated the diff for D51133: win: Omit ".exe" from lld warning and error messages..

Thanks, that's a good suggestion. I put the new function in Common/Args.h; I'm not sure if that's the best place.

Wed, Aug 22, 4:11 PM
thakis created D51134: win: Omit ".exe" from clang and clang-cl driver-level diagnostics..
Wed, Aug 22, 3:43 PM
thakis updated the diff for D51133: win: Omit ".exe" from lld warning and error messages..
Wed, Aug 22, 3:42 PM
thakis created D51133: win: Omit ".exe" from lld warning and error messages..
Wed, Aug 22, 3:42 PM

Aug 22 2018

thakis created D51117: lld-link: Separate 'undefined symbol' errors with just one newline, not two..
Aug 22 2018, 11:55 AM
thakis accepted D50610: Disable -Wnoexcept-type wholesale.
Aug 22 2018, 11:52 AM
thakis closed D51076: lld-link: Emit a warning if one of {main,wmain} and {WinMain,wWinMain} are both present and no explicit /subsystem: flag is passed..

r340420, thanks!

Aug 22 2018, 9:48 AM

Aug 21 2018

thakis created D51076: lld-link: Emit a warning if one of {main,wmain} and {WinMain,wWinMain} are both present and no explicit /subsystem: flag is passed..
Aug 21 2018, 5:33 PM
thakis closed D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr()..

r340348, thanks!

Aug 21 2018, 3:20 PM
thakis updated the diff for D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr()..

Add tests.

Aug 21 2018, 2:41 PM

Aug 20 2018

thakis added a comment to D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr()..

I don't think this is NFC. Testcase:

long long int a, b, c, d;
unsigned char f() { return _InterlockedCompareExchange128(&(++a), ++b, ++c, &(++d)); }

Today, Clang increments a, b, c, and d twice each in f().

Thanks for pointing this out, good to hear that this even happens to find a bug :-) I'll add some tests to document the progression.

Aug 20 2018, 12:09 PM
thakis added a comment to D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr()..

I don't think this is NFC. Testcase:

long long int a, b, c, d;
unsigned char f() { return _InterlockedCompareExchange128(&(++a), ++b, ++c, &(++d)); }

Today, Clang increments a, b, c, and d twice each in f().

Aug 20 2018, 12:08 PM
thakis created D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr()..
Aug 20 2018, 10:28 AM
thakis closed D50907: Make __shiftleft128 / __shiftright128 real compiler built-ins..
Aug 20 2018, 10:23 AM

Aug 17 2018

thakis accepted D50877: [MS] Mangle a hash of the main file path into anonymous namespaces.

Can you explicitly mention that this intentionally doesn't use an absolute path in MicrosoftMangleContextImpl() or similar?

Aug 17 2018, 12:50 PM
thakis added a comment to D50907: Make __shiftleft128 / __shiftright128 real compiler built-ins..

r340048, thanks!

Aug 17 2018, 10:19 AM
thakis updated the summary of D50907: Make __shiftleft128 / __shiftright128 real compiler built-ins..
Aug 17 2018, 9:58 AM
thakis added a comment to D50907: Make __shiftleft128 / __shiftright128 real compiler built-ins..

Forgot to add cfe-commits :-( Doing that now.

Aug 17 2018, 9:58 AM