pcc (Peter Collingbourne)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 28 2012, 2:34 PM (285 w, 5 d)

Recent Activity

Yesterday

pcc added a comment to D47904: Support option -plugin-opt=dwo_dir=.

Can you add a test case?

Wed, Jun 20, 11:28 AM

Mon, Jun 18

pcc added inline comments to D44788: Add an option to support debug fission on implicit ThinLTO..
Mon, Jun 18, 3:55 PM
pcc committed rL334982: IRgen: Mark aliases of ctors and dtors as unnamed_addr..
IRgen: Mark aliases of ctors and dtors as unnamed_addr.
Mon, Jun 18, 2:03 PM
pcc committed rC334982: IRgen: Mark aliases of ctors and dtors as unnamed_addr..
IRgen: Mark aliases of ctors and dtors as unnamed_addr.
Mon, Jun 18, 2:03 PM
pcc closed D48206: IRgen: Mark aliases of ctors and dtors as unnamed_addr..
Mon, Jun 18, 2:03 PM

Thu, Jun 14

pcc created D48206: IRgen: Mark aliases of ctors and dtors as unnamed_addr..
Thu, Jun 14, 8:23 PM
pcc updated the diff for D48146: ELF: Implement --icf=safe using address-significance tables..
  • Add some more tests
Thu, Jun 14, 1:46 PM
pcc added a comment to D48146: ELF: Implement --icf=safe using address-significance tables..

Thanks for the patches. Out of curiosity did you have any thoughts about how the proposal to ELF was going? https://groups.google.com/forum/#!topic/generic-abi/MPr8TVtnVn4 . In particular compressed SHT_SYMATTR seemed promising.

Thu, Jun 14, 12:50 PM

Wed, Jun 13

pcc updated the diff for D48155: Teach Clang to emit address-significance tables..
  • Add some additional driver tests
Wed, Jun 13, 6:23 PM
pcc created D48155: Teach Clang to emit address-significance tables..
Wed, Jun 13, 6:20 PM
pcc added a dependent revision for D48143: CodeGen: Add a target option for emitting .addrsig directives for all address-significant symbols.: D48155: Teach Clang to emit address-significance tables..
Wed, Jun 13, 6:20 PM
pcc committed rC334673: Driver: De-duplicate some code. NFCI..
Driver: De-duplicate some code. NFCI.
Wed, Jun 13, 5:09 PM
pcc committed rL334673: Driver: De-duplicate some code. NFCI..
Driver: De-duplicate some code. NFCI.
Wed, Jun 13, 5:09 PM
pcc updated the diff for D47567: Implement CFI for indirect calls via a member function pointer..
  • Add some more documentation to ControlFlowIntegrity.rst
Wed, Jun 13, 4:46 PM
pcc committed rC334671: docs: Add a missing LTO visibility reference..
docs: Add a missing LTO visibility reference.
Wed, Jun 13, 4:25 PM
pcc committed rL334671: docs: Add a missing LTO visibility reference..
docs: Add a missing LTO visibility reference.
Wed, Jun 13, 4:25 PM
pcc committed rL334669: docs: Correct some misstatements in the control flow integrity docs..
docs: Correct some misstatements in the control flow integrity docs.
Wed, Jun 13, 4:23 PM
pcc committed rC334669: docs: Correct some misstatements in the control flow integrity docs..
docs: Correct some misstatements in the control flow integrity docs.
Wed, Jun 13, 4:22 PM
pcc updated the diff for D48146: ELF: Implement --icf=safe using address-significance tables..
  • Address review comments and fix a few problems with the test case
Wed, Jun 13, 3:07 PM
pcc created D48146: ELF: Implement --icf=safe using address-significance tables..
Wed, Jun 13, 1:04 PM
pcc added a dependent revision for D47744: MC: Implement support for new .addrsig and .addrsig_sym directives.: D48146: ELF: Implement --icf=safe using address-significance tables..
Wed, Jun 13, 1:04 PM
pcc created D48143: CodeGen: Add a target option for emitting .addrsig directives for all address-significant symbols..
Wed, Jun 13, 12:34 PM
pcc added a dependent revision for D47744: MC: Implement support for new .addrsig and .addrsig_sym directives.: D48143: CodeGen: Add a target option for emitting .addrsig directives for all address-significant symbols..
Wed, Jun 13, 12:34 PM
pcc committed rL334630: LTO: Keep file handles open for memory mapped files..
LTO: Keep file handles open for memory mapped files.
Wed, Jun 13, 11:07 AM
pcc closed D48051: LTO: Keep file handles open for memory mapped files..
Wed, Jun 13, 11:07 AM
pcc added inline comments to D47266: Update thin-lto cache file atimes when on windows.
Wed, Jun 13, 10:41 AM

