Page MenuHomePhabricator

peter.smith (Peter Smith)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 11 2016, 8:56 AM (153 w, 23 h)

Recent Activity

Today

peter.smith added a comment to D59531: [ELF] Produce multiple PT_NOTE segments as needed.
In D59531#1434312, @pcc wrote:

Is it possible for this to fail with a linker script that looks like:

SECTIONS {
  . = SIZEOF_HEADERS;
  .note1 : { *(.note1) }
  . = 0x10000;
  .note2 : { *(.note2) }
}

?

Tue, Mar 19, 5:22 AM · Restricted Project
peter.smith added a comment to D58102: Support X86 Control-flow Enforcement Technology (CET) in LLD.

I guess you mean "I tried", not "I tend" in 1). I think that you can do accomplish 1.) via unconditionally creating the In.GnuPropertySection and SPltSection and by implementing the empty() member function, ensure that they are removed by removeUnusedSyntheticSections() if there isn't anything that uses them. Unconditionally creating the sections means that you don't need to call mergeAggregateMetaData from Driver.cpp prior to createSyntheticSections(). For the case of ifunc where you need to turn off the feature then just inform In.GnuPropertySection and In.SPltSection as they will always be there.

Hi peter, I have implemented your idea in my code, and it work well, I did not update here now because I am not sure about the ifunc.
One of my destination is to remove the Config->X86Feature1AND and do the feature collection work inside synthetic GnuPropertySection class.
I found I can remove the Config->X86Feature1AND except dealing with ifunc, If I remove the Config->X86Feature1AND I still need another global variable.
At this stage I want to disable CET when the program will use ifunc, my current code is Relocations.cpp:1072

// To Be Done.
// Now the IBT can't work with IFUNC feature, because they may conflict in
// changing the PLT. So we disable the IBT when the IFUNC enabled.
if (Sym.isGnuIFunc()) {
  Config->X86Feature1AND &= ~GNU_PROPERTY_X86_FEATURE_1_IBT;
  Config->SPltSectionEnable = false;
}

Do you have some good idea to turn off the CET feature in ifunc ?
Thank you very much!

Tue, Mar 19, 4:45 AM · Restricted Project, lld

Yesterday

peter.smith added a comment to D58102: Support X86 Control-flow Enforcement Technology (CET) in LLD.

deficiencies:

  1. I tend to collect the CET features in GnuPropertySection (SyntheticSection) class or in Writer->run like combineEhFrameSections(), but I found it should first get the CET features, then decide to create the GnuPropertySection or not (SPltSection too).
  2. I didn't remove the Config->X86Feature1AND, because I need it to tell the createSyntheticSections() to create the GnuPropertySection or not (SPltSection too).
Mon, Mar 18, 7:22 AM · Restricted Project, lld

Thu, Mar 14

peter.smith added a comment to D59120: [ELF] Sort notes by alignment in decreasing order.

