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 (179 w, 6 d)

Recent Activity

Fri, Sep 20

peter.smith created D67848: [LLD][ELF][ARM] Fix crash when discarding all of the InputSections that have .ARM.exidx sections.
Fri, Sep 20, 11:00 AM · Restricted Project
peter.smith added a comment to D67761: [ELF] Error if the linked-to section of a SHF_LINK_ORDER section is discarded.

> If sh_link(a)=b && a has SHF_LINK_ORDER, we currently place a in b's dependentSections list

I need to check for tests and previous commits on this.

If we drop the rule, we'll have to tune the SHF_LINK_ORDER rule a bit in MarkLive.cpp. As a plus(?), we'll improve compatibility with ld.bfd.

For linkerscript/discard-section-metadata.s (4 sections formed as an rooted tree via sh_link), ld.bfd errors:

% ld.bfd -T a.script a -o b
ld.bfd: b: sh_link of section `.bar' points to discarded section `.foo' of `a'
Fri, Sep 20, 4:04 AM · Restricted Project

Thu, Sep 19

peter.smith added a comment to D67761: [ELF] Error if the linked-to section of a SHF_LINK_ORDER section is discarded.

Thanks for the update. I'm happy with this approach. Will be good to hear other opinions as well.

Thu, Sep 19, 6:35 AM · Restricted Project
peter.smith added a comment to D67754: [ELF] Fix two null pointer dereferences related to SHF_LINK_ORDER with --gc-sections.

This will definitely avoid a crash but I'm not sure if a program that can trigger the crash is well defined under garbage collection. For a section A with link order dependency on section B, with A live and B not live, there is a requirement to order section B with respect to the address of section A, and we can't know what the address is if B has been garbage collected. Making an arbitrary choice for the address will work for cases where removing B means that the order of A doesn't matter, however if the order of A does matter then we could get a run-time error.

Thu, Sep 19, 5:12 AM · Restricted Project

Tue, Sep 17

peter.smith added a comment to D67504: [ELF] Make MergeInputSection merging aware of output sections.

Thanks for the update. I don't have any more comments at the moment, I'm in favour of making the change. It would be useful to get the perspective of someone coming to the patch for the first time to see if it was easy for them to understand. A possible alternative would be to filter out the MergeInputSections into a separate list. Assign all the other InputSections as before. Then work out where the MergeSyntheticSections would go, create them and insert them into the OutputSections. However I don't think that would be obviously any better.

Tue, Sep 17, 7:33 AM · Restricted Project
peter.smith added a comment to D67504: [ELF] Make MergeInputSection merging aware of output sections.

Thanks for the updates. I've made a few suggestions about adding comments to define what addSection and recordSection do and when they should be used, and in the case of addSection what they should be called. I also think that there may be a case for a postFinalizeAddSection(InputSection *isec) function to do the actions that we are inlining for the copyRelocations on Relocations.cpp:570. This would make it easier to know what was needed for any other InputSections that needed to be added later.

Tue, Sep 17, 4:14 AM · Restricted Project
peter.smith committed rG43d32cdd8717: [ELF][AARCH64] Refactor AArchErrataFix to match changes in ARMErrataFix NFC. (authored by peter.smith).
[ELF][AARCH64] Refactor AArchErrataFix to match changes in ARMErrataFix NFC.
Tue, Sep 17, 2:49 AM

Mon, Sep 16

peter.smith updated the diff for D67622: [LLD][AARCH64] Small refactor of AArchErrataFix to match changes in ARMErrataFix NFC..

Thanks for the comments. Uploaded new patch with fixes.

Mon, Sep 16, 10:23 AM · Restricted Project
peter.smith created D67622: [LLD][AARCH64] Small refactor of AArchErrataFix to match changes in ARMErrataFix NFC..
Mon, Sep 16, 7:51 AM · Restricted Project
peter.smith added a comment to D67608: [ARM] Preserve fpu behaviour for '-crypto'.

After retesting on a failing example and experimenting a bit, I think that we should only preserve +crypto with -fno-integrated-as. It would also be good to mention D66018 and maybe add the original author as a reviewer.

