pcc (Peter Collingbourne)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 28 2012, 2:34 PM (255 w, 11 h)

Recent Activity

Yesterday

pcc accepted D40203: [asan] Use dynamic shadow on 32-bit Android, try 2..

LGTM

Fri, Nov 17, 6:23 PM
pcc added a comment to D40203: [asan] Use dynamic shadow on 32-bit Android, try 2..

Could you show me a diff against your revert of r318575?

Fri, Nov 17, 5:57 PM
pcc committed rL318548: COFF: Stop emitting a non-standard COFF symbol table into PEs..
COFF: Stop emitting a non-standard COFF symbol table into PEs.
Fri, Nov 17, 11:51 AM
pcc closed D40189: COFF: Stop emitting a non-standard COFF symbol table into PEs. by committing rL318548: COFF: Stop emitting a non-standard COFF symbol table into PEs..
Fri, Nov 17, 11:51 AM
pcc committed rL318546: Enable PDB generation with lld in asan and cfi tests on Windows..
Enable PDB generation with lld in asan and cfi tests on Windows.
Fri, Nov 17, 11:50 AM
pcc closed D40188: Enable PDB generation with lld in asan and cfi tests on Windows. by committing rL318546: Enable PDB generation with lld in asan and cfi tests on Windows..
Fri, Nov 17, 11:49 AM
pcc updated the diff for D40189: COFF: Stop emitting a non-standard COFF symbol table into PEs..
  • Remove some debugging code
Fri, Nov 17, 11:25 AM
pcc added a dependent revision for D40188: Enable PDB generation with lld in asan and cfi tests on Windows.: D40189: COFF: Stop emitting a non-standard COFF symbol table into PEs..
Fri, Nov 17, 11:23 AM
pcc created D40189: COFF: Stop emitting a non-standard COFF symbol table into PEs..
Fri, Nov 17, 11:23 AM
pcc created D40188: Enable PDB generation with lld in asan and cfi tests on Windows..
Fri, Nov 17, 11:22 AM

Thu, Nov 16

pcc accepted D39864: Fix for CFI type tests lowering assert. .

Assuming that you've made the obvious change to the set type locally, this LGTM.

Thu, Nov 16, 2:02 PM
pcc added inline comments to D39864: Fix for CFI type tests lowering assert. .
Thu, Nov 16, 11:15 AM
pcc added a comment to D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.

Adding pcc as the original author of D28272.

Peter, in https://reviews.llvm.org/D28272#637284 you mention that you'd add a follow up patch that used .data.rel.ro rather than .bss.rel.ro for the copy relocations. I can't work out if that ever landed. Can you let me know if did? I've just ran into a case where the assumptions made by a ld.bfd linker script cause problems with the .bss.rel.ro name.

Thu, Nov 16, 9:47 AM

Wed, Nov 15

pcc accepted D40116: [asan] Fallback to non-ifunc dynamic shadow on android<22..

LGTM

Wed, Nov 15, 6:47 PM
pcc added a comment to D40102: [coff] correctly emit safeseh entries for handlers defined in dlls.

No yaml support for import libraries, I take it?

Looks that way. There are also a couple of other .libs in the Inputs directory, so I just did the same thing.

Wed, Nov 15, 5:31 PM
pcc accepted D40094: LTO: clarify why we need to gracefully handle sys::fs::rename failures.

LGTM, thanks.

Wed, Nov 15, 4:05 PM
pcc added a comment to D40014: [LLD] [COFF] Improve the autoexport check for symbols from import libraries with -opt:noref.

I didn't think that the pe format allowed absolute symbols to be exported.

Wed, Nov 15, 3:04 PM
pcc added inline comments to D40094: LTO: clarify why we need to gracefully handle sys::fs::rename failures.
Wed, Nov 15, 12:03 PM

Tue, Nov 14

pcc accepted D40004: [XRay][compiler-rt][x86_64] Align the stack before and after calling handlers.

LGTM

Tue, Nov 14, 6:39 PM
pcc accepted D40048: [asan] Prevent rematerialization of &__asan_shadow..