Tue, Jun 12

pcc retitled D48051: LTO: Keep file handles open for memory mapped files. from LTO: Work around a Windows kernel bug by keeping file handles open for memory mapped files. to LTO: Keep file handles open for memory mapped files..
Tue, Jun 12, 8:06 PM
pcc updated the diff for D48051: LTO: Keep file handles open for memory mapped files..
  • Replace FILE_FLAG_DELETE_ON_CLOSE with SetFileInformationByHandle/FileDispositionInfo
Tue, Jun 12, 7:52 PM
pcc retitled D47567: Implement CFI for indirect calls via a member function pointer. from [wip] Implement CFI for indirect calls via a member function pointer. to Implement CFI for indirect calls via a member function pointer..
Tue, Jun 12, 7:16 PM
pcc updated the diff for D47567: Implement CFI for indirect calls via a member function pointer..
  • Address TODOs
Tue, Jun 12, 7:16 PM
pcc added a comment to D47799: [COFF] add /errorrepro to save reproducer on error.

As I pointed out earlier, /linkrepro isn't really what I would want for that because you either need to re-run the link until you hit the same failure again, which could be very time consuming, or pre-emptively run with /linkrepro, which slows down links and writes lots of bytes you usually don't care about.

Tue, Jun 12, 5:50 PM
pcc added a comment to D47799: [COFF] add /errorrepro to save reproducer on error.

I still don't see a compelling reason to do this to be honest. The use case we had in mind was to help debug an intermittent failure, but as we discovered it wouldn't have helped debug that failure. Also, if a failure is intermittent then it can be reproduced just as much with /errorrepro as with /linkrepro.

Tue, Jun 12, 4:03 PM

Mon, Jun 11

pcc added a comment to D48051: LTO: Keep file handles open for memory mapped files..

We can’t transfer ownership of the handle in that case? With the flag
cleared we shouldn’t have to worry about the pending delete error

Mon, Jun 11, 5:27 PM
pcc added a comment to D48051: LTO: Keep file handles open for memory mapped files..
In D48051#1129127, @pcc wrote:
In D48051#1129121, @pcc wrote:
In D48051#1129112, @pcc wrote:

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

I don't think so, basically the process that creates a cache entry could exit at any time before or after any processes that might read the cache entry.

What happens if process A creates a cache entry, then exits, then process B tries to re-use the same cache entry? Since it got closed (and deleted) before anyone could try to re-use it, does it just create it again?

It doesn't normally get deleted because we clear the delete-on-close bit. It might get deleted later as a result of cache pruning, but that's a separate mechanism. But if it does get deleted through cache pruning, we do create it again.

It seems like it would be more efficient from a hit/miss ratio perspective if once a cache file was created, it would never be created again.

That's exactly what happens, provided that the cache entry isn't pruned.

So after the entire link operation is finished, there are still cache files lying around on the hard drive (because their delete on close bit was cleared)? If that's the case, can we just not even specify it in the first place? What would be the implications of that?

It would mean that if a link process is terminated forcefully or if there is a power cut, the link process's temporary files would be left on the disk.

That's why I was thinking about using SetFileInformationByHandle/FileDispositionInfo instead of FILE_FLAG_DELETE_ON_CLOSE to manage the delete-on-close bit for temporary files. It would mean that if the process is terminated in the window between CreateFile and SetFileInformationByHandle the file would be left on the disk, but other than that we should basically get the same behaviour. Since getting terminated in that window is pretty unlikely, it seems okay to me.