Mon, Sep 16, 6:40 AM · Restricted Project
peter.smith committed rG1d74940b319c: [ELF][ARM] Fix -Werror buildbots NFC. (authored by peter.smith).
[ELF][ARM] Fix -Werror buildbots NFC.
Mon, Sep 16, 3:08 AM
peter.smith committed rGea99ce5e9b49: [ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417 (authored by peter.smith).
[ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417
Mon, Sep 16, 2:41 AM
peter.smith added a comment to D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417.

LGTM, just a few nits and a requested test.

I will add a case to make sure the 0xffc is handled in a similar way to 0xffe and will add to the nopatch test case.

Mon, Sep 16, 2:21 AM · Restricted Project

Fri, Sep 13

peter.smith updated the diff for D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417.

Updated diff for latest suggestions. Main changes:

  • Simplify the mapping symbol calculations
  • Cache the result of searching for the relocation
  • Fix a bug when determining whether a patch could reach outside its section.
  • Update comments and fix white space in tests.
Fri, Sep 13, 9:35 AM · Restricted Project
peter.smith added a comment to D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417.

Thanks very much for the comments. I'll upload a patch shortly with the suggestions applied.

Fri, Sep 13, 9:29 AM · Restricted Project
peter.smith added a comment to D67504: [ELF] Make MergeInputSection merging aware of output sections.

I agree that we'll need something like this for string merging to be used on many embedded systems. I'm a bit concerned about having two semi-parallel vectors sectionBases and sections as we've got some redundancy there and that could lead to problems. At the moment sectionBases is only used for a small part of the link, yet as we don't empty it, it could be used by mistake later on instead of sections. If having two vectors is unavoidable then we should be clear on what the policy is for using one or the other. If we only intend sectionBases to be used for a short time, I'd empty them or prevent them from being used once we're done with them.

Fri, Sep 13, 7:29 AM · Restricted Project

Thu, Sep 12

peter.smith added a comment to D45375: [ELF] - Introduce synthetic file for linker script symbols..

I agree, I think that a synthetic file has some benefits over using nullptr. I have worked on a linker that did that in the past and it was a useful property to have if no Symbol had a nullptr File. There were cases where one slipped through and it caused a crash, so some extra asserts may be needed.

Thu, Sep 12, 9:43 AM
peter.smith created D67494: Remove redundant linaro slaves from slaves.py.
Thu, Sep 12, 5:27 AM · Restricted Project
peter.smith added a comment to D67482: [ELF][X86] Allow PT_LOAD to have overlapping p_offset ranges on EM_X86_64.

Just a quick, no objections from me. I'll leave the decision to go ahead to people most concerned and knowledgeable about x86_64.

Thu, Sep 12, 2:02 AM · Restricted Project
peter.smith added inline comments to D67481: [ELF] Add -z separate-loadable-segments to complement separate-code and noseparate-code.
Thu, Sep 12, 1:57 AM · Restricted Project
peter.smith added a comment to D67325: [ELF] Map the ELF header at imageBase.

One downside of this approach, albeit not a regression from the old behaviour, is that there is always an RO program segment. An alternative fix would be to detect that a program segment only contained the ELF Header and Program Header and move them to the first program segment that contained a SHF_ALLOC section. This would simplify the output when there is no .rodata to something like (AArch64 values simulated by producing a file with just rodata):

PHDR           0x000040 0x0000000000200040 0x0000000000200040 0x0000e0 0x0000e0 R   0x8
LOAD           0x000000 0x0000000000200000 0x0000000000200000 0x000124 0x000124 RE 0x10000

Any thoughts?

The RO PT_LOAD is allocated w/o or w/ this change. We can apply the optimization, with the change to createPhdrs:

--- a/ELF/Writer.cpp
+++ b/ELF/Writer.cpp
@@ -2116,3 +2116,6 @@ std::vector<PhdrEntry *> Writer<ELFT>::createPhdrs(Partition &part) {
         sec == relroEnd) {
-      load = addHdr(PT_LOAD, newFlags);
+      if (load && load->lastSec == Out::programHeaders && (newFlags & PF_R))
+        load->p_flags = newFlags;
+      else
+        load = addHdr(PT_LOAD, newFlags);
       flags = newFlags;

It will affect 125 tests, though, so I'm not sure whether we want to apply this optimization.
A user can use --no-rosegment to ensure the RO PT_LOAD does not exist.

I think the number of people that have no rodata at all in a executable or shared object with allocated headers will be sufficiently small that it isn't worth updating 125 tests for. I'm happy enough with the change now that I understand it better. I think it will be worth waiting a few days to see if there are any more opinions, and if there aren't any go ahead with this next week.

Thu, Sep 12, 1:16 AM · Restricted Project

Wed, Sep 11

peter.smith added a comment to D67325: [ELF] Map the ELF header at imageBase.

Apologies for the delay in responding. I've gone through in a bit more detail. Just to make sure I understand and hopefully give an alternative explanation to anyone else following:

  • Create an example with no .rodata, like basic-aarch64 for example:
.text
        .globl _start
        .type _start, %function
_start: 
        ret
  • createPhdrs() will create and allocate the ELF to the RO program header as it is the first PT_LOAD.
  • InputSections are assigned to OutputSections, there are no InputSections assigned to the RO program segment so it contains no SHF_ALLOC sections.
  • fixSectionAlignments assigns alignTo(script->getDot(), config->maxPageSize) + (script->getDot() % config->maxPageSize) to addrExpr for both the ELF Header SyntheticSection (RO program segment) and .text (Executable program segment)
  • AssignAddresses sets initial dot to 0x200000 (AArch64) + sizeof(headers) = 0x200120
  • AssignAddresses sets base address of .text to 0x210120 (as .text is SHF_ALLOC), the addrExpr for the ELF Header SyntheticSection (RO program header) is never used.
  • allocateHeaders searches for the first SHF_ALLOC section and finds .text as there are no SHF_ALLOC sections in the RO program segment.
  • allocateHeaders sets the ELF header to the address of the .text section
  • The RO program segment now overlaps the Executable program segment as the ELF Header is in the Executable program segment.
Wed, Sep 11, 8:24 AM · Restricted Project

Tue, Sep 10

peter.smith added a comment to D67325: [ELF] Map the ELF header at imageBase.

I'll take another look tomorrow. Thanks for the clarifications.

Tue, Sep 10, 10:31 AM · Restricted Project
peter.smith added a comment to D67325: [ELF] Map the ELF header at imageBase.

I'm struggling a bit with this one at the moment. I've put some comments in but I still need to work this one through. As I understand it we have today:

  • allocateHeaders() that decides whether the program headers are allocated or not and assigns the headers addresses. This is the same for both linkerscripts and non-linkerscripts.
  • fixSectionAlignments is performed only for the non-linkerscript case just prior to the final address allocation. It adds expressions to the OutputSections so that it will be aligned properly.
Tue, Sep 10, 7:17 AM · Restricted Project
peter.smith added inline comments to D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417.
Tue, Sep 10, 3:57 AM · Restricted Project
peter.smith added a comment to D67388: Add a feature to dump dependency graph..

I think that this will definitely be useful, especially to work out all the files that are involved in referencing a symbol. Looking to the future I'd recommend keeping the symbol dependency information, which is mostly useful for objects and libraries, from relocation dependencies as used in GC as the output is likely to be different and will answer different questions. How do you want the final interface to look when there are many diagnostic analyses?

Tue, Sep 10, 3:54 AM · Restricted Project
peter.smith updated the diff for D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417.

Thanks for the comments. Updated diff to address George's comments. Changes include:

  • remove uses of this
  • inline addend
  • add braces where needed
  • change parameter name from b to s
Tue, Sep 10, 2:28 AM · Restricted Project

Mon, Sep 9

peter.smith updated the diff for D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417.

Updated patch to address review comments.

Mon, Sep 9, 9:33 AM · Restricted Project
peter.smith added a comment to D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417.

I'm halfway through reviewing it, and it looks like a straightforward implementation of a mitigation. Can I ask a (perhaps noob) question? If a CPU can be locked up by an instruction sequence that triggers the bug, and if the CPU is used by Android, does that mean a user application can lock up the entire system?

Mon, Sep 9, 9:33 AM · Restricted Project
peter.smith added a comment to D67310: [ELF][AArch64] Apply some NFC cleanups to AArch64ErrataFix.cpp.

I'd prefer that we retained the strict semantics of mapping symbols, although there are other possible simplifications that could be made. Everything else looks fine.

Mon, Sep 9, 2:08 AM · Restricted Project
peter.smith added inline comments to D67310: [ELF][AArch64] Apply some NFC cleanups to AArch64ErrataFix.cpp.
Mon, Sep 9, 2:07 AM · Restricted Project

Fri, Sep 6

peter.smith accepted D67285: [ELF] Replace error() with errorOrWarn() for the ASSERT command.

LGTM. It can be useful to see an output file for debugging purposes. I agree it will be good to suppress multiple warnings but this could be done later.

Fri, Sep 6, 9:23 AM · Restricted Project
peter.smith created D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417.
Fri, Sep 6, 9:01 AM · Restricted Project

Wed, Sep 4

peter.smith accepted D67164: [ELF] Don't shrink RelrSection.

As far as I can tell this will definitely prevent the oscillation and has already seen success in the Android relocation side. As Rui mentioned on the PR, it would be useful to put a generic convergence limit on finalizeAddressDependentContent() to catch an infinite loop that we'd not thought possible. This could be done separately though.

Wed, Sep 4, 9:05 AM · Restricted Project
peter.smith added inline comments to D67152: Align output segments correctly.
Wed, Sep 4, 5:30 AM · Restricted Project

Mon, Sep 2

peter.smith added inline comments to D67039: [ELF] Add a spell corrector for "undefined symbol" diagnostics.
Mon, Sep 2, 2:52 AM · Restricted Project
peter.smith added a comment to D67039: [ELF] Add a spell corrector for "undefined symbol" diagnostics.

Interesting idea. I suspect we'll need to modify it over time in response to feedback. For example, what if there are several candidates, should we report them all, give one stronger weighting etc. One thing I don't think it will catch, which is my most common and least favourite typo, is a transposition which is usually a distance of 2. I'm less concerned about the algorithm speed at the moment as it should only trigger in an error case, and if it aids debugging then it is time well spent.

Mon, Sep 2, 2:44 AM · Restricted Project

Fri, Aug 30

peter.smith added a comment to D66921: GlobalISel: Add known bits to InstructionSelector.

I note that the ARM equivalent of AArch64InstructionSelector doesn't call have the equivalent of setupMF.

That isn't it. InstructionSelector isn't the pass itself (InstructionSelect is), and setupMF has a default implementation

Fri, Aug 30, 9:35 AM
peter.smith added a comment to D66717: [ELF] Do not ICF two sections with different output sections (by SECTIONS commands).

I'm happy to move forward with the change as well.

Fri, Aug 30, 6:49 AM · Restricted Project
peter.smith added a comment to D66824: [09/10] [LLD] [COFF] Support merging resource object files.

The test mixed-resource-obj.yaml is failing on our ARM/AArch64 buildbot http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/2131 as it doesn't have an X86 target. I think the test is missing a REQUIRES: x86

Fri, Aug 30, 6:14 AM · Restricted Project
peter.smith added a comment to D66921: GlobalISel: Add known bits to InstructionSelector.

This change has caused some test failures in our Armv7 only builders http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-full-sh and http://lab.llvm.org:8011/builders/clang-cmake-armv7-full . An example build failure is http://lab.llvm.org:8011/builders/clang-cmake-armv7-full/builds/7366

Fri, Aug 30, 6:00 AM

Wed, Aug 28

peter.smith added a comment to D62868: [libc++] Always build with -fvisibility=hidden.

Apologies for the late notice, we've just moved one our builders http://lab.llvm.org:8011/builders/libcxx-libcxxabi-libunwind-armv7-linux-noexceptions to use gcc rather than clang and we've just picked up some test failures due to this change. In summary when compiling libc++ with GCC the shared object defines a symbol as hidden that clang does not which is causing test failures due to linker errors. I've put the details in https://bugs.llvm.org/show_bug.cgi?id=43140 . It looks like the failure was first seen on http://lab.llvm.org:8011/builders/libcxx-libcxxabi-x86_64-linux-ubuntu-gcc5-cxx11/builds/345 (not run by us).

Wed, Aug 28, 6:56 AM · Restricted Project, Restricted Project

Tue, Aug 27

peter.smith added a comment to D66749: [ELF][ARM] Allow PT_LOAD to have overlapping p_offset ranges on EM_ARM.

I've done a basic smoke test of doing a check-all build of clang with LLD with this change applied and I didn't run into any problems.

Tue, Aug 27, 4:16 AM · Restricted Project
peter.smith added a comment to D66749: [ELF][ARM] Allow PT_LOAD to have overlapping p_offset ranges on EM_ARM.

I'd like to see this change on ARM. I'm running a bit behind due to UK public holiday yesterday so I've not had chance to go through all the test comments yet, although these could be fixed up later if needed.

Tue, Aug 27, 3:29 AM · Restricted Project
peter.smith added a comment to D66717: [ELF] Do not ICF two sections with different output sections (by SECTIONS commands).

I agree that this is going in the right direction. I'm particularly happy about splitting out the symbol assignments.

Tue, Aug 27, 3:18 AM · Restricted Project
peter.smith added a comment to D58540: [LLD][ELF][ARM] Accept and ignore -p and -no-pipeline-knowledge.

The three places were removed https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=091bb549f7722723b284f63ac665e2aedcf9dec9

This has been a no-op in binutils since 2004 (see commit dea514f51da1 in that tree). Given that the lowest officially supported of binutils for the kernel is 2.20, which was released in 2009, nobody needs this flag around so just remove it. Commit 1a381d4a0a9a ("arm64: remove no-op -p linker flag") did the same for arm64.

Maybe we can delete the the compatibility code in a future release, lld 10, or 11?

Tue, Aug 27, 1:55 AM · Restricted Project

Aug 23 2019

peter.smith accepted D66658: [ELF] Align the first section of a PT_LOAD even if its type is SHT_NOBITS.

LGTM. I've tested this on the example and in qemu-aarch64 the p_offset is aligned and the ELF file runs in the same way as when -znow isn't used.

Aug 23 2019, 9:53 AM · Restricted Project
peter.smith committed rG7d6aa7eb7f58: [ELF] Mention contents of reproduce archive and add help description. (authored by peter.smith).
[ELF] Mention contents of reproduce archive and add help description.
Aug 23 2019, 7:43 AM
peter.smith added a comment to D64930: [ELF][AArch64] Allow PT_LOAD to have overlapping p_offset ranges.

I think I know what is causing the kernel to fault the ELF file

Aug 23 2019, 7:13 AM · Restricted Project
peter.smith added a comment to D64930: [ELF][AArch64] Allow PT_LOAD to have overlapping p_offset ranges.

A critical part of the failure seems to be -znow, if I take that out then I get
./bin: error while loading shared libraries: libclang_rt.hwasan-aarch64-android.so: cannot open shared object file: No such file or directory

Will see if I can reproduce in QEMU.

Altering the -dynamic-linker in the response file to /lib/ld-linux-aarch64.so.1

Aug 23 2019, 6:23 AM · Restricted Project
peter.smith added a comment to D64930: [ELF][AArch64] Allow PT_LOAD to have overlapping p_offset ranges.

I can reproduce a segfault with this image on an Ubuntu 16.04 linux machine (CPU is a cortex-a72). For reference I needed to alter the response file to change the dynamic linker --dynamic-linker=/lib/ld-linux-aarch64.so.1

Aug 23 2019, 5:30 AM · Restricted Project
peter.smith created D66641: [LLD][ELF] Mention contents of reproduce archive and add help description..
Aug 23 2019, 4:21 AM · Restricted Project
peter.smith added a comment to D60557: Explain --reproduce option..

It would be good to get at least the tar file part added to the ld.lld --help option, I remember having to use the file command the first time I used --reproduce and I think more people will use --help than look in the man page.

I like that detailed explanation. I've already submitted this change, but can you submit yours to update it?

Aug 23 2019, 3:38 AM · Restricted Project
peter.smith added a comment to D60557: Explain --reproduce option..

It would be good to get at least the tar file part added to the ld.lld --help option, I remember having to use the file command the first time I used --reproduce and I think more people will use --help than look in the man page.

Aug 23 2019, 3:33 AM · Restricted Project

Aug 21 2019

peter.smith accepted D66539: [ELF][ARM] Simplify some llvm-objdump tests with both ARM/Thumb states.

LGTM, thanks for making the update.

Aug 21 2019, 8:10 AM · Restricted Project
peter.smith added a comment to D66279: [ELF] Make LinkerScript::assignAddresses iterative.

I'm happy with this as it stands. It should help prevent problems coming from alterations to the linux kernel's linker script and also help others migrating from ld.bfd. With luck we'll be able to build on this to fix the symbol type problem in https://bugs.llvm.org/show_bug.cgi?id=42506

Aug 21 2019, 7:01 AM · Restricted Project
peter.smith added a comment to D66426: [lld] Enable a watermark of loadable sections to be generated and placed in a note section.

My first reaction is that this seems to be quite a bit of a platform specific feature to build into the linker, it could also make the platform dependent on LLD if this didn't also get into binutils or other ELF linkers. An alternative approach which I believe has been used in other platforms before (for example https://people.freebsd.org/~tmm/elfcksum.c) is to reserve some empty space in the binary that an external tool can post-process to write any checksum/hash etc that you want into it. This is not as convenient but would be compatible with other linkers and not require a LLVM specific extension to ELF.

Aug 21 2019, 6:34 AM · Restricted Project
peter.smith added a comment to D66523: [LLD][ELF] - Simplify the bad-archive.s test case..

LGTM too.

Aug 21 2019, 4:16 AM · Restricted Project

Aug 19 2019

peter.smith created D66427: [zorg] Reorganise Linaro Armv7 and Armv8 builders.
Aug 19 2019, 10:00 AM · Restricted Project
peter.smith accepted D66007: [ELF] Move (copy relocation/canonical PLT) before error checking.

I'm happy with the changes. Looks good to me.

Aug 19 2019, 6:32 AM · Restricted Project
peter.smith added a comment to D66279: [ELF] Make LinkerScript::assignAddresses iterative.

Thanks for the update. Have been thinking in the back of my mind if there were any other way of solving the problem without needing to iterate. The only thing I can think of right now is some kind of analysis that, for want of a better word, topologically sorts expressions so that there are no forward references, with an error message if this isn't possible. This could get complicated as there is limited scope to move or reorder assignments in OutputSections. Other than that I've not got any more comments at the moment.

Aug 19 2019, 3:13 AM · Restricted Project
peter.smith committed rG2cafd872fb97: [ELF][ARM] Add a test that maxes out the thunk convergence limit (authored by peter.smith).
[ELF][ARM] Add a test that maxes out the thunk convergence limit
Aug 19 2019, 2:49 AM
peter.smith closed D66346: [LLD][ELF][ARM] Add a test that maxes out the thunk convergence limit.
Aug 19 2019, 2:48 AM · Restricted Project
peter.smith added a comment to D66346: [LLD][ELF][ARM] Add a test that maxes out the thunk convergence limit.

Looks good. 2 questions are inline.

Aug 19 2019, 2:48 AM · Restricted Project

Aug 16 2019

peter.smith updated the diff for D66346: [LLD][ELF][ARM] Add a test that maxes out the thunk convergence limit.

Thanks for the comments. I've merged the .text sections and have made an attempt at improving the comments.

Aug 16 2019, 11:08 AM · Restricted Project
peter.smith added a comment to D66279: [ELF] Make LinkerScript::assignAddresses iterative.

I've created D66346 which maxes out the permitted Thunk convergence limit. I've done it as Arm rather than AArch64 as the range limits are lower on Arm. This should be sufficient to test that if there is any interaction between convergence limits. Apologies for the delay in putting it together.

Aug 16 2019, 7:07 AM · Restricted Project
peter.smith created D66346: [LLD][ELF][ARM] Add a test that maxes out the thunk convergence limit.
Aug 16 2019, 6:44 AM · Restricted Project
peter.smith added a comment to D66337: [AArch64InstrInfo] Stop getInstSizeInBytes returning non-zero for meta instructions..

Have I got the terminology the wrong way round? I assumed relaxing meant the act of converting a branch who destination is not within range into a one that is within range either by inversion or introducing a second branch with sufficient range. I just want to make sure my commit message makes sense.

Aug 16 2019, 5:22 AM · Restricted Project
peter.smith accepted D66337: [AArch64InstrInfo] Stop getInstSizeInBytes returning non-zero for meta instructions..

Looks good to me.

Aug 16 2019, 3:54 AM · Restricted Project

Aug 15 2019

peter.smith added a comment to D66279: [ELF] Make LinkerScript::assignAddresses iterative.

I think that this is close. I've got some concerns about the interaction of convergence limits, and it may be worth investigating if it is possible to make a separate symbol update pass that runs after addresses have stabilized.

Aug 15 2019, 3:12 AM · Restricted Project
peter.smith added a comment to D66277: [ELF][AArch64] Improve error message for unknown relocations.

Looks good to me. George's suggestion for a test is a good one. From a brief look some of the other targets also return R_ABS by default from getRelExpr, such as ARM, PPC, PPC64, Hexagon, MSP430, AVR. I don't think it is worth applying this method universally to all of them, but it may be worth ARM, PPC and PPC64. When this goes in I'd be happy to submit a similar patch for ARM.

Aug 15 2019, 2:10 AM · Restricted Project

Aug 13 2019

peter.smith added a comment to D66130: [ELF] Initialize 2 fields of Symbol in SymbolTable::insert.

LGTM too, I agree with George that making the comment specific isn't helpful but a general one isn't doing any harm.

Aug 13 2019, 2:59 AM · Restricted Project
peter.smith accepted D65995: [ELF] Don't special case symbolic relocations with 0 addend to ifunc in writable locations.

Given that this is mostly deleting code, I'm happy to approve.

Aug 13 2019, 1:47 AM · Restricted Project
peter.smith added a comment to D64930: [ELF][AArch64] Allow PT_LOAD to have overlapping p_offset ranges.

I'm personally in favour of accepting D64930 and D64906 (I think that there is also D65865 on X86) as I think that this is an important size optimization already performed by Gold and BFD. Thinking about how to progress this further, I think it would be good to write a summary of where each of these reviews is at and what the plan is for this feature. For example:

  • Have all the comments been addressed, I think the main concern was about TLS?
  • Which targets do we want this to apply to, all of them, or just those with a 64k page size?
  • What is the dependency tree of the reviews, I think D64906 is a pre-requisite for all of them?
  • Does anyone have any objection, or are they happy for individual people active in a backend to approve that part?
Aug 13 2019, 1:45 AM · Restricted Project
peter.smith accepted D66091: [ELF] Simplify handling of exportDynamic and isPreemptible.

Thanks for the update, this looks a lot easier to understand. Looks good to me.

Aug 13 2019, 1:20 AM · Restricted Project
peter.smith added a comment to D65995: [ELF] Don't special case symbolic relocations with 0 addend to ifunc in writable locations.
In D65995#1626365, @pcc wrote:

HWASAN should only be using GOT relative relocations to access shadow memory, so I wouldn't expect this change to have an impact on HWASAN.

Thanks for clarification!

Thanks, I've no more objections.

Aug 13 2019, 12:44 AM · Restricted Project

Aug 12 2019

peter.smith added a comment to D66091: [ELF] Simplify handling of exportDynamic and isPreemptible.

The code changes look good to me. I've made some suggestions on the name of the flag and some of the comments.

Aug 12 2019, 11:18 AM · Restricted Project
peter.smith added a comment to D65995: [ELF] Don't special case symbolic relocations with 0 addend to ifunc in writable locations.

If I've understood correctly, this will make the case "address of an ifunc is taken from RW but there are no other relocations that would create a canonical PLT entry" worse as we would always create the canonical PLT entry even if it isn't strictly needed. The trade off is simpler code and a possible size saving from only needing one irelative relocation?

We have probably overloaded the meaning of "canonical PLT" here:) Let me give a bit more details about the two types of canonical PLT:

  1. STT_FUNC, Undefined or SharedSymbol. The canonical PLT makes it a Defined and exports it (replaceWithDefined sets the exportDynamic field) in the dynamic symbol table. This fake definition (st_value=addr(PLT)!=0, st_shndx=0, ld.so has logic to handle such symbols) can preempt a real definition in a DSO. This can be seen as an address leak (in AArch64 BTI protection such PLT needs BTI c).
  2. STT_IFUNC, Defined. The canonical PLT is created because the symbol is referenced by a non-PLT-generating-non-GOT-generating relocation. This case is already a Defined, so there is no new Defined (exportDynamic field is not set). We just change some fields of the symbol (st_value and st_type are important to ld.so. For st_shndx, as long as is non-zero, it doesn't matter what its actual value is). If the STT_GNU_IFUNC symbol was exported before, the converted STT_FUNC is still exported. If the STT_GNU_IFUNC was not (e.g. local/hidden), the new STT_FUNC is not.

    Both are created for pointer equality. This patch deals with 2).

    If the ifunc is referenced by another component.
  3. Without a canonical PLT, its type is STT_GNU_IFUNC. A reference (symbolic relocation/GLOB_DAT/JUMP_SLOT) has to call the ifunc resolver to get the real address.
  4. With a canonical PLT, its type is STT_FUNC. A reference does not have to call the ifunc resolver, but every subsequent function call has to go through the canonical PLT. Address taken of a non-preemptable ifunc in a static storage (.rodata, .data, etc) is rare, so when making the trade-off, we can lean toward implementation complexity.
Aug 12 2019, 7:06 AM · Restricted Project
peter.smith added a comment to D66007: [ELF] Move (copy relocation/canonical PLT) before error checking.

Thanks for the update, I've no more comments.

Aug 12 2019, 2:48 AM · Restricted Project

Aug 9 2019

peter.smith added a comment to D66007: [ELF] Move (copy relocation/canonical PLT) before error checking.

Not got a strong opinion here, happy to go with the consensus.

Aug 9 2019, 9:54 AM · Restricted Project
peter.smith added a comment to D65995: [ELF] Don't special case symbolic relocations with 0 addend to ifunc in writable locations.

If I've understood correctly, this will make the case "address of an ifunc is taken from RW but there are no other relocations that would create a canonical PLT entry" worse as we would always create the canonical PLT entry even if it isn't strictly needed. The trade off is simpler code and a possible size saving from only needing one irelative relocation?

Aug 9 2019, 8:40 AM · Restricted Project

Aug 8 2019

peter.smith added a comment to D65857: [MC][AArch64] Restrict use of signed relocation operators on MOV[NZK].

Thanks for getting back to me, I'll look into making a strict mode. I'll also mention the use case you've identified with movk to my colleague from our GCC team, it is a pity that we don't have a R_AARCH64_PREL_G3_NC for the movk as although not ideal, as it is skipping an overflow check, it would permit GNU ld to be extended whilst retaining the property that you don't need to disassemble the binary to know how to evaluate a relocation.

Aug 8 2019, 10:03 AM
peter.smith committed rGd4695e1d75a3: [ELF][AArch64] Support for movz, movk tprel relocations (authored by peter.smith).
[ELF][AArch64] Support for movz, movk tprel relocations
Aug 8 2019, 6:39 AM
peter.smith added a comment to D65882: [LLD][ELF][AArch64] Support for movz, movk tprel relocations.

Thanks for the review, I've applied the changes to the test prior to commit.

Aug 8 2019, 6:38 AM · Restricted Project
peter.smith added a comment to D65857: [MC][AArch64] Restrict use of signed relocation operators on MOV[NZK].
In D65857#1619366, @pcc wrote:

MHO: The assembler is a low enough level component that the user can be presumed to know what they're doing, regardless of linker limitations. So I would prefer not to do this. If we do anything about this, we should document the limitations of the GNU linkers somewhere.

Aug 8 2019, 6:18 AM

Aug 7 2019

peter.smith created D65882: [LLD][ELF][AArch64] Support for movz, movk tprel relocations.
Aug 7 2019, 8:17 AM · Restricted Project
peter.smith added a comment to D65857: [MC][AArch64] Restrict use of signed relocation operators on MOV[NZK].

I will also need to fix up the LLD test/ELF/aarch64-reloc.s as this is the only place where the signed relocations are used with _nc on mov[nz] and the checked on movk. A simple change that works is:

 .section .R_AARCH64_MOVW_PREL,"ax",@progbits
    movz x1, #:prel_g0:.+1
-   movz x1, #:prel_g0_nc:.-1
-   movk x1, #:prel_g0:.+1
+   movn x1, #:prel_g0:.-1
+   movk x1, #:prel_g0_nc:.+1
    movk x1, #:prel_g0_nc:.-1
    movz x2, #:prel_g1:.+0x20000
-   movz x2, #:prel_g1_nc:.-0x20000
-   movk x2, #:prel_g1:.+0x20000
+   movn x2, #:prel_g1:.-0x20000
+   movk x2, #:prel_g1_nc:.+0x20000
    movk x2, #:prel_g1_nc:.-0x20000
    movz x3, #:prel_g2:.+0x300000000
-   movz x3, #:prel_g2_nc:.-0x300000000
-   movk x3, #:prel_g2:.+0x300000000
+   movn x3, #:prel_g2:.-0x300000000
+   movk x3, #:prel_g2_nc:.+0x300000000
    movk x3, #:prel_g2_nc:.-0x300000000
    movz x3, #:prel_g2:.+0x300000000
    movz x4, #:prel_g3:.+0x4000000000000
    movz x4, #:prel_g3:.-0x4000000000000
-   movk x4, #:prel_g3:.+0x4000000000000
-   movk x4, #:prel_g3:.-0x4000000000000
Aug 7 2019, 6:41 AM
peter.smith created D65857: [MC][AArch64] Restrict use of signed relocation operators on MOV[NZK].
Aug 7 2019, 3:54 AM

Aug 6 2019

peter.smith committed rG7f320d4bf074: [ELF][ARM] Fix /DISCARD/ of section with .ARM.exidx section (authored by peter.smith).
[ELF][ARM] Fix /DISCARD/ of section with .ARM.exidx section
Aug 6 2019, 7:18 AM
peter.smith updated the diff for D65759: [ELF][ARM] Fix /DISCARD/ of section with .ARM.exidx section.

Thanks very much for the comments. I've updated the description, comment, used llvm::erase_if and combined the llvm-readobj into one call.

Aug 6 2019, 3:40 AM · Restricted Project
peter.smith accepted D65550: [AArch64] Do not emit '#' before immediates in inline asm.

I've built a defconfig aarch64 linux kernel without errors including this change.

Aug 6 2019, 2:15 AM · Restricted Project

Aug 5 2019

peter.smith created D65759: [ELF][ARM] Fix /DISCARD/ of section with .ARM.exidx section.
Aug 5 2019, 10:54 AM · Restricted Project
peter.smith added a comment to D65716: [ELF] Consistently prioritize non-* wildcards overs "*" in version scripts.

Thanks for the update. I've not got any more comments.

Aug 5 2019, 5:42 AM · Restricted Project
peter.smith added a comment to D65716: [ELF] Consistently prioritize non-* wildcards overs "*" in version scripts.

Overall looks good to me. I've got a few suggestions on the refactoring. One other thing that might be worth doing to make this easier to review is to split it into two patches:

  • A refactoring change that doesn't change the wildcard behaviour.
  • The change to the wildcard behaviour.

Not got a strong opinion on that now I've gone through it.

Aug 5 2019, 3:13 AM · Restricted Project

Aug 2 2019

peter.smith committed rGf5b91f2a0f9d: [AliasAnalysis] Initialize a member variable that may be used by unit test. (authored by peter.smith).
[AliasAnalysis] Initialize a member variable that may be used by unit test.
Aug 2 2019, 1:11 AM

Aug 1 2019

peter.smith added a comment to D65584: [ELF] Make binding (weak or non-weak) logic consistent for Undefined and SharedSymbol.

IIUC the desired state you are proposing is:

Aug 1 2019, 9:34 AM · Restricted Project
peter.smith added a comment to D65550: [AArch64] Do not emit '#' before immediates in inline asm.

A colleague has pointed out to me that there is a way of using the c modifier after the % "Require a constant operand and print the constant expression with no punctuation" that works with both clang and GCC. See https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#InputOperands in your example above it would be:

__asm__ ("fmla v2.4s, v0.4s, v1.s[%c0]" :: "I"(0x1))

I still think that it could be useful to have GCC and Clang have the same behaviour for "I", but at least there is a workaround available for this case.

Aug 1 2019, 4:27 AM · Restricted Project
peter.smith created D65568: [AliasAnalysis] Initialize a member variable that may be used by unit test..
Aug 1 2019, 3:23 AM · Restricted Project
peter.smith added a comment to D64906: [ELF][PPC] Allow PT_LOAD to have overlapping p_offset ranges.

I'm happy to go ahead as this is a pre-requisite for supporting AArch64.

Aug 1 2019, 3:23 AM · Restricted Project
peter.smith added reviewers for D65550: [AArch64] Do not emit '#' before immediates in inline asm: nickdesaulniers, t.p.northover, efriedma, ostannard.

I think that this will need a wider review to make check if anyone is dependent on the '#'. My personal opinion is that this is a good change to make as it will make the 'I' constraint more generally useful, without it we may need a new constraint that doesn't add a '#':

  • The # is optional for immediates.
  • GNU assembler and Clang both fault # in an element index, correctly according to my reading of the Arm ARM.
  • GCC does not output the # when expanding the 'I' constraint
    • A colleague mentioned that there may be other cases where a # might be problematic, such as ".byte %0" in a switched section.
Aug 1 2019, 2:15 AM · Restricted Project