LGTM

Tue, Nov 14, 2:32 PM
pcc accepted D39819: [cfi-verify] Add DOT graph printing for GraphResult objects..

LGTM

Tue, Nov 14, 2:32 PM
pcc added inline comments to D40048: [asan] Prevent rematerialization of &__asan_shadow..
Tue, Nov 14, 1:37 PM

Mon, Nov 13

pcc updated subscribers of D39993: Remove dead code.

Unfortunately, we were still seeing occasional permission denied errors on our bots even with that change.
e.g. https://build.chromium.org/p/chromium.clang/builders/ToTWinThinLTO64/builds/101
So this code was added as part of D39874.

Mon, Nov 13, 6:02 PM
pcc added inline comments to D39819: [cfi-verify] Add DOT graph printing for GraphResult objects..
Mon, Nov 13, 1:01 PM

Fri, Nov 10

pcc added inline comments to D39819: [cfi-verify] Add DOT graph printing for GraphResult objects..
Fri, Nov 10, 4:44 PM
pcc added inline comments to D39819: [cfi-verify] Add DOT graph printing for GraphResult objects..
Fri, Nov 10, 4:25 PM
pcc accepted D39885: Disable GC and ICF when /debug is present.

LGTM, thanks.

Fri, Nov 10, 2:50 PM
pcc committed rL317930: sanitizer_common: Try looking up symbols with RTLD_DEFAULT if RTLD_NEXT does….
sanitizer_common: Try looking up symbols with RTLD_DEFAULT if RTLD_NEXT does…
Fri, Nov 10, 2:10 PM
pcc closed D39779: sanitizer_common: Try looking up symbols with RTLD_DEFAULT if RTLD_NEXT does not work. by committing rL317930: sanitizer_common: Try looking up symbols with RTLD_DEFAULT if RTLD_NEXT does….
Fri, Nov 10, 2:09 PM
pcc accepted D39393: [asan] Use dynamic shadow on 32-bit Android..

LGTM

Fri, Nov 10, 2:00 PM
pcc accepted D39393: [asan] Use dynamic shadow on 32-bit Android..

Can you add a test that uses the -asan-with-ifunc flag?

Fri, Nov 10, 1:17 PM
pcc added a comment to D39885: Disable GC and ICF when /debug is present.

For the MinGW frontend, we decided to pass -debug:dwarf by default, unless the caller passed -s. Doesn't this change mean that a user that links without -s and then manually strips the binary later would end up with a larger and less optimized binary than if you would have linked it with -s in the first place?

Fri, Nov 10, 12:32 PM
pcc added a comment to D39819: [cfi-verify] Add DOT graph printing for GraphResult objects..

Test case?

Fri, Nov 10, 11:05 AM

Thu, Nov 9

pcc added a comment to D39885: Disable GC and ICF when /debug is present.

somewhat expensive

Do you have numbers?

hurts debuggability

According to https://msdn.microsoft.com/en-us/library/bxwfs976.aspx link.exe disables ICF by default when linking with /debug. We could do the same.

technically non-conforming.

True, but from a practical perspective, programs that have been linked with link.exe are unlikely to break as a result of the default.

disabling /OPT:REF by default

We might actually consider doing that in /debug builds because it should provide a better debugging experience.

Thu, Nov 9, 7:36 PM
pcc accepted D39874: LTO: don't fatal when value for cache key already exists.

LGTM

Thu, Nov 9, 6:04 PM
pcc added inline comments to D39874: LTO: don't fatal when value for cache key already exists.
Thu, Nov 9, 4:09 PM

Wed, Nov 8

pcc committed rL317757: ubsan: Allow programs to use setenv to configure ubsan_standalone..
ubsan: Allow programs to use setenv to configure ubsan_standalone.
Wed, Nov 8, 6:22 PM
pcc closed D39827: ubsan: Allow programs to use setenv to configure ubsan_standalone. by committing rL317757: ubsan: Allow programs to use setenv to configure ubsan_standalone..
Wed, Nov 8, 6:22 PM
pcc added a comment to D39827: ubsan: Allow programs to use setenv to configure ubsan_standalone..

