peter.smith (Peter Smith)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 11 2016, 8:56 AM (97 w, 5 d)

Recent Activity

Yesterday

peter.smith added a comment to D43665: Take SHF_ARM_PURECODE into consideration when setting the program header flags.

Thanks for doing this. It is close, but I think that it will clear the PF_R flag too aggressively at the moment. In the comments I've put a link to a very short white paper and a link to the entry in Arm ELF which unfortunately uses an old name for the flag SHF_ARM_NOREAD, first introduced by Arm's proprietary compiler but later changed to SHF_ARM_PURECODE when implemented in gcc.

Fri, Feb 23, 2:10 AM

Thu, Feb 22

peter.smith added a comment to D43005: [ARM] Error out on .arm assembler directives on windows.

I agree that we should try to fix this generally rather than hardcode this to Windows ARM.

Any concrete suggestions on how to go about it? The flag ARM::FeatureNoARM is generally assigned based on the cpu in ARM.td.

Apologies I haven't got ideas off the top of my head. I may be able to find some time tomorrow or early next week to take a look and make some suggestions.

@peter.smith - any ideas?

My apologies, I've not had a chance to look yet, my current task is taking rather longer than I'd like. I'm intending to take a look when I've finished, hopefully next week, although don't let that stop you taking a look yourself.

Thu, Feb 22, 3:00 AM

Mon, Feb 12

peter.smith added a comment to D43126: [LLD][ELF] Do not error for missing version when symbol has local version..

Peter Smith via Phabricator <reviews@reviews.llvm.org> writes:
Index: ELF/Symbols.cpp
===================================================================

    • ELF/Symbols.cpp +++ ELF/Symbols.cpp @@ -206,8 +206,10 @@ It is an error if the specified version is not defined. Usually version script is not provided when linking executable, // but we may still want to override a versioned symbol from DSO,
  • // so we do not report error in this case.
  • if (Config->Shared) + so we do not report error in this case. We also do not error + if the symbol has a local version as it won't be in the dynamic + // symbol table. + if (Config->Shared && VersionId != VER_NDX_LOCAL)

Is the Config->Shared part still required?

Mon, Feb 12, 5:41 AM

Fri, Feb 9

peter.smith created D43126: [LLD][ELF] Do not error for missing version when symbol has local version..
Fri, Feb 9, 6:34 AM
peter.smith accepted D43116: [ELF] Print the .type assembly directive correctly for STT_NOTYPE.

LGTM. I can confirm that gas uses notype and that the other .type values also match those in gas.

Fri, Feb 9, 5:26 AM
peter.smith added a comment to D43071: [ELF] - Support COPY, INFO, OVERLAY output sections attributes..

I mean, for any language, if you google for some extremely minor feature of that language, I'm pretty sure that you can always find one. So the fact that you can find a use case on the internet isn't very convincing that we should support it.

Fri, Feb 9, 3:44 AM

Thu, Feb 8

peter.smith added a comment to D43071: [ELF] - Support COPY, INFO, OVERLAY output sections attributes..

For what it is worth COPY and OVERLAY are used quite a bit in embedded systems. The Arm GCC Embedded toolchain linker scripts in the sample directory use COPY to reserve an area for the heap and stack https://developer.arm.com/open-source/gnu-toolchain/gnu-rm/downloads

Thu, Feb 8, 9:59 AM
peter.smith added a comment to D43005: [ARM] Error out on .arm assembler directives on windows.

I agree that we should try to fix this generally rather than hardcode this to Windows ARM.

Any concrete suggestions on how to go about it? The flag ARM::FeatureNoARM is generally assigned based on the cpu in ARM.td.

Thu, Feb 8, 8:42 AM

Wed, Feb 7

peter.smith added a comment to D43005: [ARM] Error out on .arm assembler directives on windows.

