pcc (Peter Collingbourne)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Yesterday

pcc accepted D50372: Introduce the VTable interleaving scheme to the CFI design documentation.

LGTM except for a few spelling/grammar nits.

Wed, Aug 15, 3:51 PM
pcc added a comment to D50754: Implementation of a vtable interleaving algorithm.

Please make sure that your patch is formatted to match the LLVM style. clang-format can do that for you.

Wed, Aug 15, 3:22 PM
pcc added a comment to D50803: [gold-plugin] Add "obj-files" option.

obj-path should produce the same order as save-temps because they use the same code path -- they both work by setting Filename to a non-empty string, which causes us to follow the code path that appends task identifiers and use non-temporary files. If you're seeing something different, maybe that should be fixed instead?

Wed, Aug 15, 2:56 PM
pcc added a comment to D50803: [gold-plugin] Add "obj-files" option.

Doesn't the obj-path option already do what you want here?

Wed, Aug 15, 1:31 PM
pcc committed rCRT339800: cfi: Remove blacklist entries for libc++..
cfi: Remove blacklist entries for libc++.
Wed, Aug 15, 11:07 AM
pcc committed rL339800: cfi: Remove blacklist entries for libc++..
cfi: Remove blacklist entries for libc++.
Wed, Aug 15, 11:06 AM
pcc committed rL339799: llvm-readobj: Fix addend in relocations for android packed format.
llvm-readobj: Fix addend in relocations for android packed format
Wed, Aug 15, 10:59 AM
pcc closed D50601: llvm-readobj: Fix addend in relocations for android packed format.
Wed, Aug 15, 10:59 AM
pcc committed rL339797: libcxx: Mark __temp_value::__temp_value as _LIBCPP_NO_CFI..
libcxx: Mark __temp_value::__temp_value as _LIBCPP_NO_CFI.
Wed, Aug 15, 10:50 AM
pcc committed rCXX339797: libcxx: Mark __temp_value::__temp_value as _LIBCPP_NO_CFI..
libcxx: Mark __temp_value::__temp_value as _LIBCPP_NO_CFI.
Wed, Aug 15, 10:50 AM
pcc closed D50743: libcxx: Mark __temp_value::__temp_value as _LIBCPP_NO_CFI..
Wed, Aug 15, 10:50 AM
pcc added a comment to D44501: COFF: Move assignment of section offsets to assignAddresses(). NFCI..

This makes the design a little more similar to the ELF linker and should allow for features such as ARM range extension thunks to be implemented more easily.

@pcc - I found you mentioned this as part of this change; do you have any concrete plan on how to add ARM range extension thunks? I'm pondering giving that a try, but if you have ideas about how/where to start, that'd get me started quicker.

Have you looked at their design in the ELF port? CC @peter.smith, who added that support.

Wed, Aug 15, 10:47 AM

Tue, Aug 14

pcc created D50743: libcxx: Mark __temp_value::__temp_value as _LIBCPP_NO_CFI..
Tue, Aug 14, 4:05 PM
pcc added a comment to D50204: [llvm-objdump] Label calls to the PLT.

Please add a test with a 32-bit non-PIC executable.

Tue, Aug 14, 2:25 PM
pcc added inline comments to D50203: Find PLT entries for x86, x86_64, and AArch64.
Tue, Aug 14, 2:25 PM
pcc accepted D50724: SafeStack: Disable Darwin support.

LGTM

Tue, Aug 14, 12:29 PM
pcc accepted D50718: SafeStack: Disable Darwin support.

(@kubamracek FYI)

Tue, Aug 14, 10:14 AM

Mon, Aug 13

pcc accepted D50601: llvm-readobj: Fix addend in relocations for android packed format.

Do you have commit access?

Mon, Aug 13, 9:51 PM
pcc added inline comments to D50203: Find PLT entries for x86, x86_64, and AArch64.
Mon, Aug 13, 5:29 PM
pcc added inline comments to D50601: llvm-readobj: Fix addend in relocations for android packed format.
Mon, Aug 13, 4:15 PM
pcc accepted D49959: [ThinLTO] Fix printing of WPD remarks.