we have interceptors for nicer support of deadly signals, so probably we'd like to install them ASAP

Wed, Nov 8, 6:09 PM
pcc created D39827: ubsan: Allow programs to use setenv to configure ubsan_standalone..
Wed, Nov 8, 5:40 PM
pcc updated the diff for D39779: sanitizer_common: Try looking up symbols with RTLD_DEFAULT if RTLD_NEXT does not work..
  • Fix test case
Wed, Nov 8, 4:30 PM
pcc accepted D39565: [ThinLTO] Ensure sanitizer passes are run.

LGTM

Wed, Nov 8, 9:47 AM
pcc added a comment to D39566: [ThinLTO] Ensure sanitizer passes are run.

I think this test would need to check that LTO is supported (see e.g. D39508).

Wed, Nov 8, 9:47 AM

Tue, Nov 7

pcc added a comment to D37993: [ThinLTO/gold] Implement ThinLTO cache pruning support.
In D37993#912428, @pcc wrote:
In D37993#893466, @pcc wrote:

Is it a good idea to support cache pruning in the gold plugin? As I mentioned in D31063 there are some unavoidable race conditions due to limitations in the plugin API.

Ping... not sure whether it's a good idea to ship a known racy feature.

If you really want this feature in gold I'd suggest extending the gold plugin API so that it accepts file descriptors, and then have the plugin support this feature conditionally on whether the gold version is new enough.

Hi Peter, thanks for pointing this out, and sorry for the delay in responding. Wondering whether instead of removing cache pruning support from gold-plugin for now, we could do something else: write the MemoryBuffer to another temp file and use that path (setting IsTemporary to true so it gets cleaned up). This would obviously require more space on disk, but maybe worth it to enable the pruning safely.

Tue, Nov 7, 8:39 PM
pcc added a comment to D39779: sanitizer_common: Try looking up symbols with RTLD_DEFAULT if RTLD_NEXT does not work..

I cannot run the test on Android myself, so it may require adjustment.

Tue, Nov 7, 6:51 PM
pcc created D39779: sanitizer_common: Try looking up symbols with RTLD_DEFAULT if RTLD_NEXT does not work..
Tue, Nov 7, 6:50 PM

Mon, Nov 6

pcc added inline comments to D39393: [asan] Use dynamic shadow on 32-bit Android..
Mon, Nov 6, 4:26 PM
pcc accepted D39695: [IPO/LowerTypesTest] Skip blockaddress when replacing uses.

LGTM

Mon, Nov 6, 4:07 PM
pcc added inline comments to D39393: [asan] Use dynamic shadow on 32-bit Android..
Mon, Nov 6, 3:47 PM
pcc added inline comments to D39695: [IPO/LowerTypesTest] Skip blockaddress when replacing uses.
Mon, Nov 6, 3:06 PM

Sun, Nov 5

pcc committed rL317450: ELF: Remove SymbolTable::SymIndex class..
ELF: Remove SymbolTable::SymIndex class.
Sun, Nov 5, 8:58 PM
pcc closed D39672: ELF: Remove SymbolTable::SymIndex class. by committing rL317450: ELF: Remove SymbolTable::SymIndex class..
Sun, Nov 5, 8:58 PM
pcc created D39672: ELF: Remove SymbolTable::SymIndex class..
Sun, Nov 5, 8:40 PM
pcc committed rL317449: ELF: Remove function Symbol::isInCurrentOutput()..
ELF: Remove function Symbol::isInCurrentOutput().
Sun, Nov 5, 8:39 PM
pcc committed rL317448: ELF: Merge DefinedRegular and Defined..
ELF: Merge DefinedRegular and Defined.
Sun, Nov 5, 8:36 PM
pcc closed D39667: ELF: Merge DefinedRegular and Defined. by committing rL317448: ELF: Merge DefinedRegular and Defined..
Sun, Nov 5, 8:36 PM
pcc committed rL317447: ELF: Remove DefinedCommon..
ELF: Remove DefinedCommon.
Sun, Nov 5, 8:34 PM
pcc closed D39666: ELF: Remove DefinedCommon. by committing rL317447: ELF: Remove DefinedCommon..
Sun, Nov 5, 8:34 PM
pcc added inline comments to D39667: ELF: Merge DefinedRegular and Defined..
Sun, Nov 5, 8:31 PM
pcc added inline comments to D39666: ELF: Remove DefinedCommon..
Sun, Nov 5, 8:29 PM
pcc accepted D39394: Do not consider Shared symbols as defined symbols..

