Page MenuHomePhabricator

bd1976llvm (ben)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2017, 9:47 AM (112 w, 5 d)

Recent Activity

Jan 16 2019

bd1976llvm added a comment to D56437: Support blank flag in SHT_GROUP sections for ELF.

I strongly feel that we shouldn't implement this. This will tie groups and --gc-sections together when they are orthogonal features; e.g. COMDAT groups will *also* have to be considered by --gc-sections as a single unit. The ability to strip individual sections form COMDAT groups is an important feature. I will add similar comment to the abi list. I'm surprised that gold implements this behavior, back in 2017 gold and bfd-ld disagreed on this.

Jan 16 2019, 12:45 AM

Jan 10 2019

bd1976llvm added a comment to D56516: [SanitizerCoverage] Don't create comdat for interposable functions..

@bd1976llvm: So do you still think this is a compiler bug? It sounds like ELF provides no guarantees about which COMDAT is kept. Which would mean we need to keep the weak symbols from going in a COMDAT.

Jan 10 2019, 1:32 PM
bd1976llvm added a comment to D56516: [SanitizerCoverage] Don't create comdat for interposable functions..

Does weak-vs-strong count towards that "api"? Because that's the only difference in the case I'm seeing...

Jan 10 2019, 12:34 PM
bd1976llvm added a comment to D56437: Support blank flag in SHT_GROUP sections for ELF.

@bd1976llvm interesting. Unfortunately I failed to find this thread :-/

Jan 10 2019, 4:01 AM
bd1976llvm added a comment to D56516: [SanitizerCoverage] Don't create comdat for interposable functions..
In D56516#1351953, @pcc wrote:

Can you create a minimal reproducer?

If objcopy is complaining about sh_link = 0 I suspect that there's some issue where a global referenced via !associated is being dropped.

Jan 10 2019, 3:54 AM

Jan 9 2019

bd1976llvm added a comment to D56437: Support blank flag in SHT_GROUP sections for ELF.

The wording of the standard is unfortunate. This has been brought up and clarified (that COMDATs are the only legal groups currently) on the list previously.

Jan 9 2019, 7:43 AM
bd1976llvm added a comment to D53234: Don't remove COMDATs when internalizing global objects.

There is a relevant discussion about non-COMDAT groups here: https://reviews.llvm.org/D56437.

Jan 9 2019, 7:27 AM
bd1976llvm added a comment to D56437: Support blank flag in SHT_GROUP sections for ELF.

I agree with Rui. My understanding is that groups in ELF were designed specifically to support C++ templates and inline functions which is why COMDAT groups are the only allowed kind.
Interestingly, I have been having a somewhat related discussion on this review: https://reviews.llvm.org/D53234.
Could the annobin tool use SHF_LINK_ORDER instead?

Jan 9 2019, 7:26 AM

Dec 21 2018

bd1976llvm added a comment to D53234: Don't remove COMDATs when internalizing global objects.

We need to resolve this point of disagreement. IMO that optimization is only invalid in regular compilation. I think that for LTO this is a valid optimization. LTO has optimizations that would normally be invalid - for example the Internalize pass.

I think the issue here is that an LLVM COMDAT has two different purposes:

  1. De-duplicate section groups
  2. Ensure that section group members are kept/removed as a unit

    In ELF files, you can have non-COMDAT section groups, which only accomplish purpose 2.
Dec 21 2018, 2:13 AM

Nov 28 2018

bd1976llvm added a comment to D53234: Don't remove COMDATs when internalizing global objects.
  1. Retain the comdats in the output (this patch). Limits the optimization potential of LTO. !associated metadata sections don't need to be in a comdats.

I don't believe that this is the right way to think about this. This patch prevents LLVM from performing an invalid optimization - that is, selectively discarding global objects that the IR explicitly indicates should stay together.

Nov 28 2018, 5:12 AM

Nov 26 2018

bd1976llvm added a comment to D53234: Don't remove COMDATs when internalizing global objects.

Trying again... Phabricator didn't like the markup in my last comment attempt.

Nov 26 2018, 6:08 PM
bd1976llvm added a comment to D53234: Don't remove COMDATs when internalizing global objects.
Nov 26 2018, 6:04 PM

Nov 16 2018

bd1976llvm added a comment to D53234: Don't remove COMDATs when internalizing global objects.

Hi Araon,

Nov 16 2018, 2:36 AM

Oct 31 2018

bd1976llvm added a comment to D53242: [Inliner] Only remove functions with a COMDAT when it's safe to do so.