My initial thought is that there must be a better way than hard-coding a windows OS choice in the hasARM() field, particularly as all the others work entirely from the feature bits. Do you happen to know why clearing the feature bits is insufficient? I haven't touched this part of the code for a long time so I haven't got an answer for why myself. I can see that the assembler might recalculate them from the CPU description when particular directives are used.

Wed, Feb 7, 2:40 AM

Tue, Feb 6

peter.smith added a reviewer for D42930: [Aarch64] Fix linker emulation for Aarch64 big endian: cpirker.

cpirker, as the original author do you have any concerns about changing the big-endian emulation name to match binutils?

Tue, Feb 6, 2:11 AM
peter.smith added a comment to D42930: [Aarch64] Fix linker emulation for Aarch64 big endian.

I can confirm that binutils use aarch64linuxb as their target linux emulation and not aarch64_be_linux. I checked the original commit http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20140310/101365.html , this has references to ld-linux-aarch64_be.so which does seem to be mentioned in binutils and the directory "/lib/aarch64_be-linux-gnu" that I couldn't find any reference to, or any close equivalent of that directory mentioned in binutils. It may be worth checking that as well.

Tue, Feb 6, 2:09 AM

Fri, Feb 2

peter.smith updated the diff for D42797: [LLD][ELF] Implement --[no-]apply-dynamic-relocs option..

Thanks to both for the comments. I've incorporated them in the diff.

Fri, Feb 2, 2:08 AM

Thu, Feb 1

peter.smith created D42797: [LLD][ELF] Implement --[no-]apply-dynamic-relocs option..
Thu, Feb 1, 8:00 AM

Jan 24 2018

peter.smith added a comment to D42471: [ARM] Fix lld crash introduced by r321154.

Thanks for the change. I've no further comments.

Jan 24 2018, 10:00 AM · lld
peter.smith added a comment to D42471: [ARM] Fix lld crash introduced by r321154.

Thanks for adding the test. I've added a small suggestion in an inline comment.

Jan 24 2018, 9:10 AM · lld
peter.smith added a comment to D42471: [ARM] Fix lld crash introduced by r321154.

Thanks for sending the patch. What I'd like to know first is in what circumstances the synthetic section has no parent and is this expected to be possible? If it isn't expected to be possible then it would be better to fix that and not try to account for the no parent case. The implementation has changed quite a bit since I last looked so I'm not sure of the intention.

Jan 24 2018, 5:55 AM · lld
peter.smith accepted D42196: [compiler-rt] [builtins] Align addresses to cache lines in __clear_cache for aarch64.

Yes I'm happy to approve.

Jan 24 2018, 2:11 AM

Jan 23 2018

peter.smith updated the diff for D42421: [LLD][ELF] Make --fix-cortex-a53-843419 work on big endian hosts.

Spoke to soon, forgot to correct the last ulittle32_t that was hiding from me. Now updated.

Jan 23 2018, 8:32 AM
peter.smith updated the diff for D42421: [LLD][ELF] Make --fix-cortex-a53-843419 work on big endian hosts.

+ const ulittle32_t *InstBuf = reinterpret_cast<const ulittle32_t *>(Buf + Off);

This is OK.

+ ulittle32_t Instr1 = *InstBuf++;
+ ulittle32_t Instr2 = *InstBuf++;
+ ulittle32_t Instr3 = *InstBuf++;

But the values should still be uint32_t. The endian.h types should only
ever be used to form pointers.

Cheers,
Rafael

Jan 23 2018, 8:28 AM
peter.smith created D42421: [LLD][ELF] Make --fix-cortex-a53-843419 work on big endian hosts.
Jan 23 2018, 7:25 AM
peter.smith added a comment to D42196: [compiler-rt] [builtins] Align addresses to cache lines in __clear_cache for aarch64.

I guess this comes down to the interpretation of (begin, end] when you can only clear a cache line at a time. I think that this makes sense as it matches the powerpc64 below, Linux also follows this approach. I'm not sure how this is done on BSD and Darwin.

