Page MenuHomePhabricator

bd1976llvm (ben)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2017, 9:47 AM (156 w, 2 d)

Recent Activity

Thu, Jan 16

bd1976llvm added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.
In D72197#1825390, @pcc wrote:
In D72197#1825012, @pcc wrote:

It looks like this change caused us to start rejecting:

.thumb
.thumb_func
.globl foo
.hidden foo
foo:
adr lr, foo + 1

with:

test.s:6:1: error: unsupported relocation on symbol
adr lr, foo + 1
^

This is exposed by this Android ART code: https://cs.android.com/android/platform/superproject/+/master:art/runtime/arch/arm/quick_entrypoints_arm.S;l=1826

I see a couple of possible fixes for this:

  1. We could go back to the previous behaviour for global hidden symbols.
  2. We could teach MC and LLD about the R_ARM_THM_ALU_PREL_11_0 relocation required to relocate this instruction. Arguably the ART code shouldn't be adding the 1 for Thumb here (because R_ARM_THM_ALU_PREL_11_0 adds the 1 itself), so ART would then need to be fixed.

foo is a hidden definition in the current object file. Can the ART code be changed to use a local symbol (adr lr, .Lfoo) instead?

It could, but I don't really see a good argument for local symbols having different behaviour to strong hidden symbols.

Not special casing hidden in the general case has the nice property that: the assembler behavior is not affected by the visibility. Only the binding can.

We can thus postpone decisions to the linker, and all the logic can be implemented on the linker side. Both a hidden defined symbol and a local symbol is considered non-preemptive, thus they have the same behavior at the linker stage.

The downside is that we increase object file sizes. Hidden symbols are not that common so the downside may be acceptable.

Thu, Jan 16, 5:00 PM · Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

I rebased the patch and addressed the review comments. I also noticed that there was a problem with the patch when explicitly placing non-mergeable sections into one of the "default" mergeable sections (e.g. .debug_str). As these "default" mergeable sections were not being added to the map it wasn't possible to explicitly place symbols into these even if the symbols entsize was compatible (as they would be put into a uniqued section instead). This is not acceptable for these "default" sections since for some of these like e.g. .debug_str, current binutils consumers make the assumption that there can only be one of these sections. The fix for this is WIP and I haven't added LIT tests for it yet; but, I have put up the patch in WIP state to get comments on the approach taken.

Thu, Jan 16, 4:23 PM · Restricted Project
bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Thu, Jan 16, 3:03 AM · Restricted Project
bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

Is this worth merging into 10.0 if it lands after the branch?

Thu, Jan 16, 2:45 AM · Restricted Project

Tue, Jan 14

bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

Thanks, I'll address the points that you have raised.
I tried updating the assert and rebasing the patch but the assert is now firing in the It tests. I will have to investigate this tomorrow and update the patch then.

Tue, Jan 14, 6:10 PM · Restricted Project
bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

This is a partial fix for

Is this still considered a partial fix? Seems pretty complete to me, otherwise what's still missing?

Tue, Jan 14, 12:21 PM · Restricted Project
bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Tue, Jan 14, 12:12 PM · Restricted Project

Mon, Jan 13

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

I laid out a series of three options before:

  • Emit different object-file sections for different mergeability settings.
  • Only mark an object-file section as mergeable if all the symbols in it would have the same mergeability settings.
  • Stop implicitly using mergeability for "ordinary" sections (i.e. sections other than the string section).

Of the above, I really think #2 is the only complete solution.

Can you explain why you think #1 is not "complete"? All three seem to establish correctness; I can see how giving up on the optimization (#3) is not a great solution, but #1 doesn't have that problem and actually preserves it in more places. To be clear, this is just taking advantage of the ability to have multiple sections with the same name in an ELF object file; they will still be assembled into a single section in the linked image.

My understanding is that changing the flags on an MCSection retroactively is pretty counter to the architecture, so I'm not sure how we'd actually achieve #2.

Ah, ok, I reread https://reviews.llvm.org/D72194 and see that it's creating non-contiguous sections (option 1), with unique entity sizes. That should be fine. Dissenting opinion retracted. We should prefer https://reviews.llvm.org/D72194 with the commit message updated.

Mon, Jan 13, 5:36 PM · Restricted Project
bd1976llvm abandoned D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.
Mon, Jan 13, 5:36 PM · Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