This change makes sense to me as it should allow for further simplification of the class hierarchy.

Sun, Nov 5, 2:03 PM
pcc created D39667: ELF: Merge DefinedRegular and Defined..
Sun, Nov 5, 2:02 PM
pcc added a dependent revision for D39666: ELF: Remove DefinedCommon.: D39667: ELF: Merge DefinedRegular and Defined..
Sun, Nov 5, 2:02 PM
pcc created D39666: ELF: Remove DefinedCommon..
Sun, Nov 5, 2:02 PM
pcc added a dependent revision for D39394: Do not consider Shared symbols as defined symbols.: D39666: ELF: Remove DefinedCommon..
Sun, Nov 5, 2:02 PM

Fri, Nov 3

pcc committed rL317371: Revert r317046, "Object: Move some code from ELF.h into ELF.cpp.".
Revert r317046, "Object: Move some code from ELF.h into ELF.cpp."
Fri, Nov 3, 2:30 PM
pcc accepted D35702: [LTO][ThinLTO] Use the linker resolutions to mark global values as dso_local..

LGTM

Fri, Nov 3, 12:53 PM
pcc added inline comments to D38658: [cfi-verify] Add an interesting unit test where undef search length changes result..
Fri, Nov 3, 11:54 AM
pcc accepted D38658: [cfi-verify] Add an interesting unit test where undef search length changes result..

LGTM

Fri, Nov 3, 11:10 AM

Thu, Nov 2

pcc added inline comments to D35702: [LTO][ThinLTO] Use the linker resolutions to mark global values as dso_local..
Thu, Nov 2, 11:21 AM

Wed, Nov 1

pcc accepted D38657: Update cl::opt<uint64_t> instances to cl::opt<unsigned long long>.

LGTM, I guess.

Wed, Nov 1, 4:35 PM
pcc committed rL317108: LTO: Apply global DCE to ThinLTO modules at LTO opt level 0..
LTO: Apply global DCE to ThinLTO modules at LTO opt level 0.
Wed, Nov 1, 10:59 AM
pcc closed D39484: LTO: Apply global DCE to ThinLTO modules at LTO opt level 0. by committing rL317108: LTO: Apply global DCE to ThinLTO modules at LTO opt level 0..
Wed, Nov 1, 10:59 AM
pcc added inline comments to D39484: LTO: Apply global DCE to ThinLTO modules at LTO opt level 0..
Wed, Nov 1, 10:47 AM
pcc updated the diff for D39484: LTO: Apply global DCE to ThinLTO modules at LTO opt level 0..
  • Address review comments
Wed, Nov 1, 10:47 AM

Tue, Oct 31

pcc added a comment to D39485: Extend SpecialCaseList to allow users to blame matches on entries in the file..

I'd be curious about what sort of perf impact (positive or negative) comes from splitting the special case list into multiple regexes, as this part of the compiler has historically been perf sensitive. It may be that the trigram filter (which is relatively new) makes the impact relatively insignificant.

Tue, Oct 31, 6:14 PM
pcc added a comment to D37993: [ThinLTO/gold] Implement ThinLTO cache pruning support.
In D37993#893466, @pcc wrote:

Is it a good idea to support cache pruning in the gold plugin? As I mentioned in D31063 there are some unavoidable race conditions due to limitations in the plugin API.