LGTM

Mon, Aug 13, 3:53 PM
pcc accepted D49960: [ThinLTO] Handle optional args in assembly format for ConstVCalls.

LGTM

Mon, Aug 13, 3:51 PM

Fri, Aug 10

pcc added inline comments to rL316543: llvm-readobj: Add support for reading relocations in the Android packed format..
Fri, Aug 10, 7:17 PM
pcc added inline comments to D50569: Change how we handle -wrap..
Fri, Aug 10, 11:05 AM
pcc added inline comments to D50569: Change how we handle -wrap..
Fri, Aug 10, 10:59 AM
pcc added inline comments to D50016: IR: Add entry/exit instrumentation symbols to the libcall list..
Fri, Aug 10, 9:41 AM

Wed, Aug 8

pcc committed rLLD339301: ELF: Only add libcall symbols to the link if defined in bitcode..
ELF: Only add libcall symbols to the link if defined in bitcode.
Wed, Aug 8, 4:49 PM
pcc committed rL339301: ELF: Only add libcall symbols to the link if defined in bitcode..
ELF: Only add libcall symbols to the link if defined in bitcode.
Wed, Aug 8, 4:49 PM
pcc closed D50475: ELF: Only add libcall symbols to the link if defined in bitcode..
Wed, Aug 8, 4:48 PM
pcc added inline comments to D50475: ELF: Only add libcall symbols to the link if defined in bitcode..
Wed, Aug 8, 4:42 PM
pcc added a comment to D50475: ELF: Only add libcall symbols to the link if defined in bitcode..

You don't need to call fetch again to add a file that you already have to the symbol table. You can directly add it by calling SymbolTable::addFile().

Wed, Aug 8, 2:54 PM
pcc added inline comments to D50475: ELF: Only add libcall symbols to the link if defined in bitcode..
Wed, Aug 8, 2:42 PM
pcc updated the diff for D50475: ELF: Only add libcall symbols to the link if defined in bitcode..
  • Add a LazyObject test
Wed, Aug 8, 2:02 PM
pcc created D50475: ELF: Only add libcall symbols to the link if defined in bitcode..
Wed, Aug 8, 2:00 PM
pcc created D50470: ELF: Inline function LazyObjFile::getBuffer() into caller. NFCI..
Wed, Aug 8, 1:22 PM

Mon, Aug 6

pcc added a comment to D50372: Introduce the VTable interleaving scheme to the CFI design documentation.

Please upload patches with context. arc diff will do this for you.

Mon, Aug 6, 7:36 PM
pcc committed rL339066: MC: Redirect .addrsig directives referring to private (.L) symbols to the….
MC: Redirect .addrsig directives referring to private (.L) symbols to the…
Mon, Aug 6, 3:00 PM
pcc committed rLLD339050: ELF: Enable address-significance tables during LTO..
ELF: Enable address-significance tables during LTO.
Mon, Aug 6, 1:12 PM
pcc committed rL339050: ELF: Enable address-significance tables during LTO..
ELF: Enable address-significance tables during LTO.
Mon, Aug 6, 1:12 PM
pcc closed D50221: ELF: Enable address-significance tables during LTO..
Mon, Aug 6, 1:12 PM
pcc closed D50221: ELF: Enable address-significance tables during LTO..
Mon, Aug 6, 1:12 PM
pcc added a comment to D50174: [LLD][ELF] - Remove dead code from ArchiveFile..

The issue is that if two object files depend on one another, we can end up in a situation where SymbolTable::fetchLazy is called recursively multiple times on the same object file, so we need to make sure to do nothing on the second call. Reproducer:

$ cat a.s
.globl a, b
a:
$ cat b.s
.globl a, b
b:
$ as -o a.o a.s
$ as -o b.o b.s
$ ar cru foo.a a.o b.o
$ ld.lld -eb foo.a -m elf_x86_64

With your patch I get:

ld.lld: error: duplicate symbol: b
>>> defined at b.o:(.text+0x0) in archive foo.a
>>> defined at b.o:(.text+0x0) in archive foo.a
Mon, Aug 6, 1:05 PM
pcc added a comment to D50221: ELF: Enable address-significance tables during LTO..