Updated the diff to address review comments and expand test coverage.

Mon, Jan 13, 5:36 PM · Restricted Project
bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mon, Jan 13, 5:28 PM · Restricted Project
bd1976llvm retitled D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes from WIP alternative approach for D68101 to [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mon, Jan 13, 5:18 PM · Restricted Project
bd1976llvm added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?

Also, this code is missing a case for STB_GLOBAL and visibility != STV_DEFAULT. Actually, I have noticed before that this case is missing in various places in MC. As it stands this patch may cause a performance regression on platforms that use -fvisibility=hidden.

Visibility is irrelevant here. For a symbol defined in the same section as the instruction, the relocation record will reference a non-SECTION symbol instead of a STT_SECTION after this change. If the symbol is STV_HIDDEN, the linker will think the symbol is non-preemptible. So, no performance regression.

Mon, Jan 13, 12:09 PM · Restricted Project

Mon, Jan 6

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

Thanks for the feedback. I'm sorry that I am being slow on this... v.busy at the moment. Will try to finish the patch in the next few days.

Mon, Jan 6, 7:47 PM · Restricted Project
bd1976llvm added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?

Mon, Jan 6, 2:08 AM · Restricted Project

Fri, Jan 3

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

The solution described in that comment is not acceptable. We are not going to tell users that they cannot assign symbols explicitly to sections because LLVM is unable to promise not to miscompile if they do. It is LLVM's responsibility to correctly compile valid code; enabling mergeability for a section containing unnamed_addr symbols is an optimization, and if it is not safe, it needs to be disabled until we can figure out a way to make it safe.

Fri, Jan 3, 6:58 PM · Restricted Project
bd1976llvm created D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Fri, Jan 3, 6:49 PM · Restricted Project

Thu, Jan 2

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

Sorry for the delay in updating this.

Thu, Jan 2, 4:59 PM · Restricted Project

Dec 3 2019

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

I can't speak for the pragma authors, but I strongly doubt the pragma is intended to force all affected globals to go into a single section unit, since the division into section units is purely an object-file representation issue.

Dec 3 2019, 2:29 AM · Restricted Project

Dec 2 2019

bd1976llvm added a comment to D70665: [llvm-readobj] - Implement --dependent-libraries flag..

The feature certainly could be implemented for COFF, so I think changing the name of the command line option is reasonable. The strings in the .deplibs sections map to libraries in an implementation defined manner, so I refer to them as "specifiers". In COFF dependent libraries are specified via "directives" in the object files. It might be worth naming the option something like: --dependent-lib-directives (as directives works for the entries in an ELF .deplibs sections equally as well as specifiers) ?

Dec 2 2019, 7:01 PM · Restricted Project
bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

Given all that, this patch seems far too aggressive. While mergeable sections can be useful for optimizing arbitrary code that might not use a section, they are also extremely useful for optimizing the sorts of global tables that programmers frequently use explicit sections for. It seems to me that the right fix is to find the place that ensures that we don't put mergeable and non-mergeable objects in the same section unit (or at least conservatively makes the section unit non-mergeable) and fix it to consider entry size as well. That should be straightforward unless that place doesn't exist, in which case we have very serious problems.

Disagree (but I spent all day thinking about this). Going back to https://bugs.llvm.org/show_bug.cgi?id=31828, I think I solved the problem there incorrectly; we should be not marking the explicit section merge-able.

Dec 2 2019, 6:31 PM · Restricted Project

Dec 1 2019

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

Just to make sure I haven't missed anything, I would like to take this patch and run some numbers, for which I need a little bit of time.

Dec 1 2019, 8:18 AM · Restricted Project

Nov 21 2019

bd1976llvm added a comment to D65430: Add `--dependency-files` option, which is equivalent to compiler option -MD..

Hi All, Apologies that I missed the request for input here.

Nov 21 2019, 12:40 PM · Restricted Project

Oct 16 2019

bd1976llvm committed rL374997: Request access for bd1976llvm.
Request access for bd1976llvm
Oct 16 2019, 4:39 AM

Oct 2 2019

bd1976llvm added a reviewer for D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable: rjmccall.
Oct 2 2019, 10:48 AM · Restricted Project

Oct 1 2019

bd1976llvm added a reviewer for D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable: mcgrathr.
Oct 1 2019, 10:20 AM · Restricted Project
bd1976llvm updated subscribers of D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

Hi Ben, this is not really my area of expertise, but it all starts to make some sense to me. I was expecting this transformation to happen earlier, but this is where the magic happens, and this probably where it belongs.
Just to make sure I haven't missed anything, I would like to take this patch and run some numbers, for which I need a little bit of time. If in the mean time someone with some more experience in this area here has a look too, that would be great....

Oct 1 2019, 10:20 AM · Restricted Project
bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

Hi Sjoerd,

Oct 1 2019, 3:46 AM · Restricted Project

Sep 27 2019

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

This feels like it could cause a pretty serious regression. This essentially disables global merging with -fdata-sections, which I know at least one linker relies upon for code size.

Sep 27 2019, 11:05 AM · Restricted Project
bd1976llvm created D68147: [MC][ELF] Prevent globals with an explicit section from being mergeable by default and add a safe option to allow mergeable.
Sep 27 2019, 10:53 AM · Restricted Project, Restricted Project

Sep 26 2019

bd1976llvm created D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.
Sep 26 2019, 1:08 PM · Restricted Project

Jun 12 2019

bd1976llvm committed rG52d3e4b4aa52: [Legacy LTO] Fix build bots: r363140: Fix export name (authored by bd1976llvm).
[Legacy LTO] Fix build bots: r363140: Fix export name
Jun 12 2019, 5:16 AM
bd1976llvm committed rL363151: [Legacy LTO] Fix build bots: r363140: Fix export name.
[Legacy LTO] Fix build bots: r363140: Fix export name
Jun 12 2019, 5:16 AM
bd1976llvm committed rG564d248ec2f2: [ThinLTO]LTO]Legacy] Fix dependent libraries support by adding querying of the… (authored by bd1976llvm).
[ThinLTO]LTO]Legacy] Fix dependent libraries support by adding querying of the…
Jun 12 2019, 4:06 AM
bd1976llvm committed rL363140: [ThinLTO]LTO]Legacy] Fix dependent libraries support by adding querying of the….
[ThinLTO]LTO]Legacy] Fix dependent libraries support by adding querying of the…
Jun 12 2019, 4:05 AM
bd1976llvm closed D62935: [ThinLTO][LTO][Legacy] Fix dependent libraries support by adding querying of the IRSymtab.
Jun 12 2019, 4:05 AM · Restricted Project

