peter.smith (Peter Smith)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 11 2016, 8:56 AM (87 w, 2 d)

Recent Activity

Today

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?

Wed, Dec 13, 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.

Wed, Dec 13, 3:09 AM · lld

Yesterday

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.

Tue, Dec 12, 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 = .

Tue, Dec 12, 2:09 AM · lld

Mon, Dec 11

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.

Mon, Dec 11, 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.

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

Comments from Rafael

Mon, Dec 11, 8:16 AM

Fri, Dec 8

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.

Fri, Dec 8, 3:15 AM

Thu, Dec 7

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.
Thu, Dec 7, 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].
Thu, Dec 7, 9:30 AM
peter.smith updated the summary of D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.
Thu, Dec 7, 9:29 AM
peter.smith created D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.
Thu, Dec 7, 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].
Thu, Dec 7, 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].
Thu, Dec 7, 9:21 AM
peter.smith updated the summary of D40966: [LLD][ELF] Refactor to remove loop copying all Sections in OS->finalize() [NFC].
Thu, Dec 7, 9:21 AM
peter.smith created D40966: [LLD][ELF] Refactor to remove loop copying all Sections in OS->finalize() [NFC].
Thu, Dec 7, 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.
Thu, Dec 7, 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].
Thu, Dec 7, 9:17 AM
peter.smith created D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC].
Thu, Dec 7, 9:16 AM

Tue, Dec 5

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

Tue, Dec 5, 3:24 AM

Mon, Dec 4

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.
Mon, Dec 4, 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.
Mon, Dec 4, 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.

Mon, Dec 4, 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?

Mon, Dec 4, 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.

Mon, Dec 4, 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?

Mon, Dec 4, 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

Mon, Dec 4, 2:34 AM

Fri, Dec 1

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.

Fri, Dec 1, 9:11 AM
peter.smith created D40735: [LLD][ELF] InX::BssRelRo should check section contents before marking relro.
Fri, Dec 1, 8:55 AM
peter.smith created D40732: [LLD][ELF] Add linker script generated data to non-contiguous relro test [NFC]..
Fri, Dec 1, 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.

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

Thu, Nov 30

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.

Thu, Nov 30, 4:13 AM

Tue, Nov 28

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

I've committed it for you r319166

Tue, Nov 28, 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.

Tue, Nov 28, 3:58 AM

Mon, Nov 27

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..
Mon, Nov 27, 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.
Mon, Nov 27, 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.

Mon, Nov 27, 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.

Mon, Nov 27, 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.
Mon, Nov 27, 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?

Mon, Nov 27, 4:23 AM

Fri, Nov 24

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.

Fri, Nov 24, 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.

Fri, Nov 24, 1:05 AM

Wed, Nov 22

peter.smith created D40365: [ELF] Rename .bss.rel.ro to .data.rel.ro.bss for some Linker Scripts..
Wed, Nov 22, 10:12 AM
peter.smith created D40364: [ELF] Skip over empty sections when checking for contiguous relro.
Wed, Nov 22, 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].

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

Tue, Nov 21

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

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

I don't have any objections.

Tue, Nov 21, 1:11 AM

Mon, Nov 20

peter.smith created D40248: [ELF][ARM] Refine check for when undefined weak needs a Thunk.
Mon, Nov 20, 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.

Mon, Nov 20, 5:43 AM

Fri, Nov 17

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.

Fri, Nov 17, 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.

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

Thu, Nov 16

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.

Thu, Nov 16, 9:53 AM
peter.smith created D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb.
Thu, Nov 16, 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.

Thu, Nov 16, 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.

Thu, Nov 16, 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?

Thu, Nov 16, 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.

Thu, Nov 16, 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?

Thu, Nov 16, 2:50 AM

Wed, Nov 15

peter.smith created D40076: [builtins][compiler-rt][ARM] re-enable test mulsc3.
Wed, Nov 15, 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:

Wed, Nov 15, 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.

Wed, Nov 15, 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.

Wed, Nov 15, 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.

Wed, Nov 15, 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?

Wed, Nov 15, 2:31 AM

Tue, Nov 14

peter.smith created D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.
Tue, Nov 14, 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.

Tue, Nov 14, 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.

Tue, Nov 14, 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
peter.smith added inline comments to D36742: [LLD][ELF][AArch64] Implement scanner for Cortex-A53 Erratum 843419.
Nov 13 2017, 8:47 AM
peter.smith updated the diff for D36742: [LLD][ELF][AArch64] Implement scanner for Cortex-A53 Erratum 843419.

Thanks very much for the review comments. I've updated the diff to address them.

Nov 13 2017, 8:45 AM

Nov 8 2017