Would the temporary files potentially be re-used on a subsequent link (if it were restarted by the user) and then cause mysterious failures? If not, I'm honestly fine to just never set the flag at all. We've been burned by it hard enough that who knows if we're triggering some additional strange / subtle codepath. That said, your idea is probably fine. At least it solves all the problems and eliminates all the duplicate handles and complexity.

Mon, Jun 11, 5:07 PM
pcc added a comment to D48051: LTO: Keep file handles open for memory mapped files..
In D48051#1129121, @pcc wrote:
In D48051#1129112, @pcc wrote:

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

I don't think so, basically the process that creates a cache entry could exit at any time before or after any processes that might read the cache entry.

What happens if process A creates a cache entry, then exits, then process B tries to re-use the same cache entry? Since it got closed (and deleted) before anyone could try to re-use it, does it just create it again?

It doesn't normally get deleted because we clear the delete-on-close bit. It might get deleted later as a result of cache pruning, but that's a separate mechanism. But if it does get deleted through cache pruning, we do create it again.

It seems like it would be more efficient from a hit/miss ratio perspective if once a cache file was created, it would never be created again.

That's exactly what happens, provided that the cache entry isn't pruned.

So after the entire link operation is finished, there are still cache files lying around on the hard drive (because their delete on close bit was cleared)? If that's the case, can we just not even specify it in the first place? What would be the implications of that?

Mon, Jun 11, 4:48 PM
pcc added a comment to D48051: LTO: Keep file handles open for memory mapped files..
In D48051#1129112, @pcc wrote:

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

I don't think so, basically the process that creates a cache entry could exit at any time before or after any processes that might read the cache entry.

What happens if process A creates a cache entry, then exits, then process B tries to re-use the same cache entry? Since it got closed (and deleted) before anyone could try to re-use it, does it just create it again?

Mon, Jun 11, 4:38 PM
pcc added a comment to D48051: LTO: Keep file handles open for memory mapped files..
In D48051#1129100, @pcc wrote:

I don't think that would prevent other processes from opening the file. Both processes just have to make sure they both specify FILE_SHARE_DELETE.

As I wrote in my message to Bob that doesn't seem to work. According to process monitor the second process fails to open the file with "DELETE PENDING".

That's because a handle has been opened with FILE_FLAG_DELETE_ON_CLOSE and then subsequently closed. As soon as you close a file that has been openend with that flag, no other opens can happen on the file. That's why I suggested transferring ownership of it.

Mon, Jun 11, 4:22 PM
pcc added a comment to D48051: LTO: Keep file handles open for memory mapped files..

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

Mon, Jun 11, 4:07 PM
pcc added a comment to D48051: LTO: Keep file handles open for memory mapped files..
In D48051#1129059, @pcc wrote:
In D48051#1129014, @pcc wrote:

Is there any we can just transfer ownership of the original handle (with FILE_FLAG_DELETE_ON_CLOSE) from whoever had it before to the TempFile. This way we can just not close that handle until we're sure we're done with it.

I'm not sure what you mean. The FILE_FLAG_DELETE_ON_CLOSE handle is created and owned by TempFile. Do you mean transferring ownership from TempFile to MemoryBuffer when we map the file?

Yea, sorry. I'm just worried that by copying and duplicating the handle, and cancelling the delete on close, and all this other stuff we're just exposing ourselves to even more strange problems. It seems like the "correct" thing to do is to just keep the original handle open until we're absolutely done with that file.

As I wrote in the commit message, that would prevent other processes from opening the file. What that would mean is that if we have two concurrent link processes they would not be able to share results.

I guess what we could do instead is exclusively use SetFileInformationByHandle/FileDispositionInfo for delete-on-close to avoid needing to do weird things to cancel FILE_FLAG_DELETE_ON_CLOSE. That should solve the sharing problem as well.

Also it would mean that we would need to change all other clients of MemoryBuffer::getOpenFile{,Slice}() to pass FD ownership to the MemoryBuffer. While that would seem to work for most in-tree clients there does appear to be at least one case where the FD is owned by external code: http://llvm-cs.pcc.me.uk/tools/gold/gold-plugin.cpp#498

So we'd probably either want the FD to be optionally owned or require the client to duplicate the handle if it doesn't own it. But that would mean that someone would still end up needing to duplicate on Windows in the non-owned case.