Jan 23 2018, 3:00 AM

Dec 15 2017

peter.smith updated the diff for D41246: [LLD][ELF] Optimize Arm PLT entries.

I've implemented the suggestion to choose the form of PLT entry based on whether the offset to the .got.plt can be represented by the shorter encoding. To maintain 16-byte alignment of the entries I've increased the size of the header to 32-bytes. As the --long-plt doesn't do anything any more I've updated all the tests to use the new encoding. Apologies for the size of the diff.

Dec 15 2017, 9:12 AM
peter.smith abandoned D35413: [LLD][ELF] Add DefinedInThunk to SymbolBody to remove need for hash lookup.

I don't think that there was consensus over whether this change was a good idea. I'm abandoning it for now, it can be resurrected if needed.

Dec 15 2017, 3:19 AM
peter.smith abandoned D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.

This revision was split up and fixed in D40359 D40365 and D40735 abandoning as it is no longer needed.

Dec 15 2017, 3:17 AM

Dec 14 2017

peter.smith added a comment to D41246: [LLD][ELF] Optimize Arm PLT entries.
In D41246#955551, @pcc wrote:

Instead of making the behaviour conditional on the flag, could we always emit the long PLT header and then conditionally emit short or long PLTs depending on what the offset is?

Dec 14 2017, 10:29 AM
peter.smith created D41246: [LLD][ELF] Optimize Arm PLT entries.
Dec 14 2017, 9:32 AM
peter.smith updated the diff for D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.

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

Dec 14 2017, 8:47 AM
peter.smith added a comment to D41234: [ELF] Fix placement of a sentinel entry in the .ARM.exidx section..

This looks sensible to me. One minor nit with the comment as the ABI does not mention a sentinel. We add it as some recent versions of libunwind depend on it being there (due to gold and ld.bfd generating one).

Dec 14 2017, 7:11 AM · lld
peter.smith updated the diff for D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.

I think you forgot to upload th new patch.

Cheers,
Rafael

Dec 14 2017, 1:56 AM

Dec 13 2017

peter.smith added a comment to D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.

Comments from Rafael via [llvm-commits]

+ struct ExidxEntry {
+ uint32_t Fn;
+ uint32_t Unwind;
+ };
+
+ // Get the last table Entry from the previous .ARM.exidx section.
+ const ExidxEntry PrevEntry = *reinterpret_cast<const ExidxEntry *>(
+ Prev->Data.data() + Prev->getSize() - sizeof(ExidxEntry));
+ if (IsExtabRef(PrevEntry.Unwind))
+ return false;

This is not safe in a big endian host, no?

Dec 13 2017, 3:49 AM
peter.smith added a comment to D41105: [ELF] Prevent crash in writing an .ARM.exidx sentinel entry..

That looks correct to me. In the test I think it would be useful to check the contents of the table llvm-objdump -s or llvm-readobj -u to check that the sentinel is written in the correct place.

Dec 13 2017, 3:09 AM · lld

Dec 12 2017

peter.smith updated the diff for D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.

Thanks very much for the review comments. I've updated the diff to apply them. The one I haven't done yet is std::unique as I don't think I can make the predicate an equivalence relation without missing opportunities. For an example consider two consecutive (made up) unwind entries { 0xabababab, 0x1 } and { 0x1, 0x1, 0x1 }; I can merge the second into the first but not the first into the second. My guess is that the libstdc++ and libc++ implementations of std::unique will let us get away with the predicate not being symmetric but I have memories of the Visual Studio debug implementation actively checking all the preconditions required by the standard.

Dec 12 2017, 3:15 AM
peter.smith added a comment to D41105: [ELF] Prevent crash in writing an .ARM.exidx sentinel entry..