Both choices look good to me:

  • emit one PT_NOTE for each SHF_ALLOC .note*
  • emit one PT_NOTE for adjacent SHF_ALLOC .note* with the same alignment (don't check alignment of their sizes)
Thu, Mar 14, 4:20 AM · Restricted Project

Wed, Mar 13

peter.smith updated the diff for D59216: [LLD][ELF][ARM] Redesign of .ARM.exidx handling to use a SyntheticSection.

Updated diff to address review comments.

Wed, Mar 13, 3:39 AM
peter.smith added a comment to D59216: [LLD][ELF][ARM] Redesign of .ARM.exidx handling to use a SyntheticSection.

Thanks for the comments I'll update the diff.

Wed, Mar 13, 3:32 AM

Tue, Mar 12

peter.smith added a comment to D58102: Support X86 Control-flow Enforcement Technology (CET) in LLD.

As an earlier comment mentioned, there is quite a lot of code that depends on the details of the .note.gnu.property section added very early to Driver.c which is not where we'd normally expect to see it. It also looks like you using quite a lot of code to get an InputSection to represent the output .note.gnu.property section. Given that there will be multiple Input .note.gnu.property sections but only one Output .note.gnu.property section, I think it would be cleaner to use a SyntheticSection to represent the .note.gnu.property section. SyntheticSections can implement all sorts of custom functionality that could move the parsing and merging code out of Driver.cpp. My suggestion is to:

  • Create a new SyntheticSection (see SyntheticSections.h), something like NotePropertySyntheticSection. This will represent our single output .note.gnu.property section. It can store the status of the FEATUREAND bits.
  • Add an entry to InStruct for a single instance of it that we will use for the output.
  • After InStruct instance is created in Writer.c but before the relocations are scanned to create PLT entries iterate through InputSections and pass each .note.gnu.property to the NotePropertySyntheticSection. Then erase these InputSections from the InputSections. The combineEhFrameSections<ELFT> function in Writer.cpp is one way of doing something like that.
  • When you add the .note.gnu.property InputSection to the NotePropertySyntheticSection you can update the FEATUREAND bits
  • The writeTo() member function of the NotePropertySyntheticSection can output the data needed to make up the output.
Tue, Mar 12, 9:48 AM · Restricted Project, lld
peter.smith updated the diff for D59216: [LLD][ELF][ARM] Redesign of .ARM.exidx handling to use a SyntheticSection.

Updated to make setSizeAndOffsets() use of Size less confusing, fixed some curly brace nits.

Tue, Mar 12, 8:44 AM
peter.smith updated the diff for D59216: [LLD][ELF][ARM] Redesign of .ARM.exidx handling to use a SyntheticSection.

Updated the diff with the requested changes. I've followed pcc's idea to make the section behave more like the EHFrame section. This gets rid of some of the nastier relocation handling code in writeTo().

Tue, Mar 12, 8:20 AM
peter.smith added a comment to D59216: [LLD][ELF][ARM] Redesign of .ARM.exidx handling to use a SyntheticSection.

Thanks for the comments, I'll upload another diff with the changes.

Tue, Mar 12, 8:12 AM

Mon, Mar 11

peter.smith added a comment to D58047: [LLD][ELF][ARM] Synthesise missing .ARM.exidx sections..

We used to handle .ARM.exidx sections as regular sections with a sentinel section that is synthesized and added to the end. We later added code to merge contiguous .ARM.exidx sections, and now this patch is adding sentinels at various points. At this point, maybe it is easier to handle .ARM.exidx as a single synthetic section just like MergeInputSection? That synthetic section removes all input section whose name is .ARM.exidx from the input section list and add itself to the input section. That design gives you more flexibility than the current design, as the intermediate data representation doesn't have to be an InputSection.

Mon, Mar 11, 9:19 AM
peter.smith created D59216: [LLD][ELF][ARM] Redesign of .ARM.exidx handling to use a SyntheticSection.
Mon, Mar 11, 9:19 AM
peter.smith added a comment to D59120: [ELF] Sort notes by alignment in decreasing order.

I personally favour a separate PT_NOTE section per alignment, that gives a note parser simple rule to iterate through the section (the p_align field). I think ordering by decreasing alignment will work in practice for the note sections that we have now, but it may not be in the future. I also think that this is worth doing even if clang is changed to generate .note.gnu.property sections that are 4-byte aligned; clang will often use libraries compiled by gcc (or some other compiler) that does use 8-byte aligned sections.

Mon, Mar 11, 3:04 AM · Restricted Project

Fri, Mar 8

peter.smith added a comment to D58102: Support X86 Control-flow Enforcement Technology (CET) in LLD.

I think https://bugs.llvm.org/show_bug.cgi?id=41000 "PT_NOTE segment not packed correctly" has some relevance to this review. In particular the .note.gnu.property sections on 64-bit machines have 8-byte alignment, merging these with 4-byte aligned note sections such as build-id can cause problems for consumers of PT_NOTE (see also https://sourceware.org/ml/libc-alpha/2018-08/msg00340.html for a discussion on the glibc mailing list). My understanding is that the eventual solution was to collate 4 and 8 byte SHT_NOTE sections separately and output a PT_NOTE section for each. I think that we should follow that and have tests for it.

Fri, Mar 8, 5:53 AM · Restricted Project, lld

Tue, Mar 5

peter.smith added a comment to D58320: [Darwin] Introduce a new flag, -fapple-link-rtlib that forces linking of the builtins library..

I've no objections to adding the command line as a Darwin only option. Implementation looks fine to me although I've not got any prior experience with Darwin.

Tue, Mar 5, 8:15 AM · Restricted Project

Mon, Mar 4

peter.smith added a comment to D58102: Support X86 Control-flow Enforcement Technology (CET) in LLD.

I'm not still convinced that having the mechanism to automatically enable/disable CET depending on how input object files are compiled is a good idea. It seems really bad idea to me. I'd think the right way to roll out CET is (1) compile source files with CET enabled, (2) enforce CET at the linker, and (3) permanently keep the linker option so that CET is not accidentally turned off. Step 3 is in practice mandatory for programs that you want to use CET, or otherwise you are exposed to risk of accidentally disabling it. And if you have to enforce CET at the linker level for programs you are serious about their security, there's no point of "automatically" enabling it. Rather, I'd think that a security mechanism that is "likely" automatically turned on is more harmful than helpful; that gives a false impression that a user's program is protected with CET even if it's not. I seems to me that that's a dangerous situation and would cause security issues that can be prevented now. Enabling and disabling security mechanism should be an intentional choice.

Mon, Mar 4, 6:48 AM · Restricted Project, lld

Fri, Mar 1

peter.smith added a comment to D58795: ELF: Write .eh_frame_hdr explicitly after writing .eh_frame..

It looks like this change broke the clang-armv7-linux-build-cache which uses clang 5.0.1 running on a 32-bit ARM container http://lab.llvm.org:8011/builders/clang-armv7-linux-build-cache/builds/11063 . I've committed a temporary fix that r355195 that changes FileSize to use size_t rather than uint64_t. Mentioning it here just in case you want to fix it in different way.

Fri, Mar 1, 2:58 AM · Restricted Project
peter.smith committed rG22ce712c1915: [ELF][ARM] Fix clang-armv7-linux-build-cache builds of LLD [NFC] (authored by peter.smith).
[ELF][ARM] Fix clang-armv7-linux-build-cache builds of LLD [NFC]
Fri, Mar 1, 2:53 AM

Wed, Feb 27

peter.smith added a comment to D58102: Support X86 Control-flow Enforcement Technology (CET) in LLD.

Unfortunately I can't point you at any official documentation of the flags yet, the next release of the 64-bit ABI will have the details and should be released soon. I hope to post some patches soon after that has been done.

Soon happened to be a few hours ago: https://developer.arm.com/docs/ihi0056/latest/elf-for-the-arm-64-bit-architecture-aarch64-abi-2018q4 search for BTI and PAC

Wed, Feb 27, 9:54 AM · Restricted Project, lld
peter.smith added a comment to D58102: Support X86 Control-flow Enforcement Technology (CET) in LLD.

Thanks for putting this feature forward. I've not had a chance to go through everything in detail but I thought it would be important to mention that AArch64 has a similar set of features (Pointer Authentication PAC and Branch Target Identification BTI) that are going to use .note.gnu.property sections with GNU_PROPERTY_AARCH64_FEATURE_1_AND (same meaning as GNU_PROPERTY_X86_FEATURE_1_AND), with two associated feature bits GNU_PROPERTY_AARCH64_FEATURE_1_BTI and GNU_PROPERTY_AARCH64_FEATURE_1_PAC. AArch64 does need a modified PLT entry to make this work but it doesn't use a .splt, in effect an extra instruction at the top of the PLT if BTI is used and one at the end if PAC is used, or both if both BTI and PAC are needed. Given that we will have at least two targets implementing a similar mechanism but with target specific details then we'll either need to make it my responsibility to generalise the .note.gnu.property implementation so that it can support both AArch64 and X86, or we make it generic from the start. The main difference for AArch64 is that there are two independent feature bits to track.

Wed, Feb 27, 8:10 AM · Restricted Project, lld

Tue, Feb 26

peter.smith added a comment to D58655: lld: add support for aarch64be -m flags.

Rui is correct, neither AArch64 or Arm support big endian at the moment. The output of the linker would certainly be broken. AArch64 and ARM make some demands of the linker that mean some additional complexity is needed:

  • AArch64 instructions are little-endian, data is big-endian. Input objects have little-endian instructions.
  • ARM v6+ in be-8 mode (default), instructions are little-endian, data is big-endian. Input objects have big-endian instructions and a linker must endian reverse the instructions.
  • ARM prior to v6 and v6/v7 in be-32 mode (backwards compatibility only) have big-endian instructions and data.

My understanding is that big-endian is used in some networking and embedded systems as they have native big-endian data, however these tend not to make it to consumer hardware.

Tue, Feb 26, 2:10 AM · Restricted Project

Mon, Feb 25

peter.smith committed rG777e1cfdc32b: [ELF][ARM] Accept and ignore -p and -no-pipleline-knowledge (authored by peter.smith).
[ELF][ARM] Accept and ignore -p and -no-pipleline-knowledge
Mon, Feb 25, 2:49 AM

Fri, Feb 22

peter.smith created D58540: [LLD][ELF][ARM] Accept and ignore -p and -no-pipeline-knowledge.
Fri, Feb 22, 2:40 AM · Restricted Project

Thu, Feb 21

peter.smith added a comment to D58314: [Driver] Sync ARM behavior between clang-as and gas..

Thanks for the update. To summarise I'd like to keep the integrated and non-integrated assembler in synch for Linux like Android, even at the risk of adding an fpu when it isn't needed. I think that given how difficult it is to not use NEON, I think the mfloat-abi=soft guard you've put on will be sufficient. By the way, I'm hoping we aren't talking past each other with the default for armv7-a. I've tried to put as much of what I understand in the comments and I hope the answers make sense, or you'll be able to correct me where I'm wrong. If the timezone delayed comments aren't working well it may be worth finding me on IRC, I usually leave the office about 6:30pm but I can easily arrange a time and log on later if you wanted to discuss.

Thu, Feb 21, 9:41 AM · Restricted Project
peter.smith added a comment to D58047: [LLD][ELF][ARM] Synthesise missing .ARM.exidx sections..

At the moment I'm happy with the current implementation assuming that the .ARM.exidx sections from input objects are treated are modelled as InputSections. If I'm going to need to rewrite all of it as a single synthetic section I can do it, but that is quite a bit of work and there isn't a guarantee it will be any better. Would it be possible to take this patch for the benefit of the teams waiting on the PR, and I'll work on a replacement? If it turns out to be better we can switch to it?

Thu, Feb 21, 1:50 AM

Wed, Feb 20

peter.smith accepted D58429: [CodeGen] Enable the complex-math test for arm.

LGTM. Thanks for the fix. My apologies for missing that at the time, it looks like a cut and paste error on my part.

Wed, Feb 20, 2:18 AM · Restricted Project, Restricted Project

Tue, Feb 19

peter.smith added a comment to D58314: [Driver] Sync ARM behavior between clang-as and gas..

My main concern is that this changes the default behaviour for arm-linux-gnueabi and arm-linux-gnueabihf targets. I've put some suggestions on what I think the behaviour should be.

Tue, Feb 19, 8:02 AM · Restricted Project
peter.smith added a comment to D58320: [Darwin] Introduce a new flag, -fapple-link-rtlib that forces linking of the builtins library..

The implementation changes in the Darwin toolchain look fine to me, although with respect to the command line option I think Petr Hosek's message on cfe-dev is interesting:

GCC implements -nolibc which could be used to achieve the same effect when combined with -nostartfiles (and -nostdlib++ when compiling C++). I'd prefer that approach not only because it improves compatibility with with GCC, but also because it matches existing flag scheme which is subtractive rather than additive (i.e. -nodefaultlibs, -nostdlib, -nostdlib++, -nostartfiles). Clang already defines this flag but the only toolchain that currently supports it is DragonFly.

Tue, Feb 19, 6:34 AM · Restricted Project
peter.smith added a comment to D58221: [msan] Don't delete MSanAtExitRecord.

The test added here is intermittently hanging on our AArch64 buildbot.

Tue, Feb 19, 6:01 AM · Restricted Project, Restricted Project

Feb 14 2019

peter.smith added a comment to D58047: [LLD][ELF][ARM] Synthesise missing .ARM.exidx sections..

The LLVM associated symbol metadata uses SHF_LINK_ORDER (https://llvm.org/docs/LangRef.html#associated-metadata) and I think this is used by the at least some of the sanitizers. I think it would be difficult to get rid of completely.

Feb 14 2019, 2:28 AM

Feb 12 2019

peter.smith added a comment to D58047: [LLD][ELF][ARM] Synthesise missing .ARM.exidx sections..

I think what I have there is correct but it obviously could be clearer. I've added some alternatives inline.

Feb 12 2019, 1:53 AM

Feb 11 2019

peter.smith updated the diff for D58047: [LLD][ELF][ARM] Synthesise missing .ARM.exidx sections..

Uploaded new diff to address review comments, main changes:

  • Added expanded comment explaining why we need these sections.
  • Moved the addition of the synthetic sections to where we add the sentinel and added test that fails if we don't.
Feb 11 2019, 8:47 AM
peter.smith added a comment to D58047: [LLD][ELF][ARM] Synthesise missing .ARM.exidx sections..

Thanks for the comments. I will upload another diff in a few minutes.

Feb 11 2019, 8:45 AM
peter.smith created D58047: [LLD][ELF][ARM] Synthesise missing .ARM.exidx sections..
Feb 11 2019, 4:18 AM

Feb 6 2019

peter.smith added a comment to D57371: ELF: Allow GOT relocs pointing to non-preemptable ifunc to resolve to an IRELATIVE where possible..

Thanks for the update. My Arm tests were fine as well. This all looks good to me, I don't have any more comments at the moment.

Feb 6 2019, 2:36 AM · Restricted Project
peter.smith committed rGaa192bb77660: [ELF][ARM] Add test case that will fail if toPlt() is removed [NFC] (authored by peter.smith).
[ELF][ARM] Add test case that will fail if toPlt() is removed [NFC]
Feb 6 2019, 2:03 AM

Feb 5 2019

peter.smith added a comment to D57371: ELF: Allow GOT relocs pointing to non-preemptable ifunc to resolve to an IRELATIVE where possible..

I've made a few suggestions on the comments, otherwise looks good to me. I think it is better to keep the complexity in one place and document it than spreading it out. It would be good to get some input from George and Rui, or someone else that hasn't been deep diving on ifuncs recently to see if they had any problems following the code. I've run this on some address comparison tests I had lying around for AArch64 and it looks good on these. I'll run these on Arm tomorrow, as that puts the .igot.plt in the .got and will comment if I find any problems, but I'm not expecting any.

Feb 5 2019, 10:53 AM · Restricted Project
Herald added a project to D57371: ELF: Allow GOT relocs pointing to non-preemptable ifunc to resolve to an IRELATIVE where possible.: Restricted Project.

I've suggested a test case that fails when toPlt() is removed in D57743. Aiming to have comments on this review by the end of the day.

Feb 5 2019, 3:38 AM · Restricted Project
peter.smith created D57743: [LLD][ELF][ARM] Add test case that will fail if toPlt() is removed [NFC].
Feb 5 2019, 3:36 AM · Restricted Project

Jan 31 2019

peter.smith added a comment to D57143: [builtins] Rounding mode support for addxf3/subxf3.

The mix of soft-floating point and hard-floating point (with soft-float interface) can happen on embedded systems code as well. The solution Arm's proprietary toolchain had to this problem is for the linker to select libraries based on build-attibutes. It would detect the rounding mode build attribute and select the appropriate library variant, but that isn't available to us here. A colleague suggested an easily available customisation point that someone that had a local need for a different rounding mode could implement.

Jan 31 2019, 8:15 AM · Restricted Project, Restricted Project
peter.smith added a comment to D57371: ELF: Allow GOT relocs pointing to non-preemptable ifunc to resolve to an IRELATIVE where possible..

Thanks for the update. I'll be able to take a look at this in detail next week.

Jan 31 2019, 7:24 AM · Restricted Project

Jan 29 2019

peter.smith added a comment to D57371: ELF: Allow GOT relocs pointing to non-preemptable ifunc to resolve to an IRELATIVE where possible..

Thanks for sharing the WIP. I think the approach will work and should match the behaviour of ld.bfd more closely. I've been trying to think of alternatives to redirecting all the symbols for the non GOT generating case. Off the top of my head:

  • Keep the _PLT RelExpr variants and instead of changing the symbol alter the Expr for the non-plt non-got generating reference. This has the disadvantage of needing _PLT variants for any RelType that might fall into that category.
  • Still create a new symbol for the non-plt, non-got generating reference, but instead of replacing the existing symbol, just change Sym for the affected relocations, leaving the others untouched. I think this would mean you wouldn't need to create DirectSym but it would mean having to store the symbol somewhere so that subsequent non-plt, non-got generating references could use it.

Not sure if either of those are better at this point though.

Jan 29 2019, 6:31 AM · Restricted Project

Jan 28 2019

peter.smith accepted D57274: ELF: Set sh_info on RelaIplt to point to the IgotPlt output section..

Looks good to me. Thanks for the fix.

Jan 28 2019, 3:31 AM

Jan 25 2019

peter.smith added a comment to D57146: [llvm-objdump] - Print LMAs when dumping section headers..
  • If there is a command line option to print the LMA, then there is a risk that a good portion of people won't read far into the help to discover it exists. For that reason I quite like the idea of printing the column only when there are program headers that have different VMA and LMA.

I think James idea is to have both: (1) print the column when there are program headers that have different VMA and LMA and (2) have an option to force always printing the LMA. Having such an option which is an addition to (1) makes sense if you want to have a way to produce consistent output for parsers.

Yes, exactly. The switch means that those reliant on the output format can specify it, or to print LMA where for some specific instance the LMA and VMA happen to be the same, but that doesn't always mean it will be. Finally, having a single switch whose default can be toggled in downstream ports is easier to maintain than a private patch in the middle of the formatting code.

Jan 25 2019, 4:08 AM

Jan 24 2019

peter.smith added a comment to D57146: [llvm-objdump] - Print LMAs when dumping section headers..

I've not got a strong opinion here, for what it is worth:

  • The code changes look fine.
  • I can see LMA being useful to embedded developers, or tools vendors, given a binary without access to or ability to generate a map file. Would they intuitively look for objdump for that information though? I don't know.
  • gnu objdump also gives file offset and flag information which probably have more justification as they can't be derived from other options. If we are looking for output format compatibility then we ought to add those too. I don't think that there is any expectation of that at the moment though.
  • If there is a command line option to print the LMA, then there is a risk that a good portion of people won't read far into the help to discover it exists. For that reason I quite like the idea of printing the column only when there are program headers that have different VMA and LMA.
Jan 24 2019, 7:41 AM

Jan 17 2019

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

I'm happy to split the patch into basic support and consider how we support GC separately.

Jan 17 2019, 1:26 AM

Jan 16 2019

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

Given the comments from bd1976llvm about the intention vs the wording of the ELF specification, and the restriction in this patch of the behaviour to non-comdat groups (assuming intentional) then I think we should take a step back and decide why we want to make the change and what the requirements are. I think that if we are taking the annobin interpretation of the specification with respect to garbage collection and what to be strictly compliant then we should apply the behaviour to all groups. Another interpretation is that only use case for non-comdat groups is to affect garbage collection so we can handle them differently to comdat groups; given the only use case I've seen in many years is annobin then this could be low risk without adding group member squared extra dependencies for all comdat groups. Alternatively if we just want to support annobin then there may be a better way of doing this without involving groups. There is a relocation from each SHT_NOTE section to the code section that it describes. It could be possible to handle these in the same way as .ARM.exidx sections by making the SHT_NOTE section from the group dependent on the code section it relocates against. If we did that then you'd only need to preserve the SHF_GROUP flag on the SHT_NOTES section and wouldn't need to make all the sections in the group depend on each other.

Jan 16 2019, 8:55 AM

Jan 15 2019

peter.smith created D56724: [LLD][ELF][AArch64] Add R_AARCH64_PLT_PAGE_PC to isRelExpr.
Jan 15 2019, 8:03 AM
peter.smith added a comment to D55505: [LLD][ELF] Implement option to force PIC compatible Thunks.

Ping. I think that all the currently requested changes have been made and the out of range link errors problem caused by enabling it has been fixed in D56396.

Jan 15 2019, 3:37 AM

Jan 14 2019

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

I've had a look at the code in a bit more detail and have left some comments. I think we should state somewhere, either as a comment or in a commit message about how we handle garbage collection/icf for Section Groups. As it stands LLD marks all non-SHF_ALLOC sections as live so the primary use case for the non-comdat section groups (ensure removal of function removes all the build notes associated with it) isn't going to happen.

Jan 14 2019, 10:09 AM
peter.smith added a comment to D56650: [lld] [ELF] Support customizing behavior on target triple.

I think we need to be very careful here. If I've understood this correctly then if this functionality is used for anything critical then a manually supplied target will be needed when doing cross-linking. For example my host LLD is x86_64, is just called ld.lld and will have an inferred x86_64 target triple. If someone customises the behaviour of LLD on the triple in a way that doesn't get caught by the test suite then we could get some strange breakages when doing cross-linking. I personally would prefer to see any option like this not try and auto-infer the target unless it can do it reliably and accurately from the input objects and I don't know if that is possible for all supported targets.

Jan 14 2019, 8:47 AM
peter.smith created D56666: [LLD][ELF][AArch64] Add missing PLT relocations to isStaticLinkTimeConstant.
Jan 14 2019, 7:51 AM

Jan 10 2019

peter.smith added a comment to D56205: Add -z common-page-size option.

I don't it is correct to use common-page-size to override the value of max-page-size. My understanding of common-page-size is that it is used to save memory in some cases when the same page in the file can be mapped at a different virtual address. At the moment I don't think LLD supports this so there isn't any benefit in having a different common-page-size.

Jan 10 2019, 10:49 AM · Restricted Project
peter.smith added a comment to D56396: [LLD][ELF] Fix ARM and Thumb V7PILongThunk overflow behavior..

Thanks, I've updated the test to use multiple echos. I also specified a load address of .text_high to force the generation of a second program header. A single one was generating a close to 4Gb file.

Jan 10 2019, 8:14 AM

Jan 9 2019

peter.smith added a comment to D56396: [LLD][ELF] Fix ARM and Thumb V7PILongThunk overflow behavior..

Thanks very much for the review. I'll wait a day before committing (I don't think that this is controversial) and will make the suggested changes in the test there.

Jan 9 2019, 6:16 AM
peter.smith added a comment to D56437: Support blank flag in SHT_GROUP sections for ELF.

I can confirm that the group sections are coming from the annobin tool.
From the man page: https://www.mankier.com/1/annobin

"attach""no-attach"
When gcc compiles code with the -ffunction-sections option active it will place each function into its own section.  When the annobin attach option is active the plugin will attempt to attach the function section to a group containing the notes and relocations for the function.  In that way, if the linker decides to discard the function, it will also know that it should discard the notes and relocations as well.
Jan 9 2019, 2:42 AM

Jan 8 2019

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

For some background, the static glibc.a in the latest (unreleased) build of Fedora contains these non-comdat group sections.

Jan 8 2019, 9:46 AM
peter.smith added a comment to D53906: [ARM][AArch64] Increase TLS alignment to reserve space for Android's TCB.

I've got a small suggestion about the comment. Otherwise I'm happy with the change. I think that it is reasonable to increase the alignment for all operating systems as the amount is not likely to be significant for platforms that make use of TLS.

Jan 8 2019, 3:35 AM

Jan 7 2019

peter.smith added a comment to D55505: [LLD][ELF] Implement option to force PIC compatible Thunks.

I've created D56396 to cover the relocation out of range errors seen when linking the linux kernel.

Jan 7 2019, 8:47 AM
peter.smith created D56396: [LLD][ELF] Fix ARM and Thumb V7PILongThunk overflow behavior..
Jan 7 2019, 8:46 AM
peter.smith updated the diff for D55505: [LLD][ELF] Implement option to force PIC compatible Thunks.

Updated to remove --pic-thunk command line option. We now use only the ld.bfd option --pic-veneer. No other changes.

Jan 7 2019, 3:19 AM
peter.smith added a comment to D55505: [LLD][ELF] Implement option to force PIC compatible Thunks.

Generally looking good.

What is a reason to add the option as --pic-thunk with an alias of --pic-veneer? If existing code uses --pic-veneer, maybe we could just add that option?

Jan 7 2019, 3:19 AM

Jan 2 2019

peter.smith added a comment to D55505: [LLD][ELF] Implement option to force PIC compatible Thunks.

In my test case using Linux compiled for Arm using allyesconfig and ld.lld with these changes applied linking fails as follows:

...
ld.lld: error: relocation R_ARM_MOVT_PREL out of range: -4403772844 is not in [-2147483648, 2147483647]
...

I guess this is since we are using R_ARM_MOVT_PREL in ARMV7PILongThunk. Not sure whether we really care as this is more a constructed case with an insanely big kernel (with ld.bfd the linked kernel ends up at 201MiB!).

ld.bfd seems to use a different approach which allows the kernel to successfully link. See also:
https://lore.kernel.org/patchwork/patch/1021192/#1213770

Jan 2 2019, 11:13 AM

Dec 20 2018

peter.smith added a comment to D55906: [MC] [AArch64] Correctly resolve ":abs_g1:3" etc..

LGTM as well

Dec 20 2018, 1:57 AM
peter.smith added a comment to D55896: [MC] [AArch64] Support resolving fixups for :abs_g0: etc..

LGTM as well

Dec 20 2018, 1:51 AM

Dec 18 2018

peter.smith added a comment to D55817: [ELF][ARM] Process ARM build attributes of dynamic libraries..

Unless we have evidence that many users are making the mistake of linking against incompatible shared objects, my personal preference is that it is better to follow ld.bfd and ignore. I think most toolchains and distributions make this mistake hard to do by accident.

Dec 18 2018, 6:13 AM · lld
peter.smith added a comment to D55817: [ELF][ARM] Process ARM build attributes of dynamic libraries..

I have several reservations about doing this:

  • BuildAttributes are only defined for relocatable object files, the ABI does not require or prevent them from appearing in Shared Objects or Executables therefore nothing should rely on their presence.
  • Can it be guaranteed that the shared object we link against will have the same attributes as the shared object at run time?
  • Should the attributes of the shared object be aggregated with the attributes from the relocatable object?
Dec 18 2018, 4:02 AM · lld

Dec 17 2018

peter.smith updated the diff for D55709: [docs][ARM] Improve How to Cross Compile Compiler-rt Builtins For Arm.

Thanks for the comments. I used projects as that was what was in the LLVM Getting Started guide. I do agree that it is confusing though. I've put a bit more text at the start about where to get compiler-rt source code and where to place it. I've also moved the Baremetal CMake cache to the end as it is different to the other two sections.

Dec 17 2018, 3:54 AM

Dec 14 2018

peter.smith created D55709: [docs][ARM] Improve How to Cross Compile Compiler-rt Builtins For Arm.
Dec 14 2018, 9:25 AM
peter.smith updated the diff for D55555: [LLD][ELF][ARM] Add support for architecture v6m thunks.

Thanks for the comments. I've added some more information to mention where the flags come from and added a reference to InputFiles.cpp where updateSupportedARMFeatures() contains the mapping between architecture and the flags along with some explanation.

Dec 14 2018, 3:25 AM

Dec 13 2018

peter.smith updated the diff for D55555: [LLD][ELF][ARM] Add support for architecture v6m thunks.

Ok I've removed the extra register. I've revisited my notes and the instructions that could potentially fault if not 8-byte aligned (ldrd and strd on v5te) don't exist in v6-m anyway.

Dec 13 2018, 2:10 AM

Dec 12 2018

peter.smith created D55599: [LLD][ELF][AArch64] Fix ADRP relocations to undefined weak reference..
Dec 12 2018, 6:13 AM
peter.smith added a comment to D55555: [LLD][ELF][ARM] Add support for architecture v6m thunks.

It is true that the AAPCS only requires 8-byte stack alignment at procedure call boundaries. I have seen some interrupt handlers assuming 8 byte stack alignment, or at least not considering the case that it might not be, so I thought it better to be safe given that thunks are not easily visible to the programmer. I'm not wedded to the idea though so if you'd prefer I can remove the save of the extra register.

Dec 12 2018, 12:59 AM

Dec 11 2018

peter.smith updated the diff for D55555: [LLD][ELF][ARM] Add support for architecture v6m thunks.

Yes, thanks for the spot, there is a copy-paste error. My apologies for missing that; now corrected.

Dec 11 2018, 6:26 AM
peter.smith updated the diff for D55555: [LLD][ELF][ARM] Add support for architecture v6m thunks.

Thanks for the comments. I've applied the suggested change to remove the else.

Dec 11 2018, 5:47 AM
peter.smith created D55555: [LLD][ELF][ARM] Add support for architecture v6m thunks.
Dec 11 2018, 5:28 AM
peter.smith added a comment to D55550: [LLD][ELF] - Fix the different behavior of the linker script symbols on different platforms..

Overall I'm happy with this change as I think it is simpler than adding another call to Writer<ELFT>::run(). The one remaining thought is whether Writer<ELFT>::run() can be moved into finalizeAddressDependentContent() as there shouldn't be anything after that function that changes address. Could we move the remaining Writer<ELFT>::run() to the end of finalizeAddressDependentContent() ?

Dec 11 2018, 3:52 AM

Dec 10 2018

peter.smith updated the diff for D55505: [LLD][ELF] Implement option to force PIC compatible Thunks.

Thanks very much for the comments, I've applied the suggested changes and updated the docs.

Dec 10 2018, 6:09 AM
peter.smith created D55505: [LLD][ELF] Implement option to force PIC compatible Thunks.
Dec 10 2018, 4:13 AM
peter.smith added a comment to D55423: [LLD][ELF] - A fix for "linker script assignment loses relative nature of section" bug..

I quite like the idea of renaming maybeNeedsThunks(), for example addAddressDependentContent()? We could make sure that there is always at least one pass of assignAddresses done there.

Dec 10 2018, 3:37 AM

Dec 7 2018

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

Thanks for the fix. I kind of thought it might be possible to lazily postpone symbol assignments to other symbols till after linker scripts but this requires quite a bit of contextual information to know when we need the value of a symbol expression. It looks like ld.bfd has a similar post-script fixup for symbol sections and values although it just evaluates all linker defined symbols again rather than all the addresses again. That could be another option if there were performance concerns about redoing the whole of assignAddresses.

Dec 7 2018, 5:54 AM

Dec 6 2018

peter.smith added a comment to D55211: [LLD][ELF] - Support discarding the .dynamic section..

I've had a chance to check through this and other 3 patches in series. If we are intending to do the minimum to stop the linker from crashing and assume the user knows what they are doing when they do this, then these look correct to me. I think these changes are important for use of LLD to link the linux kernel.

Dec 6 2018, 9:19 AM

Dec 3 2018

peter.smith added a comment to D55211: [LLD][ELF] - Support discarding the .dynamic section..

I've managed to build the linux kernel with D55211, D55215 and D55218 (after working around https://bugs.llvm.org/show_bug.cgi?id=39857) . I've not got the means to run the kernel, but it seems like the rela.dyn is present and correct and can be dumped with readelf.

Dec 3 2018, 10:23 AM
peter.smith added a comment to D55211: [LLD][ELF] - Support discarding the .dynamic section..

Will try and test these patches out on the linux kernel build on Thursday (going to be away Tuesday/Wednesday). One thought I had that could potentially simplify all three patches is to treat .dynamic, .dynstr and and .dynsym as a single discardable unit. For example:

  • The .dynsym is not useful without the .dynstr.
  • If there is a .dynsym then there must be a symbol that needs looking up with a dynamic loader, hence there is a strong case for the .dynamic section.
Dec 3 2018, 9:44 AM

Nov 30 2018

peter.smith added a comment to D54621: [ELF] - Do not remove empty sections referenced in LOADADDR/ADDR commands..

This could also occur with ALIGNOF and SIZEOF. It looks like Script->getOrCreateSectionName() is only called when a part of a script references another, whereas SECTIONS uses Script->createSectionName(). Might it be better to set ForceKeepIfEmpty in that function.

Nov 30 2018, 5:31 AM
peter.smith added a comment to D55118: [ELF] - Report an error if empty sections are referenced in LOADADDR/ADDR commands..

I personally would prefer D54621 but I think this is better than producing incorrect output.

Nov 30 2018, 5:26 AM
peter.smith added a comment to D55029: set default max-page-size to 4KB in lld for Android Aarch64.

LGTM. Please commit.

Peter, I wonder if you are fine with the default 64KiB page size with lld, especially given that lld always round up the text segment size to the maximum page size on disk and fill the padding with trap instructions. On average, that should increase the executable size by 32 KiB compared to other linkers. I don't think that size is necessarily bad, because we are doing that for a security purpose, but I wonder if people are OK with that.

Nov 30 2018, 3:07 AM
peter.smith added a comment to D54621: [ELF] - Do not remove empty sections referenced in LOADADDR/ADDR commands..

I think that this type of script is common in embedded systems. A company will have a product family represented by a common core + variable feature sets, they often have a single linker script that is shared between the projects. This often results in some Output Sections being empty for some of the products. On larger projects it can be difficult to know when this will occur, for example all it takes is one developer to remove the use of a library to silently break a working program.

Nov 30 2018, 2:27 AM

Nov 29 2018

peter.smith added a comment to D55029: set default max-page-size to 4KB in lld for Android Aarch64.

Yes this is fine. The effects are entirely within the Android target.

Nov 29 2018, 2:34 AM

Nov 28 2018

peter.smith updated the diff for D54624: [LLD][ELF] Error if _GLOBAL_OFFSET_TABLE_ is defined in input objects.

Thanks for the comment. I've made the suggested change.

Nov 28 2018, 6:12 AM
peter.smith updated the diff for D54624: [LLD][ELF] Error if _GLOBAL_OFFSET_TABLE_ is defined in input objects.

Thanks for the comments. I've added a specific error message for when _GLOBAL_OFFSET_TABLE_ is already defined.

Nov 28 2018, 3:11 AM

Nov 27 2018

peter.smith added a comment to D53980: [ARM, AArch64] Move ARM/AArch64 target parsers into separate files to enable future changes..

No problem, I'm still ok with the changes.

Nov 27 2018, 6:58 AM
peter.smith updated the diff for D54624: [LLD][ELF] Error if _GLOBAL_OFFSET_TABLE_ is defined in input objects.

Thanks, superfluous parentheses removed.

Nov 27 2018, 3:11 AM
peter.smith updated the diff for D54624: [LLD][ELF] Error if _GLOBAL_OFFSET_TABLE_ is defined in input objects.

Updated to remove now unnecessary cast and to make sure REQUIRES has a colon at the end. Reposting in case Rui has any comments.

Nov 27 2018, 3:02 AM
peter.smith updated the diff for D54759: [LLD][ELF] Use more specific method to calculate DT_PLTRELSZ.

Thanks for the comments. I've made the lambda a free function and have made sure REQUIRES has a colon after it in all the tests.

Nov 27 2018, 2:05 AM

Nov 23 2018

peter.smith created D54854: [LLD][AArch64] Cortex-a53-843419 erratum should not apply to relaxed TLS..
Nov 23 2018, 3:56 AM

Nov 22 2018

peter.smith added a comment to D54314: [ELF] - Fix R_AARCH64_ADR_GOT_PAGE, R_AARCH64_LD64_GOT_LO12 handling against IFUNC symbols..

I think this looks good. It is likely that US based people will be away for thanks giving so it will be worth pinging Rui next week.

Nov 22 2018, 1:42 AM
peter.smith accepted D54627: [ELF] - Make SymbolTable::addDefined return Defined..

LGTM. I think that this one is low risk anyway.

Nov 22 2018, 1:40 AM

Nov 21 2018

peter.smith added a comment to D54759: [LLD][ELF] Use more specific method to calculate DT_PLTRELSZ.

Thanks, I'll wait a bit before committing to see if Rui has any comments.

Nov 21 2018, 3:05 AM
peter.smith updated the diff for D54759: [LLD][ELF] Use more specific method to calculate DT_PLTRELSZ.

Agreed, I've made the change to In.RelaIplt->Name == In.RelaPlt->Name.

Nov 21 2018, 3:00 AM