peter.smith updated the summary of D39744: [LLD][ELF][AArch64] Add support for AArch64 range extension thunks..
Nov 8 2017, 9:50 AM
peter.smith added a comment to D39700: [Builtins] Do not use tailcall for Thumb1.

With respect to this review specifically, I think that the change should make compiler-rt no worse and for its likely use case of v6-m, there aren't any non-core registers for the C language functions to corrupt. With respect to the larger problem of many of the other __aeabi_ functions calling C functions I think we should address separately to this Review. I have raised pr35243 to cover the non-compliance https://bugs.llvm.org/show_bug.cgi?id=35243 please feel to comment/correct as necessary.

Nov 8 2017, 3:10 AM

Nov 7 2017

peter.smith added a comment to D39700: [Builtins] Do not use tailcall for Thumb1.

I agree that we must avoid the 16-bit branch on Thumb1 and that the code sequence is fine for that purpose.

I do wonder if compiler-rt should define the aeabi_... forwarding functions in it? I would have expected the C-library to define them. For example the v6-m libgcc.a does not define the aeabi memcpy family, the forwarding is done in newlib. Similarly Arm's proprietary compiler's C library defines aeabi_memcpy that just falls through directly into memcpy. I'm guessing that there is some C-library that we support that does not define the aeabi_ function?

I looked the MUSL C library: it implements aeabi_mem{cpy,move,set,clear}, but misses "aeabi_memcmp".
From the "C Library ABI for the ARM" [1], it only requires some aeabi specific constants or typedefs in libc's header file.
My understanding is, compiler-rt should define complete function sets to support other libc.

[1] http://infocenter.arm.com/help/topic/com.arm.doc.ihi0039d/IHI0039D_clibabi.pdf

Nov 7 2017, 10:34 AM
peter.smith created D39744: [LLD][ELF][AArch64] Add support for AArch64 range extension thunks..
Nov 7 2017, 10:15 AM
peter.smith updated the diff for D36749: [LLD][ELF][AArch64] Complete implementation of -fix-cortex-a53-843419.

Rebase after changes made in D36742.

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

Fix header guard to use the new filename. My apologies for missing the first time.

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

Thanks very much for the comments. I've made the following changes:

  • Changed the name of the files from SectionPatcher to AArch64ErrataFix. The technique for fixing errata isn't unique to AArch64, it has been applied to Arm before in other linkers, but I guess we can rename the file later if it becomes more generic.
  • Removed the A64 struct.
  • Got rid of GetInstr and simplified a bit.
  • Created a function to remove some of the indentation.
Nov 7 2017, 6:46 AM
peter.smith added a comment to D39548: [ELF] Set the section size on the section start symbols.

Currently we generate a special ELF section __cap_relocs containing a capability "relocations" for every global pointer. For historic reasons we don't use ELF relocations but an array of structs that is processed by crt_init_globals() in the initial startup code.
This separate section also has the advantage that we don't need any symbol tables at runtime but can just loop over this array containing addresses resolved at static link time.
We need sizes on every ELF symbol that can be used as a pointer in order to set the correct runtime-bounds on the resulting capability so that we can find all bounds violations.

Nov 7 2017, 4:43 AM
peter.smith added a comment to D39700: [Builtins] Do not use tailcall for Thumb1.

I agree that we must avoid the 16-bit branch on Thumb1 and that the code sequence is fine for that purpose.

Nov 7 2017, 2:27 AM
peter.smith added a comment to D39600: [docs][ARM] Add HowTo for cross compiling and testing compiler-rt builtins on Arm.

Thanks very much for the comments, I agree that there is a lot of scope for improvement and that we can do that over time.

Nov 7 2017, 12:43 AM
peter.smith added a comment to D39600: [docs][ARM] Add HowTo for cross compiling and testing compiler-rt builtins on Arm.

Should we use "ARM" instead of "Arm" ?

Nov 7 2017, 12:41 AM

Nov 6 2017

peter.smith added a comment to D39548: [ELF] Set the section size on the section start symbols.

Can you share a bit more about how CHERI uses the symbol size? Is it only for known linker generated Sections like in your example above, or to support arbitrary user mysection_start symbols? Is the mysection_start symbol special cased within CHERI or is it just an arbitrary ELF Symbol. I'm just a bit curious about the implications of setting the symbol size, for example if this is for special sections like .dynamic we would only expect the _start and _end, and _DYNAMIC symbols to be defined, in an arbitrary mysection_start there could be multiple other symbols defined as well and having the symbol size set to the section size would mean its [0, size) range would overlap all other symbols in the section, this could potentially confuse other ELF processing tools that might skip over functions/data based on Symbol Size. I'm also curious as to how the symbol size is used? Does the OS/loader require the static symbol table to be present, what if it has been stripped?