I think that this is the right thing to do for .ARM.exidx sections. I'm pretty sure the original implementation did do something like that, but it must have been refactored sometime later to use addSection(). I don't think that this is expected for orphans in general though. For example consider the silly example: SECTIONS { .text : { t.o(.text) foo = .; } }
If I provide an object t.o with a .text section and an object t2.o with a .text section, the t2.o will be an orphan that will match in .text. The bfd linker places this after the assignment to foo = .

Dec 12 2017, 2:09 AM · lld

Dec 11 2017

peter.smith added a comment to D38361: [ELF] Stop setting output section size early.

No objections from me. I've successfully rebased D40964 onto this change.

Dec 11 2017, 9:12 AM
peter.smith updated the diff for D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC].

Rebased in light of changes to D38361. We know use the OutSecOff field for comparisons of .ARM.exidx sections.

Dec 11 2017, 9:10 AM
peter.smith updated the diff for D36749: [LLD][ELF][AArch64] Complete implementation of -fix-cortex-a53-843419.

Comments from Rafael

Dec 11 2017, 8:16 AM

Dec 8 2017

peter.smith updated the diff for D36749: [LLD][ELF][AArch64] Complete implementation of -fix-cortex-a53-843419.

Updated patch to remove the assumption that a single .text section is less than 128Mb, and added test case to cover. The change just inserts patches at roughly branch range intervals rather than always putting them at the end.

Dec 8 2017, 3:15 AM

Dec 7 2017

peter.smith added a dependent revision for D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC]: D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.
Dec 7 2017, 9:30 AM
peter.smith added a dependency for D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections: D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC].
Dec 7 2017, 9:30 AM
peter.smith updated the summary of D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.
Dec 7 2017, 9:29 AM
peter.smith created D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.
Dec 7 2017, 9:29 AM
peter.smith added a dependent revision for D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC]: D40966: [LLD][ELF] Refactor to remove loop copying all Sections in OS->finalize() [NFC].
Dec 7 2017, 9:21 AM
peter.smith added a dependency for D40966: [LLD][ELF] Refactor to remove loop copying all Sections in OS->finalize() [NFC]: D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC].
Dec 7 2017, 9:21 AM
peter.smith updated the summary of D40966: [LLD][ELF] Refactor to remove loop copying all Sections in OS->finalize() [NFC].
Dec 7 2017, 9:21 AM
peter.smith created D40966: [LLD][ELF] Refactor to remove loop copying all Sections in OS->finalize() [NFC].
Dec 7 2017, 9:19 AM
peter.smith added a dependency for D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC]: D38361: [ELF] Stop setting output section size early.
Dec 7 2017, 9:17 AM
peter.smith added a dependent revision for D38361: [ELF] Stop setting output section size early: D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC].
Dec 7 2017, 9:17 AM
peter.smith created D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC].
Dec 7 2017, 9:16 AM

Dec 5 2017

peter.smith added a comment to D36742: [LLD][ELF][AArch64] Implement scanner for Cortex-A53 Erratum 843419.

peter.smith updated this revision to Diff 125355.
peter.smith added a comment.

Updated and rebased after addition of range-extension thunks for aarch64.

  • The erratum scanning is now done within the same loop as range extension thunks.
  • Added a new test case of an erratum sequence that will only be detected on the second pass. As the scanner only detects patches and doesn't try and fix them we cannot test the case where a new thunk is required due to a patch being added.

What is the plan for when we detect sequences to patch on mulitple
iterations? Given that the address of the instructions are important, we
can end up patching sequences that in the end are in addresses that
don't actually need patching, no?

That is probably fine as avoiding it would probably require a fancier
optimization algorithm.

LGTM.

Cheers,
Rafael

Dec 5 2017, 3:24 AM

Dec 4 2017

peter.smith updated the diff for D36749: [LLD][ELF][AArch64] Complete implementation of -fix-cortex-a53-843419.