Jun 11 2019

bd1976llvm added a comment to D62935: [ThinLTO][LTO][Legacy] Fix dependent libraries support by adding querying of the IRSymtab.

Thanks. I was not sure if the new functions in LTOModule.h/.cpp were in the right file - but I don't see a better place to put them.

This is fine. Do you think you might want to query symbols from InputFile instead of LTOModule in the future?

Jun 11 2019, 1:03 AM · Restricted Project

Jun 10 2019

bd1976llvm added a comment to D62935: [ThinLTO][LTO][Legacy] Fix dependent libraries support by adding querying of the IRSymtab.

We should look at either merging the implementations or providing an api function that provides access to the "llvm.linker.options" named metadata via the IRSymtab. It may not be possible to merge the implementations because in COFF and MACHO the representation of dependent libraries is a string of command line options; whereas, the ELF representation is an array of library strings. This is because for COFF and MACHO there is basically only one linker for each file format so the command line format is fixed. For ELF there are a greater variety of linkers, so I just pass the library strings through to the linker, without processing them into command line arguments, and it is deferred to the linker to interpret the library strings.

It would be good if the API is flexible to return either. Not against having a different API to return linker options but I think that is less clean.

Jun 10 2019, 6:11 PM · Restricted Project

Jun 7 2019

bd1976llvm added a comment to D62935: [ThinLTO][LTO][Legacy] Fix dependent libraries support by adding querying of the IRSymtab.

Thanks for the response.

Jun 7 2019, 5:24 PM · Restricted Project
bd1976llvm retitled D62935: [ThinLTO][LTO][Legacy] Fix dependent libraries support by adding querying of the IRSymtab from [LTO][Legacy] Fix dependent libraries support by adding querying of the IRSymtab to [ThinLTO][LTO][Legacy] Fix dependent libraries support by adding querying of the IRSymtab.
Jun 7 2019, 2:43 AM · Restricted Project