Ping

Mon, Aug 6, 12:19 PM

Fri, Aug 3

pcc added a comment to D50284: [COFF, ARM64] Recognize the .hidden directive.

I don't think the .hidden directive is meaningful in COFF, is it?

Fri, Aug 3, 5:06 PM
pcc added inline comments to D50204: [llvm-objdump] Label calls to the PLT.
Fri, Aug 3, 10:57 AM
pcc added inline comments to D50204: [llvm-objdump] Label calls to the PLT.
Fri, Aug 3, 10:17 AM

Thu, Aug 2

pcc created D50221: ELF: Enable address-significance tables during LTO..
Thu, Aug 2, 8:49 PM
pcc accepted D50196: Use the same constants as zlib to represent compression level..

LGTM

Thu, Aug 2, 12:03 PM
pcc added a comment to D50126: [Support] fix TempFile infinite loop and permission denied errors.

Looks great, thanks.

Thu, Aug 2, 11:22 AM
pcc added inline comments to D50126: [Support] fix TempFile infinite loop and permission denied errors.
Thu, Aug 2, 10:57 AM
pcc requested changes to D50126: [Support] fix TempFile infinite loop and permission denied errors.

No, I have comments.

Thu, Aug 2, 10:38 AM

Tue, Jul 31

pcc accepted D50116: [X86] FastISel fall back on !absolute_symbol GVs.

LGTM

Tue, Jul 31, 4:43 PM
pcc committed rLLD338434: ELF: Add libcall symbols to the link when LTO is being used..
ELF: Add libcall symbols to the link when LTO is being used.
Tue, Jul 31, 1:36 PM
pcc committed rL338434: ELF: Add libcall symbols to the link when LTO is being used..
ELF: Add libcall symbols to the link when LTO is being used.
Tue, Jul 31, 1:36 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Tue, Jul 31, 1:36 PM
pcc added a comment to D50048: [Windows FS] Allow moving files in TempFile::keep.

But if we change the TempFile API so that it always creates the temporary in the same directory as the destination, that becomes a non-problem, no?

Tue, Jul 31, 12:16 PM
pcc added a comment to D50048: [Windows FS] Allow moving files in TempFile::keep.

Can't we change dsymutil to create its temporary file in the same directory as the destination?

Tue, Jul 31, 12:01 PM
pcc added a reviewer for D50016: IR: Add entry/exit instrumentation symbols to the libcall list.: dexonsmith.
Tue, Jul 31, 11:50 AM
pcc added a comment to D50017: ELF: Add libcall symbols to the link when LTO is being used..

I thought about that but decided against it for a few reasons:

  1. It seemed best to express what we want to happen directly. Putting the symbols in the object files seemed like a more round-about way of fixing the bug.
  2. The set of libcall symbols is a property of the code generator linked into lld, not the one that was used to create the bitcode file. That means that if we see an old bitcode file it would not be correct to use its libcall symbols.
  3. It would bloat every bitcode file with the same (potentially incorrect due to 2) content when that can be easily avoided.
Tue, Jul 31, 11:41 AM

Mon, Jul 30

pcc accepted D50023: Make ICF output deterministic..

I'd probably mention in the commit message that it's just the log output that's being made deterministic. As far as I know this shouldn't affect the contents of the output file.

Mon, Jul 30, 4:27 PM
pcc added a comment to D50017: ELF: Add libcall symbols to the link when LTO is being used..

I'm wondering if we have to keep the list of symbols that could be added as undefined symbols by LTO. If LTO adds new undefined symbol, we always have to resolve them whatever they are, no?

Mon, Jul 30, 4:14 PM
pcc added a comment to D50017: ELF: Add libcall symbols to the link when LTO is being used..

It might be a silly question, but are these functions (e.g. udivhi3 or mulxf3) are functions in static archives?