Rebased after changes to D36749.

  • We no longer initialize the mapping symbols in the constructor as we may be constructed but not used.
  • If a patch is inserted we must update addresses again for thunks.
Dec 4 2017, 9:42 AM
peter.smith updated the diff for D36742: [LLD][ELF][AArch64] Implement scanner for Cortex-A53 Erratum 843419.

Updated and rebased after addition of range-extension thunks for aarch64.

  • The erratum scanning is now done within the same loop as range extension thunks.
  • Added a new test case of an erratum sequence that will only be detected on the second pass. As the scanner only detects patches and doesn't try and fix them we cannot test the case where a new thunk is required due to a patch being added.
Dec 4 2017, 9:35 AM
peter.smith added a comment to D40735: [LLD][ELF] InX::BssRelRo should check section contents before marking relro.

I agree with your summary, and I'll remove the .space pre-commit. Will wait till tomorrow to do so.

Dec 4 2017, 8:43 AM
peter.smith updated the diff for D40735: [LLD][ELF] InX::BssRelRo should check section contents before marking relro.

Thanks for the comment, I've adapted the test to include parts of the previous error if mix .bss and .bss.rel.ro. I'm not sure if it adds much value in having two separate test cases, but do let me know if you'd like them split up?

Dec 4 2017, 6:42 AM
peter.smith added a comment to D38361: [ELF] Stop setting output section size early.

I think that this should now be unblocked after the reverting the change that caused PR35478. I'd very much like to see this land as I'd like to implement ARM.exidx table compression and that will depend on the OutSecPos changes here.

Dec 4 2017, 2:57 AM
peter.smith updated the diff for D40735: [LLD][ELF] InX::BssRelRo should check section contents before marking relro.

peter.smith created this revision.
Herald added a subscriber: emaste.

When a linker script is used with a pattern like { *(.bss .bss.*) } the InX::BssRelRo section will match against .bss.*. We want to make sure that when InX::BssRelRo hasn't created >>any .bss.rel.ro InputSections we don't report the .bss OutputSection as relro. We also want to give an error message if relro and non relro bss is placed within the same >>OutputSection.

I wonder if we need to do that much.

Any thoughts on the attached patch? It uses the idea of depending on the
output section name for .bss.rel.ro but doesn't issue an error message
if the user puts .bss.rel.ro in a rw section. That is similar to what
happens with .data.rel.ro, no?

Dec 4 2017, 2:52 AM
peter.smith added a comment to D40732: [LLD][ELF] Add linker script generated data to non-contiguous relro test [NFC]..

This is almost exactly what I added as
test/ELF/relro-non-contiguous-script-data.s.

Would you mind rebasing this to add the BYTE case there instead?

Sorry,
Rafael

Dec 4 2017, 2:34 AM

Dec 1 2017

peter.smith updated the diff for D40732: [LLD][ELF] Add linker script generated data to non-contiguous relro test [NFC]..

Thanks for the comment, I've uploaded a new diff that tests each case individually.

Dec 1 2017, 9:11 AM
peter.smith created D40735: [LLD][ELF] InX::BssRelRo should check section contents before marking relro.
Dec 1 2017, 8:55 AM
peter.smith created D40732: [LLD][ELF] Add linker script generated data to non-contiguous relro test [NFC]..
Dec 1 2017, 8:23 AM
peter.smith added a comment to D40710: [LLD][ELF] Revert r318924 Skip over empty sections when checking for contiguous relro.

Thanks, if there are no objections from the US timezone, I'll commit on Monday.

Dec 1 2017, 8:18 AM
peter.smith created D40710: [LLD][ELF] Revert r318924 Skip over empty sections when checking for contiguous relro.
Dec 1 2017, 3:29 AM

Nov 30 2017

peter.smith added a comment to D38361: [ELF] Stop setting output section size early.