Jun 5 2019

bd1976llvm created D62935: [ThinLTO][LTO][Legacy] Fix dependent libraries support by adding querying of the IRSymtab.
Jun 5 2019, 4:45 PM · Restricted Project

May 21 2019

bd1976llvm added a comment to D62090: [runtimes] Support ELF dependent libraries feature.

Great to see someone wanting to use this. During the RFC we decided not to set SHF_EXCLUDE on the .deplibs ELF section that is used to store the strings from these pragmas. This means that other linkers that don't support dependent libraries will not remove the sections and they will end up in the output. This shouldn't cause any functional issues; however, it might be noticed by developers and commented on. Perhaps we should change this decision? Also, have you considered the other option of having the build system invoke the clang with the --dependent-library=<specifier> switch. This is an alternative approach where modifying the source code is not appropriate, an example use is: https://reviews.llvm.org/D61742.

May 21 2019, 12:48 PM · Restricted Project

May 16 2019

bd1976llvm committed rL360984: [ELF] Implement Dependent Libraries Feature.
[ELF] Implement Dependent Libraries Feature
May 16 2019, 8:44 PM
bd1976llvm committed rG1d16515fb407: [ELF] Implement Dependent Libraries Feature (authored by bd1976llvm).
[ELF] Implement Dependent Libraries Feature
May 16 2019, 8:44 PM
bd1976llvm committed rC360984: [ELF] Implement Dependent Libraries Feature.
[ELF] Implement Dependent Libraries Feature
May 16 2019, 8:44 PM
bd1976llvm committed rLLD360984: [ELF] Implement Dependent Libraries Feature.
[ELF] Implement Dependent Libraries Feature
May 16 2019, 8:43 PM
bd1976llvm closed D60274: [ELF] Implement Dependent Libraries Feature.
May 16 2019, 8:43 PM · Restricted Project

May 9 2019

bd1976llvm committed rG3edca1ac1aea: [LLD][NFC] Refactor: BuildID hash size now computed in one place. (authored by bd1976llvm).
[LLD][NFC] Refactor: BuildID hash size now computed in one place.
May 9 2019, 1:07 AM
bd1976llvm committed rLLD360316: [LLD][NFC] Refactor: BuildID hash size now computed in one place..
[LLD][NFC] Refactor: BuildID hash size now computed in one place.
May 9 2019, 1:06 AM
bd1976llvm committed rL360316: [LLD][NFC] Refactor: BuildID hash size now computed in one place..
[LLD][NFC] Refactor: BuildID hash size now computed in one place.
May 9 2019, 1:06 AM
bd1976llvm closed D61078: [LLD][NFC] Refactor: BuildID hash size calculation is now done in a single place..
May 9 2019, 1:06 AM · Restricted Project

May 1 2019

bd1976llvm committed rG6e32dd6cfd0a: [LLD] Emit dynamic relocations for references to script symbols in -pie links (authored by bd1976llvm).
[LLD] Emit dynamic relocations for references to script symbols in -pie links
May 1 2019, 7:06 AM
bd1976llvm committed rLLD359683: [LLD] Emit dynamic relocations for references to script symbols in -pie links.
[LLD] Emit dynamic relocations for references to script symbols in -pie links
May 1 2019, 7:05 AM
bd1976llvm committed rL359683: [LLD] Emit dynamic relocations for references to script symbols in -pie links.
[LLD] Emit dynamic relocations for references to script symbols in -pie links
May 1 2019, 7:05 AM

Apr 30 2019

bd1976llvm added a comment to D55423: [LLD][ELF] - A fix for "linker script assignment loses relative nature of section" bug..

@grimar - this patch causes a regression in behaviour for references to script symbols. I think https://reviews.llvm.org/D61298 is a reasonable fix. Could you take a look?

Apr 30 2019, 2:52 AM · Restricted Project
bd1976llvm created D61298: [LLD] Emit dynamic relocations for references to script symbols in -pie links.
Apr 30 2019, 2:45 AM · Restricted Project

Apr 25 2019

bd1976llvm updated the diff for D61078: [LLD][NFC] Refactor: BuildID hash size calculation is now done in a single place..

Addressed review comments