Tue, Oct 31, 5:18 PM
pcc created D39484: LTO: Apply global DCE to ThinLTO modules at LTO opt level 0..
Tue, Oct 31, 5:11 PM
pcc committed rL317046: Object: Move some code from ELF.h into ELF.cpp..
Object: Move some code from ELF.h into ELF.cpp.
Tue, Oct 31, 3:49 PM
pcc closed D39271: Object: Move some code from ELF.h into ELF.cpp. by committing rL317046: Object: Move some code from ELF.h into ELF.cpp..
Tue, Oct 31, 3:49 PM
pcc committed rL317045: Inline compareAddr function into its only caller. NFCI..
Inline compareAddr function into its only caller. NFCI.
Tue, Oct 31, 3:49 PM
pcc accepted D38654: Parse DWARF information to reduce false positives..

LGTM

Tue, Oct 31, 3:43 PM
pcc added a comment to D38654: Parse DWARF information to reduce false positives..

I'd expect there to be a test which makes sure that a jump with no line info in a file with line info doesn't count as an unprotected jump. Can you add one?

Tue, Oct 31, 11:31 AM
pcc added inline comments to D38654: Parse DWARF information to reduce false positives..
Tue, Oct 31, 10:55 AM
pcc accepted D39458: Do not access beyond the end of local symbols..

LGTM

Tue, Oct 31, 10:16 AM

Mon, Oct 30

pcc accepted D39443: [ThinLTO] Double bits of module hash used for renaming.

LGTM

Mon, Oct 30, 7:25 PM
pcc accepted D39358: [CFI] Add CFI-icall pointer type generalization.

LGTM

Mon, Oct 30, 5:56 PM
pcc added inline comments to D39358: [CFI] Add CFI-icall pointer type generalization.
Mon, Oct 30, 4:36 PM
pcc added a comment to D39358: [CFI] Add CFI-icall pointer type generalization.

That's the size of the object files, not the size of the final executable, right?

Mon, Oct 30, 1:28 PM

Sun, Oct 29

pcc committed rL316877: ELF: Correctly set edata if there are no .bss sections..
ELF: Correctly set edata if there are no .bss sections.
Sun, Oct 29, 3:32 PM
pcc closed D39399: ELF: Correctly set edata if there are no .bss sections. by committing rL316877: ELF: Correctly set edata if there are no .bss sections..
Sun, Oct 29, 3:32 PM
pcc accepted D39406: Merge SymbolBody and Symbol into one class, SymbolBody..
In D39406#910260, @ruiu wrote:

The size of the symbol remains the same 88 bytes.

Sun, Oct 29, 2:44 PM
pcc added a comment to D39406: Merge SymbolBody and Symbol into one class, SymbolBody..

I think this change makes sense at least from a stylistic perspective. The original idea of the split was to separate symbol properties from symbol name properties, but that created other issues like where to put member functions.

Sun, Oct 29, 12:10 PM

Fri, Oct 27

pcc created D39399: ELF: Correctly set edata if there are no .bss sections..
Fri, Oct 27, 9:14 PM
pcc accepted D39380: Fix llvm-special-case-list-fuzzer regexp exception.

Should this fix also be sent upstream?

Fri, Oct 27, 12:10 PM
pcc committed rL316775: ELF: Add support for emitting dynamic relocations in the Android relocation….
ELF: Add support for emitting dynamic relocations in the Android relocation…
Fri, Oct 27, 10:50 AM
pcc closed D39152: ELF: Add support for emitting dynamic relocations in the Android relocation packing format. by committing rL316775: ELF: Add support for emitting dynamic relocations in the Android relocation….
Fri, Oct 27, 10:50 AM

Thu, Oct 26

pcc updated the diff for D39152: ELF: Add support for emitting dynamic relocations in the Android relocation packing format..
  • Simplify
Thu, Oct 26, 3:34 PM
pcc added a comment to D38654: Parse DWARF information to reduce false positives..

Should this include a test case?

Thu, Oct 26, 1:27 PM

Wed, Oct 25

pcc updated the diff for D39152: ELF: Add support for emitting dynamic relocations in the Android relocation packing format..
  • Add commas
Wed, Oct 25, 7:54 PM