I don't think that would prevent other processes from opening the file. Both processes just have to make sure they both specify FILE_SHARE_DELETE.

Mon, Jun 11, 4:01 PM
pcc added a comment to D48051: LTO: Keep file handles open for memory mapped files..
In D48051#1129014, @pcc wrote:

Is there any we can just transfer ownership of the original handle (with FILE_FLAG_DELETE_ON_CLOSE) from whoever had it before to the TempFile. This way we can just not close that handle until we're sure we're done with it.

I'm not sure what you mean. The FILE_FLAG_DELETE_ON_CLOSE handle is created and owned by TempFile. Do you mean transferring ownership from TempFile to MemoryBuffer when we map the file?

Yea, sorry. I'm just worried that by copying and duplicating the handle, and cancelling the delete on close, and all this other stuff we're just exposing ourselves to even more strange problems. It seems like the "correct" thing to do is to just keep the original handle open until we're absolutely done with that file.

Mon, Jun 11, 3:45 PM
pcc added a comment to D48051: LTO: Keep file handles open for memory mapped files..

Thanks for the fix! A couple of suggestions:

I wouldn't call it a bug unless we are sure that it is. My reading of the documentation of FILE_FLAG_DELETE_ON_CLOSE is that what we're hitting may actually be the intended behavior. Instead of "bug", can we describe is at "observed behavior" or words to that effect? That way, we aren't implying that the behavior is incorrect while still explaining why our code needs to be the way it is.

Mon, Jun 11, 3:08 PM
pcc added a comment to D48051: LTO: Keep file handles open for memory mapped files..

Is there any we can just transfer ownership of the original handle (with FILE_FLAG_DELETE_ON_CLOSE) from whoever had it before to the TempFile. This way we can just not close that handle until we're sure we're done with it.

Mon, Jun 11, 3:08 PM
pcc created D48051: LTO: Keep file handles open for memory mapped files..
Mon, Jun 11, 1:36 PM
pcc accepted D47898: IRMover: Account for matching types present across modules.

LGTM

Mon, Jun 11, 12:42 PM
pcc added a comment to D47994: [Darwin] Do not error on '-lto_library' option.

ELF linkers support a similar flag, -plugin, for passing the path to an LTO plugin. At least on Linux we can't always tell whether the linker is lld so we just pass -plugin all the time, and then in the linker we ignore it. We can do the same for -lto_library.

Mon, Jun 11, 12:33 PM

Wed, Jun 6

pcc committed rL334149: Add definition for ELF dynamic tag DT_SYMTAB_SHNDX..
Add definition for ELF dynamic tag DT_SYMTAB_SHNDX.
Wed, Jun 6, 5:12 PM
pcc closed D47803: Add definition for ELF dynamic tag DT_SYMTAB_SHNDX..
Wed, Jun 6, 5:12 PM
pcc committed rL334147: llvm-readobj: fix printing number of relocations in Android packed format..
llvm-readobj: fix printing number of relocations in Android packed format.
Wed, Jun 6, 5:07 PM
pcc closed D47800: llvm-readobj: fix printing number of relocations in Android packed format..
Wed, Jun 6, 5:07 PM
pcc accepted D47790: Expand the file comment for the error handlers..

LGTM

Wed, Jun 6, 4:55 PM
pcc added a comment to D47799: [COFF] add /errorrepro to save reproducer on error.

Here is the args.gn I was using. It's basically the same as the bot:

is_clang = true
is_component_build = false
is_debug = false
is_official_build = true
symbol_level = 2
use_lld = true
use_thin_lto = true

I also had a local patch to chrome to get it to use my hacked lld:

diff --git a/build/toolchain/win/BUILD.gn b/build/toolchain/win/BUILD.gn
index 4d9d1f45f870..8e4a30b223ba 100644
--- a/build/toolchain/win/BUILD.gn
+++ b/build/toolchain/win/BUILD.gn
@@ -109,7 +109,7 @@ template("msvc_toolchain") {
       # lld-link includes a replacement for lib.exe that can produce thin
       # archives and understands bitcode (for lto builds).
       lib = "$prefix/$lld_link /lib /llvmlibthin"
-      link = "$prefix/$lld_link"
+      link = "C:/src/l/ra/bin/lld-link.exe"
       if (host_os != "win") {
         # See comment adding --rsp-quoting to $cl above for more information.
         link = "$link --rsp-quoting=posix"

I built it with this batch file:

for /l %%x in (1, 1, 100) do (
  mkdir llvm_exp%%x
  cd llvm_exp%%x
  copy ..\args.gn .
  gn gen .
  ninja all > ninja.out
  cd ..
)
Wed, Jun 6, 12:27 PM
pcc added a comment to D47841: [asan] Instrument comdat globals on COFF targets.

there is no reason that we cannot also do this for ELF.

Wed, Jun 6, 12:12 PM
pcc added a comment to D47799: [COFF] add /errorrepro to save reproducer on error.

If the test failure is related to LTO, would this really help? As I understand it, the failures are on the object files created by LTO, which I don't think would appear in the linkrepro file.

Wed, Jun 6, 11:29 AM

Tue, Jun 5

pcc accepted D47803: Add definition for ELF dynamic tag DT_SYMTAB_SHNDX..

LGTM

Tue, Jun 5, 7:03 PM
pcc accepted D47602: Correct aligment computation for shared object symbols.

This LGTM if @ruiu is happy.

Tue, Jun 5, 4:07 PM
pcc accepted D47800: llvm-readobj: fix printing number of relocations in Android packed format..

LGTM

Tue, Jun 5, 2:40 PM
pcc added inline comments to D47602: Correct aligment computation for shared object symbols.
Tue, Jun 5, 2:06 PM
pcc added inline comments to D44788: Add an option to support debug fission on implicit ThinLTO..
Tue, Jun 5, 11:45 AM
pcc added inline comments to D46653: Start support for linking object files with split stacks.
Tue, Jun 5, 11:44 AM
pcc added inline comments to D47602: Correct aligment computation for shared object symbols.
Tue, Jun 5, 11:25 AM

Mon, Jun 4

pcc added inline comments to D47399: Add _LIBCPP_LARGE_CODEBASE.
Mon, Jun 4, 6:09 PM
pcc accepted D46700: [ThinLTO] Add testing of new summary index format to a couple CFI tests.

LGTM

Mon, Jun 4, 2:33 PM
pcc updated the diff for D47744: MC: Implement support for new .addrsig and .addrsig_sym directives..
  • Add a dwo test
Mon, Jun 4, 1:44 PM
pcc created D47744: MC: Implement support for new .addrsig and .addrsig_sym directives..
Mon, Jun 4, 1:43 PM
pcc created D47743: MC: Don't emit the .llvm.call-graph-profile section into dwo files..
Mon, Jun 4, 1:36 PM
pcc accepted D47731: Fix for llvm-dis/llvm-bcanalyzer overflows.

LGTM

Mon, Jun 4, 12:18 PM

Fri, Jun 1

pcc accepted D44965: [llvm][Instrumentation/MC] Add Call Graph Profile pass and object file emission..

LGTM

Fri, Jun 1, 7:16 PM
pcc added inline comments to D47602: Correct aligment computation for shared object symbols.
Fri, Jun 1, 3:05 PM
pcc committed rL333793: ELF: Ignore argument after --plugin..
ELF: Ignore argument after --plugin.
Fri, Jun 1, 2:55 PM
pcc committed rLLD333793: ELF: Ignore argument after --plugin..
ELF: Ignore argument after --plugin.
Fri, Jun 1, 2:55 PM
pcc closed D47657: ELF: Ignore argument after --plugin..
Fri, Jun 1, 2:55 PM
pcc updated the diff for D47657: ELF: Ignore argument after --plugin..
  • Add test
Fri, Jun 1, 2:49 PM
pcc accepted D47656: Show choices of multiple-choice options in `ld.lld --help` message..

LGTM

Fri, Jun 1, 2:46 PM
pcc created D47657: ELF: Ignore argument after --plugin..
Fri, Jun 1, 2:43 PM
pcc accepted D47605: Add weak definitions of trace-cmp hooks to dfsan.

LGTM

Fri, Jun 1, 2:30 PM
pcc added inline comments to D47602: Correct aligment computation for shared object symbols.
Fri, Jun 1, 1:58 PM
pcc abandoned D47648: ELF: Don't query a shared symbol's alignment unless necessary..

There are other cases as well: http://llvm-cs.pcc.me.uk/tools/lld/ELF/Symbols.h/rgetFile

Fri, Jun 1, 1:47 PM
pcc added a comment to D47648: ELF: Don't query a shared symbol's alignment unless necessary..

That certainly makes sense for Defined, but maybe not so much for SharedSymbol, at least when we don't have any code which actually creates fake shared symbols. You'll note that there are already dependencies on there being an actual .so file, e.g. with VerdefIndex, and I reckon that it's probably better to depend on the .so fully rather than being in a half dependent state.

Fri, Jun 1, 1:31 PM
pcc accepted D46326: ThinLTO+CFI: short-circuit direct calls to jump table entries.

LGTM

Fri, Jun 1, 12:29 PM
pcc created D47648: ELF: Don't query a shared symbol's alignment unless necessary..
Fri, Jun 1, 12:13 PM
pcc added inline comments to D47602: Correct aligment computation for shared object symbols.
Fri, Jun 1, 11:06 AM

Thu, May 31

pcc added inline comments to D47602: Correct aligment computation for shared object symbols.
Thu, May 31, 8:09 PM
pcc added inline comments to D47602: Correct aligment computation for shared object symbols.
Thu, May 31, 6:00 PM
pcc added inline comments to D47602: Correct aligment computation for shared object symbols.
Thu, May 31, 3:55 PM
pcc added a comment to D47602: Correct aligment computation for shared object symbols.

Thanks @ruiu .

@pcc do you agree with initializing Ret to uint64-1?

Thu, May 31, 2:39 PM
pcc added inline comments to D47602: Correct aligment computation for shared object symbols.
Thu, May 31, 2:13 PM
pcc accepted D47594: [ThinLTOBitcodeWriter] Emit summaries for regular LTO modules.

LGTM

Thu, May 31, 1:50 PM
pcc added inline comments to D47602: Correct aligment computation for shared object symbols.
Thu, May 31, 1:39 PM
pcc added inline comments to D47602: Correct aligment computation for shared object symbols.
Thu, May 31, 1:30 PM
pcc added a reviewer for D47399: Add _LIBCPP_LARGE_CODEBASE: mclow.lists.

Probably either mclow or EricWF.

Thu, May 31, 1:19 PM
pcc accepted D47588: Print out "Alias for -foo" instead of repeating the same help message for -foo..

LGTM

Thu, May 31, 11:41 AM
pcc committed rC333677: IRGen: Write .dwo files when -split-dwarf-file is used together with -fthinlto….
IRGen: Write .dwo files when -split-dwarf-file is used together with -fthinlto…
Thu, May 31, 11:31 AM
pcc committed rL333677: IRGen: Write .dwo files when -split-dwarf-file is used together with -fthinlto….
IRGen: Write .dwo files when -split-dwarf-file is used together with -fthinlto…
Thu, May 31, 11:31 AM
pcc closed D47597: IRGen: Write .dwo files when -split-dwarf-file is used together with -fthinlto-index..
Thu, May 31, 11:31 AM
pcc created D47597: IRGen: Write .dwo files when -split-dwarf-file is used together with -fthinlto-index..
Thu, May 31, 11:20 AM

Wed, May 30

pcc created D47567: Implement CFI for indirect calls via a member function pointer..
Wed, May 30, 7:44 PM
pcc accepted D47562: Add "(default)" to default options.

LGTM

Wed, May 30, 4:03 PM
pcc committed rL333602: IRGen: Rename bitsets -> type metadata. NFC..
IRGen: Rename bitsets -> type metadata. NFC.
Wed, May 30, 3:33 PM
pcc committed rC333602: IRGen: Rename bitsets -> type metadata. NFC..
IRGen: Rename bitsets -> type metadata. NFC.
Wed, May 30, 3:33 PM
pcc committed rC333600: AST: Remove an unused ctor. NFC..
AST: Remove an unused ctor. NFC.
Wed, May 30, 3:18 PM