Apr 25 2019, 5:30 PM · Restricted Project
bd1976llvm added a comment to D60274: [ELF] Implement Dependent Libraries Feature.

I am keen to keep this moving.

Apr 25 2019, 4:55 PM · Restricted Project

Apr 24 2019

bd1976llvm created D61078: [LLD][NFC] Refactor: BuildID hash size calculation is now done in a single place..
Apr 24 2019, 10:14 AM · Restricted Project

Apr 16 2019

bd1976llvm updated the diff for D60274: [ELF] Implement Dependent Libraries Feature.

No longer shortening "dependent libraries" to "deplibs" except for the .deplibs section (as this takes up bytes on disk).

Apr 16 2019, 5:54 PM · Restricted Project
bd1976llvm added inline comments to D60555: [llvm-objcopy] Fill .symtab_shndx section correctly.
Apr 16 2019, 10:28 AM · Restricted Project

Apr 15 2019

bd1976llvm added a comment to D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

LGTM! Thanks for waiting for me to confirm. Comments inline:

Apr 15 2019, 2:01 PM · Restricted Project

Apr 12 2019

bd1976llvm added a comment to D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

Sorry for the delay in replying. I was trying to understand if the upgrade path was important. Comments inline:

Apr 12 2019, 1:52 PM · Restricted Project

Apr 10 2019

bd1976llvm added a comment to D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

Hi Steven,

Apr 10 2019, 8:25 AM · Restricted Project

Apr 9 2019

bd1976llvm added inline comments to D60274: [ELF] Implement Dependent Libraries Feature.
Apr 9 2019, 11:07 AM · Restricted Project
bd1976llvm added a comment to D60274: [ELF] Implement Dependent Libraries Feature.

I have updated the diff to address review comments. I think we can continue to refine this patch in parallel with discussing the design further (I am not dismissing the concerns raised by @compnerd and @jyknight).

Apr 9 2019, 7:53 AM · Restricted Project
bd1976llvm updated the diff for D60274: [ELF] Implement Dependent Libraries Feature.

Addressed review comments.

Apr 9 2019, 7:24 AM · Restricted Project

Apr 8 2019

bd1976llvm added a comment to D60242: Add IR support, ELF section and user documentation for partitioning feature..
In D60242#1456509, @pcc wrote:

Hi Peter,

I really like the concept. I had some nebulous concerns when I read the RFC; but, I didn't comment as I couldn't think of any alternatives to what you were proposing. What I am writing here is still not fully formed.. so feel free to ignore if it is unhelpful.

My main concern is that this is supporting a rare usecase and I don't really like the idea of adding complexity to the core tools for niche cases.

I feel like it would be better if this could be implemented via post-processing in some way; outside of the core tools.

Some aspect of this, like the linker being able to report all of the functions reachable from a given set of entry points, are of general utility though.

I wonder if there is any scope for implementing something different along the lines of:

  1. Do an initial link.
  2. Analyse the resulting elf and report all of the functions in each partition.
  3. Do another link but this time provide an ordering file so that all of the functions for each partition are placed contiguously.
  4. Extract the functions for each partition, apart form the first, into a partition file which also contains metadata specifying the address range.
  5. Write zeros over all of the functions apart from those in the main partition and then compress the executable (saving the file space).
  6. A loader extension patches in the missing functions for each partition when needed.

Hi Ben, thanks for the feedback. I shared a similar concern around complexity, but I don't see any reasonable alternatives to implementing this directly in the linker.

Your idea of using orderfiles is interesting, but I see at least two showstopper problems with it. The first is that it is incompatible with range extension thunks. Imagine that you have one main partition and two loadable partitions. An orderfile is used to sort the main partition's sections before partition 1's sections before partition 2's sections. We're going to split partition 1 and partition 2 into separate files, but the linker doesn't know that, so it may place a range extension thunk in the middle of partition 1's sections and have partition 2 call it. Now at runtime we will crash if partition 2 is loaded without partition 1 and it tries to call one of the range extension thunks in partition 1. We might be able to work around this issue somehow by telling the linker where the partition boundaries are, but at that point since the linker needs to know how you're going to split the file up anyway it might as well actually split it up for you.