Mon, Jul 30, 3:38 PM
pcc created D50017: ELF: Add libcall symbols to the link when LTO is being used..
Mon, Jul 30, 2:56 PM
pcc added a dependent revision for D50016: IR: Add entry/exit instrumentation symbols to the libcall list.: D50017: ELF: Add libcall symbols to the link when LTO is being used..
Mon, Jul 30, 2:56 PM
pcc created D50016: IR: Add entry/exit instrumentation symbols to the libcall list..
Mon, Jul 30, 2:55 PM

Fri, Jul 27

pcc added inline comments to D49938: [ELF] Add missing options to ld.lld.1.
Fri, Jul 27, 6:31 PM
pcc added a comment to D49383: [cfi-verify] Support cross-DSO.

Thanks for the explanations! pcc, that example API was very helpful. I have the core part of it working, including integration with cfi-verify and objdump. I'll finish it up and put the patches up for review at some point.

Fri, Jul 27, 2:33 PM
pcc committed rLLD338153: Reland r338088, "ELF: Make --print-icf-sections output deterministic.".
Reland r338088, "ELF: Make --print-icf-sections output deterministic."
Fri, Jul 27, 12:11 PM
pcc committed rL338153: Reland r338088, "ELF: Make --print-icf-sections output deterministic.".
Reland r338088, "ELF: Make --print-icf-sections output deterministic."
Fri, Jul 27, 12:11 PM
pcc abandoned D49923: Support: xxHash64: Zero extend byte values loaded from the input data..

Oh, it looks like @MaskRay fixed this in a different way in rL338128.

Fri, Jul 27, 11:53 AM
pcc added a comment to D49923: Support: xxHash64: Zero extend byte values loaded from the input data..

It is a char pointer.

Fri, Jul 27, 11:28 AM
pcc created D49923: Support: xxHash64: Zero extend byte values loaded from the input data..
Fri, Jul 27, 11:14 AM

Thu, Jul 26

pcc accepted D49777: [LTO] Don't internalize declarations.

LGTM

Thu, Jul 26, 7:53 PM
pcc committed rLLD338088: ELF: Make --print-icf-sections output deterministic..
ELF: Make --print-icf-sections output deterministic.
Thu, Jul 26, 4:35 PM
pcc committed rL338088: ELF: Make --print-icf-sections output deterministic..
ELF: Make --print-icf-sections output deterministic.
Thu, Jul 26, 4:34 PM
pcc closed D49877: ELF: Make --print-icf-sections output deterministic..
Thu, Jul 26, 4:34 PM
pcc added a comment to D49877: ELF: Make --print-icf-sections output deterministic..

I suspect that we aren't gaining much from the flags/relocation count/section size because all of these properties typically depend on the section contents anyway, so I left them out.

Thu, Jul 26, 4:32 PM
pcc updated the diff for D49877: ELF: Make --print-icf-sections output deterministic..
  • Switch to xxhash
Thu, Jul 26, 4:26 PM
pcc added a comment to D49877: ELF: Make --print-icf-sections output deterministic..

I think we should use a deterministic hash function such as xxHash64 instead of hash_combine to fix this issue, as I believe it's easier and more robust.

Thu, Jul 26, 3:23 PM
pcc created D49877: ELF: Make --print-icf-sections output deterministic..
Thu, Jul 26, 3:12 PM
pcc added inline comments to D49777: [LTO] Don't internalize declarations.
Thu, Jul 26, 1:16 PM
pcc added inline comments to D49526: Updated llvm-proto-fuzzer to execute the compiled code.
Thu, Jul 26, 11:44 AM

Wed, Jul 25

pcc added a comment to D49383: [cfi-verify] Support cross-DSO.

By the state to be tracked, do you mean an index of .got.plt relocations by address?

Wed, Jul 25, 3:45 PM
pcc added a comment to D49383: [cfi-verify] Support cross-DSO.

I'm not sure that DataRefImpl would be a better API because you would need to keep track of a lot of state between getting the info for each PLT entry and that could get complicated. DataRefImpl is better suited for the thin wrappers around object file data structures where you're just enumerating the elements of an array so the between-element state is smaller.