My apologies for PR35478, I'll take a look this afternoon. The main intention there was to guard against 0 sized SyntheticSections as the user can't always be held responsible for them. Personally I'd be in favour of making those fields optional as I clearly didn't have enough tests to catch the problem there. I also think that our tests, for good reasons, tend to look at one feature at a time and we miss out on cases where combinations of features cause problems.

Nov 30 2017, 4:13 AM

Nov 28 2017

peter.smith added a comment to D40423: [ARM][AArch64] Workaround ARM/AArch64 percularity in clearing icache..

I've committed it for you r319166

Nov 28 2017, 4:35 AM
peter.smith updated the diff for D39744: [LLD][ELF][AArch64] Add support for AArch64 range extension thunks..

Updated diff to use ADRP for position independent code. This thunk has a maximum range of +- 4 Gigabytes which is not the whole address space. However the default small code model only supports programs up to 4 Gigabytes in size and the large code model is not currently implemented in gcc and clang for position independent code so it is safe to use for position independent thunks.

Nov 28 2017, 3:58 AM

Nov 27 2017

peter.smith added a dependency for D36742: [LLD][ELF][AArch64] Implement scanner for Cortex-A53 Erratum 843419: D39744: [LLD][ELF][AArch64] Add support for AArch64 range extension thunks..
Nov 27 2017, 9:31 AM
peter.smith added a dependent revision for D39744: [LLD][ELF][AArch64] Add support for AArch64 range extension thunks.: D36742: [LLD][ELF][AArch64] Implement scanner for Cortex-A53 Erratum 843419.
Nov 27 2017, 9:31 AM
peter.smith added a comment to D36742: [LLD][ELF][AArch64] Implement scanner for Cortex-A53 Erratum 843419.

Many thanks for taking the time to review this and some of the other Arm/AArch64 specific stuff, it is much appreciated.

Nov 27 2017, 8:52 AM
peter.smith updated the diff for D36823: [LLD][ELF] Read ARM BuildAttributes section to determine supported features..

Updated review diff incorporating review comments, I'll commit tomorrow if there are no more changes needed.

Nov 27 2017, 7:52 AM
peter.smith added a comment to D36823: [LLD][ELF] Read ARM BuildAttributes section to determine supported features..

Putting comment in Phabricator from llvm-commits

Index: test/ELF/arm-bl-v6.s
===================================================================

  • /dev/null +++ test/ELF/arm-bl-v6.s @@ -0,0 +1,50 @@ + RUN: llvm-mc -filetype=obj -triple=arm-none-linux-gnueabi %s -o %t + RUN: ld.lld %t -o %t2 2>&1 | FileCheck %s + Requires: arm + + On Arm v6 the range of a Thumb BL instruction is only 4 megabytes as the + extended range encoding is not supported. The following example has a Thumb + BL that is out of range on ARM v6 and requires a range extension thunk. + As v6 does not support MOVT or MOVW instructions the Thunk must not + use these instructions either. At present we don't support v6 so we give a + warning for unsupported features. + + CHECK: warning: lld uses extended branch encoding, no object with architecture supporting feature detected. +// CHECK-NEXT: warning: lld may use movt/movw, no object with architecture supporting feature detected.
Nov 27 2017, 7:22 AM
peter.smith added a comment to D39744: [LLD][ELF][AArch64] Add support for AArch64 range extension thunks..

Adding in comment from [llvm-commits]

  • I've used Thunks that can access the whole address range. There are more efficient Thunks that can be used, for example the PI thunk could use the usual ADRP addressing mode, but this would limit the range to +/- 4Gb. In an ideal world we could generate the thunks with a limited >range only when we know they are in range, but this will require some changes and additional complexity in the underlying framework so I chose to keep it simple.

Do you know what other linkers do? Do they start with a adrp and upgrade
the thunk if that is not sufficient?

Nov 27 2017, 4:23 AM

Nov 24 2017

peter.smith added a comment to D40423: [ARM][AArch64] Workaround ARM/AArch64 percularity in clearing icache..