The second is that replacing parts of the file with zeros and compressing it isn't always useful from a size perspective; in many cases it's the uncompressed size that matters because that's what takes up storage space on the device. One might consider uncompressing the code at program startup but that leads to other problems: it slows down startup and prevents the kernel from paging out the pages containing the uncompressed code.

I also see other problems that aren't showstoppers but are still significant:

  • We're leaving some of the binary size gains on the table because we can't move exception-handling sections, dynamically relocatable sections (e.g. vtables) or symbol tables.
  • Since we aren't splitting the symbol table the solution is more error prone since there's nothing stopping someone from dlsym'ing a symbol in an unloaded partition.
  • This requires linking multiple times, which will slow down linking and hurt developer productivity, but the problem is made worse when (full) LTO is being used since we'll need to do LTO multiple times.
Apr 8 2019, 7:49 AM · Restricted Project
bd1976llvm added inline comments to D60274: [ELF] Implement Dependent Libraries Feature.
Apr 8 2019, 7:01 AM · Restricted Project

Apr 5 2019

bd1976llvm added inline comments to D60274: [ELF] Implement Dependent Libraries Feature.
Apr 5 2019, 2:31 PM · Restricted Project
bd1976llvm added a comment to D60274: [ELF] Implement Dependent Libraries Feature.

The early check for ELF is the problem. I care very little about .linker-options itself as long as the ability to ensure that framework linkage is provided (i.e. -framework ..., and '-L ...` can be passed along as options to the linker).

Apr 5 2019, 10:42 AM · Restricted Project
bd1976llvm added a comment to D60274: [ELF] Implement Dependent Libraries Feature.

Am I mistaken in that this effectively prevents the use of options to handle frameworks? That really is a strong requirement and should be part of this patch.

Apr 5 2019, 9:48 AM · Restricted Project
bd1976llvm added a comment to D60242: Add IR support, ELF section and user documentation for partitioning feature..

Hi Peter,

Apr 5 2019, 2:45 AM · Restricted Project

Apr 4 2019

bd1976llvm created D60274: [ELF] Implement Dependent Libraries Feature.
Apr 4 2019, 10:19 AM · Restricted Project

Mar 28 2019

bd1976llvm added a comment to D59553: [LLD][ELF][DebugInfo] llvm-symbolizer shows incorrect source line info if --gc-sections used.

Apologies that I didn't check the information on the original bug. The GDB behaviour does look reasonable, thanks.

Mar 28 2019, 10:07 AM · lld, Restricted Project

Mar 22 2019

bd1976llvm added a comment to D59553: [LLD][ELF][DebugInfo] llvm-symbolizer shows incorrect source line info if --gc-sections used.

<Trying again via phabricator rather than replying on the list.>

Mar 22 2019, 3:57 AM · lld, Restricted Project

Mar 21 2019

bd1976llvm added a comment to D59553: [LLD][ELF][DebugInfo] llvm-symbolizer shows incorrect source line info if --gc-sections used.

Does this do the right thing for a 32-bit target? 64-bit is common, but not universal.

Re. the underlying DWARF issues:
I can't remember whether the DWARF committee has ever considered specifying a special value for "address does not exist" but I doubt it, because the DWARF spec states "If an entity has no associated machine code, none of these [address-related] attributes are specified." (DWARF v5 section 2.17, p.51.) So, the current position of the DWARF spec is that the linker is supposed to rewrite the DWARF, I guess.

I don't think that's the intent of the DWARF standard/committee - it's been pretty careful about designing features that work with existing linker behavior without requiring the linker to be DWARF aware in any way.

Likely the best way to solve this would be to put the debug information for the rest of it in the same section group as the function. In particular the subprogram die for the concrete function should be in a section group along with the function. This was part of the considerations with bringing other things back into the debug_info section in dwarf5 as well.

I'd worry about the overhead of making hunks of standalone DIEs for every function. (though, that said - we do actually use 4 bytes for both a CU-relative offset, and for a sec_offset, I think - so maybe it doesn't matter /that/ much (adding a unit header would be the main cost - and maybe that's significant enough to be problematic size-wise too)).

Currently linkers don't eliminate debug info for garbage-collected sections, so the debug info for exectuables/shared objects are larger than necessary, so you could save bytes of the final output by using more bytes for intermediate object files. That doesn't seem like a bad deal to me.

Mar 21 2019, 3:26 AM · lld, Restricted Project

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