Wed, Jul 25, 3:36 PM
pcc added a comment to D49383: [cfi-verify] Support cross-DSO.

And agreed with eugenis that if you can avoid using the disassembler in findPltEntries that might be simpler.

Wed, Jul 25, 3:32 PM
pcc added a comment to D49383: [cfi-verify] Support cross-DSO.
In D49383#1175738, @pcc wrote:

I agree with eugenis that the PLT symbolization should live in lib/Object. That would also help us implement PLT symbolization in llvm-objdump. I was imagining that there would be a function that would take an object file and return a list of (PLT entry address, symbol) pairs. That would seem to be sufficient for llvm-objdump as well as for this code.

I was discussing PLT symbolization offline with eugenis and did a little more investigation. It turns out that it shouldn't be that hard to symbolize the PLT, at least not on aarch64. All of bfd/gold/lld start their PLT entries with a straightforward ADRP/LDR sequence, which you can pretty straighforwardly map onto a GOT entry and then onto a symbol.

As I said initially, I did think putting this logic in core LLVM was the best way, but I still don't understand how to do it (other than the way I did). Do you have more details or a sample patch?

Wed, Jul 25, 3:27 PM
pcc committed rLLD337967: ELF: Do not ICF SHF_LINK_ORDER sections..
ELF: Do not ICF SHF_LINK_ORDER sections.
Wed, Jul 25, 2:41 PM
pcc committed rL337967: ELF: Do not ICF SHF_LINK_ORDER sections..
ELF: Do not ICF SHF_LINK_ORDER sections.
Wed, Jul 25, 2:41 PM
pcc closed D49716: ELF: Do not ICF SHF_LINK_ORDER sections..
Wed, Jul 25, 2:41 PM
pcc added a comment to D49716: ELF: Do not ICF SHF_LINK_ORDER sections..

Looks like we already have https://bugs.llvm.org/show_bug.cgi?id=36272 about this.

Wed, Jul 25, 2:32 PM
pcc added a comment to D49716: ELF: Do not ICF SHF_LINK_ORDER sections..

I'm assuming that no merging of the code sections is done if the SHF_LINK_ORDER sections are different.

Wed, Jul 25, 2:16 PM
pcc added a comment to D49383: [cfi-verify] Support cross-DSO.

I agree with eugenis that the PLT symbolization should live in lib/Object. That would also help us implement PLT symbolization in llvm-objdump. I was imagining that there would be a function that would take an object file and return a list of (PLT entry address, symbol) pairs. That would seem to be sufficient for llvm-objdump as well as for this code.

Wed, Jul 25, 1:34 PM

Tue, Jul 24

pcc added inline comments to D49383: [cfi-verify] Support cross-DSO.
Tue, Jul 24, 5:03 PM
pcc accepted D49764: Simplify ObjFile::createDefined()..

LGTM

Tue, Jul 24, 4:48 PM
pcc accepted D49762: Fix error messages for bad symbols..

LGTM

Tue, Jul 24, 3:48 PM
pcc added inline comments to D49764: Simplify ObjFile::createDefined()..
Tue, Jul 24, 3:44 PM
pcc added a comment to D49757: [COFF] Keep using associative comdats for xdata/pdata for MinGW.

I'd prefer if we did something like this rather than changing the linker. I see gcc's non-conformance with the coff spec as a bug and if there aren't any real users of gcc+lld I'd prefer us not to have workarounds for that case.

Tue, Jul 24, 3:25 PM
pcc committed rL337847: Put "built-in" function definitions in global Used list, for LTO. (fix bug….
Put "built-in" function definitions in global Used list, for LTO. (fix bug…
Tue, Jul 24, 12:35 PM
pcc closed D49434: Put "built-in" function definitions in global Used list, for LTO. (fix bug 34169).
Tue, Jul 24, 12:35 PM
pcc requested changes to D49745: Generate fundamental type info with weak linkage.

As I mentioned in https://bugs.llvm.org/show_bug.cgi?id=38281 I don't see a good reason to do this. Also extern_weak is not an appropriate linkage for definitions, only declarations.

Tue, Jul 24, 11:06 AM