The code changes make sense to me. There is a small typo in the title, it should be "peculiarity", not worth updating the review but will be worth changing in any commit message. It will be worth waiting to see if there are comments early next week from the US due to thanksgiving.

Nov 24 2017, 5:22 AM
peter.smith added a comment to D40365: [ELF] Rename .bss.rel.ro to .data.rel.ro.bss for some Linker Scripts..

Thanks for the comments, I've applied those prior to commit.

Nov 24 2017, 1:05 AM

Nov 22 2017

peter.smith created D40365: [ELF] Rename .bss.rel.ro to .data.rel.ro.bss for some Linker Scripts..
Nov 22 2017, 10:12 AM
peter.smith created D40364: [ELF] Skip over empty sections when checking for contiguous relro.
Nov 22 2017, 10:01 AM
peter.smith added a comment to D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.

I'm going to split this patch up into 3 at the suggestion of this message from [llvm-commits].

Nov 22 2017, 7:50 AM
peter.smith created D40359: [ELF] Give error message when relro sections are not contiguous..
Nov 22 2017, 7:43 AM

Nov 21 2017

peter.smith updated the diff for D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.

Updating diff based on mail comments llvm-commits, rather than paste everything in here, the comments start from these points.
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171113/503153.html
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171120/503888.html

Nov 21 2017, 7:58 AM
peter.smith added a comment to D40269: [compiler-rt] Avoid unnecessarily hiding inline visibility [NFC].

I don't have any objections.

Nov 21 2017, 1:11 AM

Nov 20 2017

peter.smith created D40248: [ELF][ARM] Refine check for when undefined weak needs a Thunk.
Nov 20 2017, 6:50 AM
peter.smith added a comment to D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb.

Would be nice to rename the variable prior to commit.

Nov 20 2017, 5:43 AM

Nov 17 2017

peter.smith added a comment to D40174: [ELF][MIPS] Fix crash in LLD when linking code that needs PIC thunks.

(Adding my Linaro email address as a reviewer) My apologies for missing that case. This looks good to me as by definition we can't find our target section in an empty InputSectionDescription. Will be worth seeing if Rui has any style comments.

Nov 17 2017, 7:14 AM
peter.smith updated the diff for D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb.

Updated diff with an attempt to simplify the check for filetype and mthumb. I've left the existing Args.filtered in expression for now as I couldn't make a better alternative with std::for_any.

Nov 17 2017, 3:11 AM
peter.smith added inline comments to D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb.
Nov 17 2017, 3:09 AM

Nov 16 2017

peter.smith added a comment to D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.

Thanks for pointing that out. I missed that part of the discussion on the Phabricator review. I'll have an experiment with renaming to see what happens.

Nov 16 2017, 9:53 AM
peter.smith created D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb.
Nov 16 2017, 6:50 AM
peter.smith added a comment to D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.

I see. Thank you for explaining. But, how much dangerous is it to use only the last RELRO compared to only the first one? I wonder if it makes sense to just reverse the RELRO segments. Anyway, I'm also interested in hearing from pcc.

Nov 16 2017, 5:51 AM
peter.smith added a comment to D40076: [builtins][compiler-rt][ARM] re-enable test mulsc3.

Thanks for the review. I've tested it locally; with luck it won't show up any problems in configurations I haven't covered. I'll revert and investigate if it causes any problem.

Nov 16 2017, 5:01 AM
peter.smith added a comment to D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.
In D40029#927209, @ruiu wrote:

Hmmm, I think that approach will be difficult for a couple of reasons. The first is that I think it will be difficult to get all the existing dynamic loaders to change unless we could think of an important use-case that can't be solved by just a single segment. I think that if I were a dynamic loader author I'd say "Why do you want multiple non-contiguous RELRO segments in an ELF file designed for an OS with a standard image layout? Why do I need to add complexity for an edge case?". The second is that there is a large lag time between getting a patch accepted in glibc and it getting deployed, for LTS releases we could be looking at several years before loaders that could support multiple RELRO segments.