Hi Aron, I'm interested in reviewing this. Would you mind responding to my comments on https://reviews.llvm.org/D53234 first, as you have based the correctness of this on the correctness of that change?

Oct 31 2018, 10:37 AM
bd1976llvm added a comment to D53234: Don't remove COMDATs when internalizing global objects.

So perhaps this code needs to handle comdat containing an object with associated metadata specially (e.g. could just include in the ExternalComdats set)?

My concern is that the current behavior would still be incorrect for other uses of COMDATS - or at the very least, quite counterintuitive.
If I specify a COMDAT for two global objects, I would expect one of the following things to happen:

  1. Both objects appear in the final object, both still in the COMDAT.
  2. Neither object appears in the final object, and the COMDAT section does not exist (both objects were optimized out).

    Having only one of the two objects be present, and having it lack a COMDAT, seems very unexpected to me - especially considering that the COMDAT section can be read using external tools (e.g. readelf).
Oct 31 2018, 10:35 AM

Sep 5 2018

bd1976llvm added a comment to D51536: [LLD] Do not set alignment to entsize for mergable sections if entsize is not a power of 2.

Thanks for the comments everyone.

Sep 5 2018, 7:53 AM

Aug 31 2018

bd1976llvm updated the diff for D51536: [LLD] Do not set alignment to entsize for mergable sections if entsize is not a power of 2.

Added missing newline at end of test file

Aug 31 2018, 5:30 AM
bd1976llvm created D51536: [LLD] Do not set alignment to entsize for mergable sections if entsize is not a power of 2.
Aug 31 2018, 5:27 AM
bd1976llvm committed rL341207: [LLD] Add test missed from r341206. NFC..
[LLD] Add test missed from r341206. NFC.
Aug 31 2018, 5:10 AM
bd1976llvm committed rLLD341207: [LLD] Add test missed from r341206. NFC..
[LLD] Add test missed from r341206. NFC.
Aug 31 2018, 5:10 AM
bd1976llvm committed rLLD341206: [LLD] Check too large offsets into merge sections earlier.
[LLD] Check too large offsets into merge sections earlier
Aug 31 2018, 4:55 AM
bd1976llvm committed rL341206: [LLD] Check too large offsets into merge sections earlier.
[LLD] Check too large offsets into merge sections earlier
Aug 31 2018, 4:55 AM
bd1976llvm closed D51180: [LLD] Check for too large offsets into merge sections earlier to avoid DenseMap tombstone collision.
Aug 31 2018, 4:55 AM

Aug 23 2018

bd1976llvm retitled D51180: [LLD] Check for too large offsets into merge sections earlier to avoid DenseMap tombstone collision from [LLD] Check for too large offsets into merge sections earlier to avoid DenseMap tombstone collsiion to [LLD] Check for too large offsets into merge sections earlier to avoid DenseMap tombstone collision.
Aug 23 2018, 1:28 PM
bd1976llvm created D51180: [LLD] Check for too large offsets into merge sections earlier to avoid DenseMap tombstone collision.
Aug 23 2018, 1:21 PM

Aug 22 2018

bd1976llvm added a comment to D51027: [LLD][ELD] - Do not reject INFO output section type when used with a start address..

Thanks for the detailed reply Peter!

Aug 22 2018, 1:58 AM

Aug 21 2018

bd1976llvm added a comment to D50688: [LLD] Warn if non-alloc sections occur before alloc sections in linker scripts.

Thanks for the link! However, I'm not sure if this is really related. The issue there was that a non-alloc section should be placed into a program header explicitly via PHDRS commands, which might be a valid use case.

Aug 21 2018, 7:41 AM
bd1976llvm added a comment to D51027: [LLD][ELD] - Do not reject INFO output section type when used with a start address..

Also, note that we already support the following:

.stack address_expression (NOLOAD) :
{

but not the

.stack address_expression (INFO) :
{

yet. So I think it is worth to implement both for consistency and few users we have.

Aug 21 2018, 4:20 AM
bd1976llvm added a comment to D51027: [LLD][ELD] - Do not reject INFO output section type when used with a start address..

Hi George. Do we actually want this behaviour?

Aug 21 2018, 3:43 AM

Aug 20 2018

bd1976llvm added a comment to D50688: [LLD] Warn if non-alloc sections occur before alloc sections in linker scripts.

Hi. Sorry to be slow to comment on this.

Aug 20 2018, 5:55 PM

Aug 2 2018

bd1976llvm updated subscribers of D48577: [llvm-ar] Correct help text.

This wasn't committed in time for the 7.0 release but I wonder if rL338703 + rL338709 should be put on the release branch? The current output is slightly embarrassing.

Aug 2 2018, 10:57 AM
bd1976llvm committed rL338709: [llvm-ar] Fix help text test. NFC..
[llvm-ar] Fix help text test. NFC.
Aug 2 2018, 5:27 AM
bd1976llvm committed rL338703: [llvm-ar] Correct help text.
[llvm-ar] Correct help text
Aug 2 2018, 4:28 AM
bd1976llvm closed D48577: [llvm-ar] Correct help text.
Aug 2 2018, 4:28 AM

Jul 23 2018

bd1976llvm added a comment to D49622: ELF: Make sections with KeepUnique bit eligible for ICF..

Hi Peter,

Jul 23 2018, 5:17 AM
bd1976llvm added a comment to D48577: [llvm-ar] Correct help text.

ping!

Jul 23 2018, 4:32 AM

Jun 28 2018

bd1976llvm added inline comments to D48577: [llvm-ar] Correct help text.
Jun 28 2018, 5:21 AM
bd1976llvm updated the diff for D48577: [llvm-ar] Correct help text.

updated diff to address review comments

Jun 28 2018, 5:12 AM
bd1976llvm added a reviewer for D48577: [llvm-ar] Correct help text: Bigcheese.
Jun 28 2018, 4:29 AM

Jun 26 2018

bd1976llvm updated the diff for D48577: [llvm-ar] Correct help text.

Having thought about this I have decided to try to simplify the help text as much as possible to aid maintenance.

Jun 26 2018, 7:30 AM

Jun 25 2018

bd1976llvm created D48577: [llvm-ar] Correct help text.
Jun 25 2018, 6:27 PM

Jun 22 2018

bd1976llvm added a comment to D48298: [ELF] Uniquify --wrap list..

I would propose adding the ability to retrieve a unique filtered list of arguments.

for (StringRef Name : Args.getAllUniqueArgValues(OPT_wrap))
  Symtab->addSymbolWrap<ELFT>(Name);

Relevant for other options: --defsym, --trace-symbol, --undefined etc..

I am not sure it is useful for other options we have, but it perhaps can be a nice refactoring for this particular case.

For example, I am not sure we should use it for --defsym, as in theory it can have different values: --defsym=foo=1 ... --defsym=foo=2.
It is OK to process all of them one by one like we do now. The same applies for --trace-symbol and --undefined - there is no issue to process them one by one.
I would not overcomplicate the current code for these cases until it is proven to be a useful change.

Jun 22 2018, 2:22 AM

Jun 19 2018

bd1976llvm added a comment to D48298: [ELF] Uniquify --wrap list..

I would propose adding the ability to retrieve a unique filtered list of arguments.

Jun 19 2018, 2:25 AM

Jun 4 2018

bd1976llvm added a comment to D47588: Print out "Alias for -foo" instead of repeating the same help message for -foo..

Hi Rui,

Jun 4 2018, 7:25 AM

May 31 2018

bd1976llvm added a comment to D47540: [lld] Enable Visual Studio compatible output.

Hi Rui! Thanks for looking at the patch.

May 31 2018, 4:02 AM

Mar 27 2018

bd1976llvm added a comment to D44669: Use local symbols for creating .stack-size.

Why are extra symbols *in the assembly* a problem? We normally don't print assembly

Mar 27 2018, 11:55 AM
bd1976llvm added a comment to D44669: Use local symbols for creating .stack-size.

Hi Rafael,

Mar 27 2018, 9:16 AM
bd1976llvm added a comment to D44669: Use local symbols for creating .stack-size.

Hi Rafael. The SN-Linker has the following rule for metadata sections:

Mar 27 2018, 3:36 AM

Mar 26 2018

bd1976llvm added a comment to D44669: Use local symbols for creating .stack-size.

LGTM., as long as this doesn't cause the number of symbols to double up?

Mar 26 2018, 8:59 AM

Jan 19 2018

bd1976llvm added inline comments to D42267: [ThinLTO] Allow 0 to be a valid value for pruning interval for C LTO API..
Jan 19 2018, 5:03 AM

Dec 22 2017

bd1976llvm committed rL321376: [ThinLTO][CachePruning] explicitly disable pruning.
[ThinLTO][CachePruning] explicitly disable pruning
Dec 22 2017, 10:33 AM
bd1976llvm closed D41497: [ThinLTO][CachePruning] explicitly disable pruning.
Dec 22 2017, 10:33 AM
bd1976llvm added inline comments to D41497: [ThinLTO][CachePruning] explicitly disable pruning.
Dec 22 2017, 10:25 AM
bd1976llvm added inline comments to D41497: [ThinLTO][CachePruning] explicitly disable pruning.
Dec 22 2017, 9:58 AM
bd1976llvm added a comment to D41497: [ThinLTO][CachePruning] explicitly disable pruning.

Thanks this looks much nicer. Question below about the handling of the Optional though.

As an aside, it would be great to have similar handling in parseCachePruningPolicy(), which is used by gold-plugin and lld, and which gives an error when -1 is passed as the interval. But that doesn't need to be in this patch, just noting it here.

Dec 22 2017, 8:15 AM
bd1976llvm updated the diff for D41497: [ThinLTO][CachePruning] explicitly disable pruning.

Addressed review comments

Dec 22 2017, 3:41 AM

Dec 21 2017

bd1976llvm added a comment to D41279: [ThinLTO][C-API] Correct api comments.

I have reworked this now, see: https://reviews.llvm.org/D41497.

Dec 21 2017, 10:37 AM
bd1976llvm added inline comments to D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning.
Dec 21 2017, 10:35 AM
bd1976llvm added a comment to D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning.

If Teresa is fine with this, then I don't want to hold this up. However, I would recommend looking at other ways to implement this behavior in the long term.

Dec 21 2017, 8:59 AM
bd1976llvm created D41497: [ThinLTO][CachePruning] explicitly disable pruning.
Dec 21 2017, 8:52 AM

Dec 19 2017

bd1976llvm committed rL321078: [ThinLTO][C-API] Correct api comments.
[ThinLTO][C-API] Correct api comments
Dec 19 2017, 6:50 AM
bd1976llvm closed D41279: [ThinLTO][C-API] Correct api comments.
Dec 19 2017, 6:50 AM
bd1976llvm committed rL321077: [Support][CachePruning] Disable cache pruning regression fix.
[Support][CachePruning] Disable cache pruning regression fix
Dec 19 2017, 6:43 AM
bd1976llvm closed D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning.
Dec 19 2017, 6:43 AM

Dec 18 2017

bd1976llvm added inline comments to D41279: [ThinLTO][C-API] Correct api comments.
Dec 18 2017, 9:44 AM
bd1976llvm updated the diff for D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning.

Explicitly cast system_clock::now type as well.

Dec 18 2017, 9:17 AM
bd1976llvm updated the diff for D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning.

Thanks for the great review comments.

Dec 18 2017, 8:22 AM

Dec 15 2017

bd1976llvm updated the diff for D41279: [ThinLTO][C-API] Correct api comments.

Updated patch to address review comments

Dec 15 2017, 12:35 PM
bd1976llvm added inline comments to D41279: [ThinLTO][C-API] Correct api comments.
Dec 15 2017, 10:15 AM
bd1976llvm added a comment to D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning.

Add a test? You can use llvm-lto to trigger this legacy interface - although it looks like no support was added to llvm-lto to invoke this interface. It should be straightforward to add that (see where it calls setCacheDir).

Dec 15 2017, 9:01 AM
bd1976llvm added a comment to D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning.

I like your proposal.

However, as the cache code is shared between multiple api's and only the c api has this disabling behaviour for negative values a fix here is more appropriate..

I don't quite agree with that conclusion -- the fact we have one upper layer that supports feature X means that the lower layer needs to support it as well (the other front-ends can just choose not to use it).

Dec 15 2017, 7:01 AM
bd1976llvm updated the diff for D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning.

I removed the comment change.

Dec 15 2017, 3:39 AM
bd1976llvm created D41279: [ThinLTO][C-API] Correct api comments.
Dec 15 2017, 3:36 AM

Dec 14 2017

bd1976llvm updated the diff for D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning.

I like your proposal.

Dec 14 2017, 9:04 AM
bd1976llvm created D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning.
Dec 14 2017, 5:21 AM

Nov 17 2017

bd1976llvm committed rL318524: [Support][CachePruning] Fix regression in pruning interval.
[Support][CachePruning] Fix regression in pruning interval
Nov 17 2017, 6:43 AM

Nov 16 2017

bd1976llvm committed rL318397: [Support][CachePruning] Fix regression in pruning interval.
[Support][CachePruning] Fix regression in pruning interval
Nov 16 2017, 5:16 AM
bd1976llvm closed D40119: [Support][CachePruning] Fix regression in pruning interval by committing rL318397: [Support][CachePruning] Fix regression in pruning interval.
Nov 16 2017, 5:16 AM
bd1976llvm created D40119: [Support][CachePruning] Fix regression in pruning interval.
Nov 16 2017, 3:31 AM

Sep 29 2017

bd1976llvm added a comment to D38283: Do not remove a target file in FileOutputBuffer::create()..

Hi Ruiu,

Sep 29 2017, 5:56 AM
bd1976llvm created D38401: [ELF] Fix incorrect exit status bug exposed by D38283.
Sep 29 2017, 5:53 AM
bd1976llvm committed rL314496: [NFC] Removed accidenatally added file.
[NFC] Removed accidenatally added file
Sep 29 2017, 2:17 AM
bd1976llvm committed rL314495: [ELF] Simpler scheme for handling common symbols.
[ELF] Simpler scheme for handling common symbols
Sep 29 2017, 2:10 AM
bd1976llvm closed D38137: [ELF] Simpler scheme for handling common symbols by committing rL314495: [ELF] Simpler scheme for handling common symbols.
Sep 29 2017, 2:10 AM

Sep 28 2017

bd1976llvm added inline comments to D38137: [ELF] Simpler scheme for handling common symbols.
Sep 28 2017, 9:35 AM
bd1976llvm updated the diff for D38137: [ELF] Simpler scheme for handling common symbols.

Missed a review comment. Now addressed.

Sep 28 2017, 9:31 AM
bd1976llvm updated the diff for D38137: [ELF] Simpler scheme for handling common symbols.

Addressed review comments.

Sep 28 2017, 9:25 AM

Sep 26 2017

bd1976llvm updated the diff for D38137: [ELF] Simpler scheme for handling common symbols.

updated with review comments.

Sep 26 2017, 3:30 PM

Sep 25 2017

bd1976llvm updated the diff for D38137: [ELF] Simpler scheme for handling common symbols.

Updated as per review comments.
Apologies that it took some time to update the patch.

Sep 25 2017, 7:12 AM

Sep 21 2017

bd1976llvm abandoned D38136: [ELF] Simplify handling of removed sections.
Sep 21 2017, 5:12 PM
bd1976llvm updated the diff for D38137: [ELF] Simpler scheme for handling common symbols.

Updated the diff to address review comments.

Sep 21 2017, 4:18 PM
bd1976llvm added a comment to D38136: [ELF] Simplify handling of removed sections.

Having thought about this I still think that we want this patch.

Sep 21 2017, 11:50 AM
bd1976llvm added a comment to D38137: [ELF] Simpler scheme for handling common symbols.
In D38137#877803, @ruiu wrote:

First of all, could you read https://llvm.org/docs/CodingStandards.html and follow that style? One of the easiest way is to run clang-format-diff on your patch so that everything in your patch is formatted automatically.

Sep 21 2017, 11:24 AM
bd1976llvm added a comment to D38136: [ELF] Simplify handling of removed sections.

Check out lld/trunk/test/ELF/common-gc3.s for a test case.

Sep 21 2017, 11:10 AM
bd1976llvm added a comment to D38136: [ELF] Simplify handling of removed sections.

This case commonly arises where the section that is being patched is informational and the linker can't garbage collect it e.g .debug_info.

Sep 21 2017, 11:09 AM
bd1976llvm added a comment to D38136: [ELF] Simplify handling of removed sections.
In D38136#877788, @ruiu wrote:

I'm not sure if this is an improvement. Essentially, we shouldn't call getSymVA on dead symbols (or, more strictly speaking, symbols pointing to dead sections), and I don't think we want to allow it.

Sep 21 2017, 11:02 AM
bd1976llvm added a comment to D37718: [ELF] Handle references to garbage collected common symbols.

I posted this to the mailing list (copy here):

Sep 21 2017, 8:40 AM
bd1976llvm created D38137: [ELF] Simpler scheme for handling common symbols.
Sep 21 2017, 8:36 AM
bd1976llvm created D38136: [ELF] Simplify handling of removed sections.
Sep 21 2017, 8:31 AM

Sep 12 2017

bd1976llvm committed rL313086: [ELF] Handle references to garbage collected common symbols.
[ELF] Handle references to garbage collected common symbols
Sep 12 2017, 3:43 PM