Nov 6 2017, 6:52 AM
peter.smith updated the diff for D36749: [LLD][ELF][AArch64] Complete implementation of -fix-cortex-a53-843419.

Rebased to account for recent refactorings. No other changes.

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

Rebased to account for recent refactorings. No other changes.

Nov 6 2017, 3:45 AM
peter.smith added a comment to D39600: [docs][ARM] Add HowTo for cross compiling and testing compiler-rt builtins on Arm.

Thanks very much for taking a look.

Nov 6 2017, 3:07 AM

Nov 3 2017

peter.smith created D39600: [docs][ARM] Add HowTo for cross compiling and testing compiler-rt builtins on Arm.
Nov 3 2017, 8:09 AM

Oct 31 2017

peter.smith added a comment to D37585: Add ARM backend support for pagerando.

I'm mostly a linker person so I'm not best qualified to review the backend changes. I've tried to take a look from an overall toolchain perspective.

Oct 31 2017, 8:58 AM
peter.smith added inline comments to D37584: Add target-independent backend modifications for pagerando.
Oct 31 2017, 8:58 AM
peter.smith added inline comments to D37582: Remove pipeline dependencies on StackProtector.
Oct 31 2017, 8:58 AM
peter.smith added inline comments to D37581: Implement pagerando wrapper functions to initialize POT register.
Oct 31 2017, 8:58 AM
peter.smith added a comment to D37580: Add Position Independent Pages (PIP) relocation model.

I've got a few comments on some of the later patches, as this is the first in the series I just thought I'd mention that, as far as I know, adding llvm-commits as a subscriber after the initial creation is not sufficient for the mailing list to get any updates. I think the usual advice is to abandon the existing reviews and start new ones with llvm-commits subscribed. I'd also suggest adding a link to your llvm-dev post http://lists.llvm.org/pipermail/llvm-dev/2017-June/113794.html to at least one of the reviews as it isn't possible to understand without it. Some of the descriptions of later reviews are quite terse, especially for the backend changes that could do with some more description on why you've made the changes that you would like to make.

Oct 31 2017, 8:58 AM

Oct 26 2017

peter.smith added a comment to D34692: [LLD][ELF] Add support for multiple passes to createThunks().
In D34692#907451, @ruiu wrote:

OK, let's land this first. I'm really sorry to take that long to review this patch, I apologize for that. I hoped that I could have refactored Relocations.cpp before landing more code there, but apparently I couldn't be a roadblock anymore for this change. I'll try to reduce complexity of relocation handling after you submit this patch.

Oct 26 2017, 5:39 AM
peter.smith updated the diff for D34689: [LLD][ELF] Pre-create ThunkSections at Target specific intervals.

Strictly speaking we don't need the ThunkSectionSize variable. I think it is worth having ThunkSectionSpacing less than the maximum branch range as it makes it more likely that all the thunks can be created in one pass. Does the value we reduce ThunkSectionSpacing by need to be as large as 16,384 Thunks, most likely not. It is a very conservative upper bound. I can change it but it will mean updating some of the tests in later approved reviews so I'd prefer not to do it here.

Oct 26 2017, 5:34 AM

Oct 24 2017

peter.smith added a comment to D39152: ELF: Add support for emitting dynamic relocations in the Android relocation packing format..

Thanks for the update, I don't have any particularly strong views about the detailed comments so unless anyone else strongly agrees, I'm happy to keep with what you have. I don't have any further suggestions at the moment.

Oct 24 2017, 3:05 AM

Oct 23 2017

peter.smith created D39179: [AArch64] Fix PR34625 -mtune without -mcpu should not set -target-cpu.
Oct 23 2017, 5:31 AM

Oct 22 2017

peter.smith added a comment to D39152: ELF: Add support for emitting dynamic relocations in the Android relocation packing format..

I think that this is worthwhile putting into the linker as relocations can be sorted to optimise packing and make it easier to account for address changes to the rest of the ELF file.

Oct 22 2017, 1:39 AM

Oct 13 2017

peter.smith added a comment to D38588: Clear LastMappingSymbols and LastEMS(Info) when resetting the ARM(AArch64)ELFStreamer.

Personally I would be happy with the change without a heavyweight test.

I'm mainly not sure what I'm expected to do. E.g. I wasn't sure if the previous comment is about we don't need a test or a guide for how to add such a test. I added it mainly because removing the tests later is significantly easier.

I don't have a particularly good answer here. If a test could be a maintenance burden due to a possibly unstable API then it may not be worthwhile if the change it is testing is on the safe side. I'm hoping for input from a code owner to see whether they think introducing a UnitTest is worth it.

Oct 13 2017, 10:12 AM