I don't know if it is actually a problem. In this patch, you are attempting to add a RELRO segment only for the first contiguous regions that should be RELRO. My understanding is that the existing loader works OK with multiple RELRO segments, but only the first RELRO segment takes effect. That means, if you create multiple RELRO segments, that effectively works the same as creating a RELRO for the first contiguous region, no? If that's the case, creating multiple RELRO should fix the crash bug that you are trying to fix.

By doing that, we can buy time to fix dynamic loaders. As a result, the situation will be (1) if you are using an old loader, your binary still work, but the second and further RELRO segments are ignored, and (2) if you are using a newer loader, all RELRO segments are processed.

The above was my plan. What am I missing?

Nov 16 2017, 3:40 AM
peter.smith added a reviewer for D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR: pcc.

Adding pcc as the original author of D28272.

Nov 16 2017, 2:54 AM
peter.smith added a comment to D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.
In D40029#927020, @ruiu wrote:

I think that the right thing to do is to create a RELRO segment for each contiguous RELRO sections and then update the loader so that it can handle multiple RELROs. What do you think?

Nov 16 2017, 2:50 AM

Nov 15 2017

peter.smith created D40076: [builtins][compiler-rt][ARM] re-enable test mulsc3.
Nov 15 2017, 6:57 AM
peter.smith added a comment to D39489: [ELF] - Linkerscript: fix issue with multiple output sections definitions..

If I understand correctly is that there is no restriction on OutputSection name so something like:

bar : { *.bar.1 }
bar : { *.bar.2 }

Would be legal, but not very sensible and a user could just change the name of the second bar. Whereas the case you describe:

Nov 15 2017, 6:32 AM
peter.smith added a comment to D36823: [LLD][ELF] Read ARM BuildAttributes section to determine supported features..

Ping for review. I would like to get this change in as currently lld may produce output that is incompatible with older Arm CPUs. I have some follow up patches that add support for older Arm CPUs meaning that the warnings can be removed. They all depend on detecting the Architecture via the build attributes though.

Nov 15 2017, 3:06 AM
peter.smith added a comment to D39744: [LLD][ELF][AArch64] Add support for AArch64 range extension thunks..

Ping for review; this patch just adds support for AArch64 range extension thunks to the existing framework, no extra complexity has been added to the Relocations implementation.

Nov 15 2017, 2:53 AM
peter.smith abandoned D34634: [LLD][ELF] Full patch for range thunks (not for review).

The individual parts of this collective patch have been committed individually in D34034, D34035, D34036, D34037, D34344, D34345, D34688, D34690, D37743, D34689, D34691, D34692. Abandoning this review as it is no longer necessary.

Nov 15 2017, 2:50 AM
peter.smith added a comment to D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.
In D40029#925731, @ruiu wrote:

I wonder if we can just have two or more PT_GNU_RELRO segments. Is it illegal?

Nov 15 2017, 2:31 AM

Nov 14 2017

peter.smith created D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.
Nov 14 2017, 8:16 AM
peter.smith updated the diff for D36749: [LLD][ELF][AArch64] Complete implementation of -fix-cortex-a53-843419.

Rebased on top of changes to D36742, no other changes.

Nov 14 2017, 2:31 AM
peter.smith updated the diff for D36742: [LLD][ELF][AArch64] Implement scanner for Cortex-A53 Erratum 843419.

Thanks for the clarification about the header. I've renamed the create... to report... and have moved the comment to below the header in the .cpp file.

Nov 14 2017, 2:28 AM

Nov 13 2017

peter.smith updated the diff for D36749: [LLD][ELF][AArch64] Complete implementation of -fix-cortex-a53-843419.

Updated to account for rebasing of D36742

Nov 13 2017, 8:47 AM