peter.smith (Peter Smith)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

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

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

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

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

Thu, Nov 22, 1:40 AM

Yesterday

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

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

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

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

Wed, Nov 21, 3:00 AM
peter.smith created D54786: [LLD][ARM] Change REQUIRES: ARM to Requires: arm.
Wed, Nov 21, 2:50 AM
peter.smith updated the diff for D54759: [LLD][ELF] Use more specific method to calculate DT_PLTRELSZ.

Thanks very much for the comments. I agree that this is a somewhat niche use case, most likely embedded systems related where someone has built their own loader. I've updated the diff with the following changes:

  • The ARM test passed with your updated changes because it was always being skipped! The REQUIRES: ARM should have been REQUIRES: arm. There are a couple of other tests that have that pattern that I'll fix up later today.
  • I've simplified the test for the non-Android Arm case in a simpler way (just check the In.RelaIplt name isn't .rel.dyn)
  • I've inlined the function and updated the comments in the test as suggested.
Wed, Nov 21, 2:32 AM

Tue, Nov 20

peter.smith created D54759: [LLD][ELF] Use more specific method to calculate DT_PLTRELSZ.
Tue, Nov 20, 8:46 AM

Mon, Nov 19

peter.smith added a comment to D54422: [ELF] - Do not ICF two sections with different output sections when using linker scripts.

I agree with James here. I strongly suspect that in systems where merging content is a problem such as embedded systems with overlays or only a subset of memory available for booting there may be little correlation between the input section names chosen by the compiler and that given to the output section.

Mon, Nov 19, 8:48 AM

Fri, Nov 16

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

I've updated the patch to inline the call for _GLOBAL_OFFSET_TABLE_ so that addOptionalRegular remains unchanged. The cast to defined can be removed if/when D54627 lands.

Fri, Nov 16, 6:11 AM
peter.smith added a comment to D54627: [ELF] - Make SymbolTable::addDefined return Defined..

This looks fine to me. It makes sense for addDefined to actually return a Defined.

Fri, Nov 16, 5:51 AM
peter.smith created D54624: [LLD][ELF] Error if _GLOBAL_OFFSET_TABLE_ is defined in input objects.
Fri, Nov 16, 3:12 AM

Tue, Nov 13

peter.smith updated the diff for D54474: [LLD][AArch64] Fix resolution of R_PLT_PAGE RelExpr .

Thanks, for the comments. I've updated the diff to incorporate them.

Tue, Nov 13, 9:44 AM
peter.smith added a comment to D54314: [ELF] - Fix R_AARCH64_ADR_GOT_PAGE, R_AARCH64_LD64_GOT_LO12 handling against IFUNC symbols..

Created D54474 for the related non -fpic segfault.

Tue, Nov 13, 7:21 AM
peter.smith created D54474: [LLD][AArch64] Fix resolution of R_PLT_PAGE RelExpr .
Tue, Nov 13, 7:20 AM
peter.smith added a comment to D54314: [ELF] - Fix R_AARCH64_ADR_GOT_PAGE, R_AARCH64_LD64_GOT_LO12 handling against IFUNC symbols..

It looks like the handling for R_PLT_PAGE_PC is broken. It is handled exactly as R_PAGE_PC which always uses Sym.getVA(A). I think it should be using (Sym.getPltVA() + A). I'll submit a separate patch for this as it is unrelated.

Tue, Nov 13, 5:41 AM
peter.smith added a comment to D54314: [ELF] - Fix R_AARCH64_ADR_GOT_PAGE, R_AARCH64_LD64_GOT_LO12 handling against IFUNC symbols..

Tracing the segfault in the example:

Tue, Nov 13, 3:47 AM
peter.smith added a comment to D54314: [ELF] - Fix R_AARCH64_ADR_GOT_PAGE, R_AARCH64_LD64_GOT_LO12 handling against IFUNC symbols..

Yes it is the same one as reported there. I think you are most likely right that it is independent of this particular change.

Tue, Nov 13, 2:49 AM
peter.smith added a comment to D54145: [ELF] - Fix R_X86_64_GOTPCRELX/R_X86_64_REX_GOTPCRELX when target is IFUNC..

My understanding is that the .igot, .igot.plt come from the bfd linker script.

.got            : { *(.got) *(.igot) }
. = DATA_SEGMENT_RELRO_END (SIZEOF (.got.plt) >= 24 ? 24 : 0, .);
.got.plt        : { *(.got.plt)  *(.igot.plt) }

As you can see they are logically part of the .got with the .igot used to make sure the ifunc resolver entries happen last (and are resolved last).

Tue, Nov 13, 2:48 AM
peter.smith added a comment to D54314: [ELF] - Fix R_AARCH64_ADR_GOT_PAGE, R_AARCH64_LD64_GOT_LO12 handling against IFUNC symbols..

I've tested the patch against the original C++ example in D54145 and that works ok. I get a segfault when I remove -fPIC though. It does work correctly when I compile with clang but link with ld.bfd (From binutils 2.25).

Tue, Nov 13, 2:42 AM

Mon, Nov 12

peter.smith updated the diff for D45962: [MC] Use local MCSubtargetInfo in writeNops.

Rebased, and added support for WebAssembly and RiscV as my default build configuration didn't include them at the time. No other changes.

Mon, Nov 12, 10:08 AM
peter.smith updated the diff for D45961: [MC] Add MCSubtargetInfo to MCAlignFragment [NFC].

Uploaded rebased diff, no functional changes.

Mon, Nov 12, 9:41 AM
peter.smith added a comment to D45960: [MC] Add MCSubtargetInfo to MCPaddingFragment [NFC].

Yes, my apologies for not following up.
This is an enabling patch for allowing writeNops() to use a passed in Subtarget. There are 3 parts:

  • Add STI to MCPaddingFragment (This review)
  • Add STI to MCAlignFragment D45961
  • Actually use the STI in the fragments to pass to writeNops() D45962
Mon, Nov 12, 7:26 AM
peter.smith accepted D53980: [ARM, AArch64] Move ARM/AArch64 target parsers into separate files to enable future changes..

Thanks for the update. This looks good to me. May be worth waiting till tomorrow before committing to give the US timezone a last chance to object.

Mon, Nov 12, 4:11 AM
peter.smith added a comment to D54314: [ELF] - Fix R_AARCH64_ADR_GOT_PAGE, R_AARCH64_LD64_GOT_LO12 handling against IFUNC symbols..

I wasn't sure if this was to be applied on top of D54154 or not so I merged the two and ended up with a test failure in the test modifications done in ELF/gotpc-relax.s for D54154.

I merged this one with D54145 today manually and run the test suite. It does not fail for me.
They are fully independent I beleive, so it is what I expected.
Here is the merged version I got:

I think just something went wrong on your side perhaps?

Mon, Nov 12, 4:03 AM
peter.smith accepted D54265: [DAGCombiner] Fix load-store forwarding of indexed loads..

Thanks for the update. Looks good to me.

Mon, Nov 12, 3:21 AM

Fri, Nov 9

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

I wasn't sure if this was to be applied on top of D54154 or not so I merged the two and ended up with a test failure in the test modifications done in ELF/gotpc-relax.s for D54154.

Fri, Nov 9, 8:40 AM
peter.smith added a comment to D54265: [DAGCombiner] Fix load-store forwarding of indexed loads..

This is looking good. I've got one style nit that I'll leave to your discretion. I think it is probably worth tidying up the test case a little more though. I've made a proposal in the comment.

Fri, Nov 9, 3:12 AM

Thu, Nov 8

peter.smith added a comment to D54265: [DAGCombiner] Fix load-store forwarding of indexed loads..

Thanks very much for the quick update. I can confirm that the original test file compiles successfully. I'll take a look at the change tomorrow (need to leave office).

Thu, Nov 8, 10:22 AM
peter.smith added a comment to D54145: [ELF] - Fix R_X86_64_GOTPCRELX/R_X86_64_REX_GOTPCRELX when target is IFUNC..

In my example I was using -fpic , without it then we get a different relocation type R_AARCH64_ADR_PREL_PG_HI21 (with -fpic it is R_AARCH64_ADR_GOT_PAGE), I suspect that is why I didn't see the IRELATIVE relocation. I don't mind if we keep this patch to be specific to non-AArch64, but if we do the we either shouldn't close the PR until we've made it work, or raise another PR to make sure we don't forget about it.

Thu, Nov 8, 6:19 AM

Wed, Nov 7

peter.smith added a comment to D49200: [DAGCombine] Improve Load-Store Forwarding.

This commit (found via git bisect) looks like it is the cause of https://bugs.llvm.org/show_bug.cgi?id=39571 which is derived from the Linux kernel and is a regression from LLVM 7.0. We get an internal fault in instruction selection for ARM state for pre ARM-v7 architectures (test uses armv4t but it is reproducible on v5t and v6).

Wed, Nov 7, 6:26 AM

Tue, Nov 6

peter.smith added a comment to D54145: [ELF] - Fix R_X86_64_GOTPCRELX/R_X86_64_REX_GOTPCRELX when target is IFUNC..

I've tested this out on X86, Arm and AArch64 and it works for X86 and Arm. Unfortunately it doesn't seem to work with AArch64 yet. I don't see any R_AARCH64_IRELATIVE relocations.

I haven't found out why as yet. I'll do some digging to see if there is anything obvious that we are missing. If you have access to a X86 Linux box this is reproducible with a GCC aarch64-linux-gnu cross compilation toolchain and the linux user mode emulator qemu-aarch64.

Thanks! My quick guess is that perhaps some new Expr is passed to toPlt(RelExpr Expr) for AArch64 case and hence condition if (needsPlt(Expr) && !Sym.isInPlt()) is not satisfied.
I'll try to take a look at what is going on there during this week.

Tue, Nov 6, 7:31 AM
peter.smith added a comment to D54145: [ELF] - Fix R_X86_64_GOTPCRELX/R_X86_64_REX_GOTPCRELX when target is IFUNC..

I've tested this out on X86, Arm and AArch64 and it works for X86 and Arm. Unfortunately it doesn't seem to work with AArch64 yet. I don't see any R_AARCH64_IRELATIVE relocations.

Tue, Nov 6, 7:07 AM

Mon, Nov 5

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

Apologies for the delay in responding, I've taken a look in a bit more detail at how AArch64 specifies the FPU.

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

I've no objections to splitting the headers apart, may be worth adding some x86, amdgpu folks on the review list with respect to my comment about splitting out their targets too.

Mon, Nov 5, 2:25 AM

Thu, Nov 1

peter.smith added a comment to D53906: [ELF] Allow configuring the TLS layout for an Android executable.

Increasing the alignment of the TLS segment is an interesting idea as it allows the possibility of a larger TCB and in theory could also be used by the loader to detect compatible ELF files (reject those without the alignment).

Thu, Nov 1, 3:32 AM

Wed, Oct 31

peter.smith added a comment to D53906: [ELF] Allow configuring the TLS layout for an Android executable.

Just trying to understand the implications here. Unfortunately I find I can't keep the details in my memory for more than the time I have to work on it. Can you correct me where I'm wrong?

Wed, Oct 31, 5:19 AM

Tue, Oct 30

peter.smith added a comment to D53864: [ELF] - Do not crash when -r output uses linker script with `/DISCARD/`.

I can confirm that this change does fix the problem observed in pr39493.

Tue, Oct 30, 3:59 AM

Tue, Oct 23

peter.smith added a comment to D53486: [libcxx] Only define __libcpp_is_floating_point<_Float16> for Clang.

For what it's worth a colleague pointed me at the GCC commit message https://patchwork.ozlabs.org/patch/638659/ there is a brief mention of C++ (search for C++ note)

Tue, Oct 23, 5:19 AM

Oct 23 2018

peter.smith added a comment to D53486: [libcxx] Only define __libcpp_is_floating_point<_Float16> for Clang.

To the best of my knowledge (references in the docs below) the _Float16 is defined in a TS: "ISO/IEC, Floating point extensions for C, ISO/IEC TS 18661-3".
I got that by following the links from the Arm C Library Extensions that refer to an older storage only type called __fp16 and its interaction with _Float16 (Links below)

Oct 23 2018, 2:57 AM

Oct 22 2018

peter.smith added a comment to D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.

Added LLVM_FALLTHROUGH; in r344890.

Oct 22 2018, 3:43 AM
peter.smith added a comment to D53437: Schedule Hot Cold Splitting pass after most optimization passes.

It looks like the test is failing on some of the Arm buildbots. In particular some of them don't have the X86 backend: http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/4611 so it looks like it is most likely missing a REQUIRES: x86. Can you take a look?

Oct 22 2018, 2:56 AM
peter.smith added a comment to D53444: Support ARM_V4BX relocation.

This change looks fine to me. Just to check what CPU you are intending to run the output on. The LLD interworking, Thunk and PLT support assumes the presence of the BLX instruction which is only present on ARMv5 CPUs and above (LLD will warn if it doesn't detect at least one object compiled for ARMv5 or above).

Oct 22 2018, 2:20 AM

Oct 19 2018

peter.smith added a comment to D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.

Thanks for pointing that out, I'm out of office today, will look at describing the intention to fall through when I get back in on Monday.

Oct 19 2018, 12:10 AM

Oct 17 2018

peter.smith added a comment to D49992: [ELF][ARM] Add Arm ABI names for float ABI ELF Header flags.

hey there, I've run into problems with stripping static-libs on arm when using llvm/clang-7. Could you imagine this patch being at fault?

strip: armv7a-unknown-linux-gnueabihf-strip --strip-unneeded -R .comment -R .GCC.command.line -R .note.gnu.gold-version
    lib/libbz2.so.1.0.6
    usr/bin/bzip2recover
    bin/bzip2
    usr/lib/libbz2.a
 armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11
 armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11

Section 11 is: [11] .llvm_addrsig LOOS+0xfff4c03 00000000 003de4 000002 00 E 0 0 1

this didn't happen with llvm/clang-6, and the patch got commited within the fitting time frame?

Oct 17 2018, 10:18 AM
peter.smith added a comment to D45240: [ARM] Compute a target feature which corresponds to the ARM version..

hey there, I've run into problems with stripping static-libs on arm when using llvm/clang-7. Could you imagine this patch being at fault?

strip: armv7a-unknown-linux-gnueabihf-strip --strip-unneeded -R .comment -R .GCC.command.line -R .note.gnu.gold-version
    lib/libbz2.so.1.0.6
    usr/bin/bzip2recover
    bin/bzip2
    usr/lib/libbz2.a
 armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11
 armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11
Oct 17 2018, 9:19 AM

Oct 16 2018

peter.smith accepted D53272: Add target requirement to profile remap test..

Looks good to me. REQUIRES: x86-registered-target is used in the same directory for tests that depend on specific targets.

Oct 16 2018, 1:38 AM

Oct 15 2018

peter.smith updated the diff for D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.

Updated diff to reflect review comments. Main changes are:

  • isArmBigEndian always returns false if the target architecture isn't Arm.
  • Added tests to make sure "--be8" doesn't get added by mistake (would have been in previous patch for aarch64_be arch with -mbig-endian flag.
Oct 15 2018, 5:45 AM
peter.smith added a comment to D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.

Thanks very much for the comments. I'll post an update shortly.

Oct 15 2018, 5:42 AM

Oct 12 2018

peter.smith updated the diff for D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.

I've decided to roll the linker changes in with the assembler ones as the linker use case affects the design. It turns out that only Arm needs to check to see if the -mbig-endian and -mlittle-endian flags override the triple as computeTargetTriple() accounts for them for AArch64.

Oct 12 2018, 6:24 AM
peter.smith retitled D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker from [ARM][AArch64] Pass through endianness flags to the GNU assembler to [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.
Oct 12 2018, 6:16 AM
peter.smith accepted D53121: [Driver] Add defaults for Android ARM FPUs..

Looks good to me. By generic target I just meant --target=arm-linux-androideabi21 with a -march to specify the revision, which you've got in the test.

Oct 12 2018, 1:47 AM

Oct 11 2018

peter.smith added a comment to D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.

On reflection after looking at what would be needed for the linker tools::gnutools::Linker::ConstructJob() I think it may be better to move the triple detection into a separate function. I'll work on that and will hopefully post an update soon.

Oct 11 2018, 8:30 AM
peter.smith added a comment to D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.

Thanks for the comments. I agree with you that the -EB and -EL flags need to be passed to the linker as well, I kind of expected that ld.bfd would infer it from the endianness of the first object but it doesn't seem to do that. If it's ok I'll do that in a separate patch?

Oct 11 2018, 7:21 AM
peter.smith added a comment to D53121: [Driver] Add defaults for Android ARM FPUs..

It looks like we might be able to simplify a bit, and I think there probably could be some more test cases but no objections from me.

Oct 11 2018, 4:58 AM

Oct 10 2018

peter.smith added a comment to D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.

Ping. Does anyone have any changes they'd like me to make?

Oct 10 2018, 10:02 AM

Oct 9 2018

peter.smith added a comment to D53003: [ELF] Fix link failure with Android compressed relocation support..

FWIW: I fixed a similar issue in the LLVM assembler with the exception table: https://reviews.llvm.org/D42722

I wonder if a non-contrived input could require more than 10 rounds to converge. It seems unlikely to happen, at least.

This change seems fine to me.

Oct 9 2018, 2:39 AM

Oct 5 2018

peter.smith updated the diff for D52834: [ARM] Account for implicit IT when calculating inline asm size.

Updated diff to make the second run: command actually run and test. My apologies for missing that.

Oct 5 2018, 1:22 AM

Oct 4 2018

peter.smith updated the diff for D52834: [ARM] Account for implicit IT when calculating inline asm size.

Thanks for the comments. I've made an attempt at a diff where the MaxInstLength is increased to 6. This moves all the significant code to getInstSizeInBytes(). I've applied the change to all Targets (Elf, Macho, Windows ...) as it is possible to use implicit-IT on all of them.

Oct 4 2018, 6:00 AM

Oct 3 2018

peter.smith created D52834: [ARM] Account for implicit IT when calculating inline asm size.
Oct 3 2018, 9:40 AM

Oct 2 2018

peter.smith added a comment to D51854: [Arm builtins] Remove non-necessary IS check.

Thanks Peter! As I don't have commit access, can I ask for someone to commit this on my behalf?

I'll do it tomorrow morning if no-one else beats me to it.

I don't mind doing it.

Oct 2 2018, 9:30 AM
peter.smith updated the diff for D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.

Updated diff to add more tests, including some that use -mlittle-endian when the target is big-endian.

Oct 2 2018, 8:45 AM
peter.smith added a comment to D51854: [Arm builtins] Remove non-necessary IS check.

Thanks Peter! As I don't have commit access, can I ask for someone to commit this on my behalf?

Oct 2 2018, 8:25 AM
peter.smith added a comment to D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.

Hi Peter,

Looks ok. Why did you need to change all cmdlines to have -EL? I imagine you just need one for each case, everything else remains the default (which should still work).

I thought that in general we wouldn't know what the default endianness of the assembler we are targeting is so it was safest to be explicit. For a cross-toolchain it is probably inherent in the name, but in theory we could be running clang on a native big-endian system where the assembler is just called as.

Oct 2 2018, 8:20 AM
peter.smith accepted D51854: [Arm builtins] Remove non-necessary IS check.

Looks good to me, thanks for the update.

Oct 2 2018, 8:07 AM
peter.smith created D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.
Oct 2 2018, 8:06 AM

Oct 1 2018

peter.smith added a comment to D52705: First-pass at ARM hard/soft-float multilib driver (NOT WORKING YET).

I think that you may be better off posting a RFC to llvm-dev to get discussion on the best approach here. It would also be good to step back a bit and consider the requirements, as it stands it looks like this might be a solution for just one particular multilib layout and we might be able to get some input on some other use cases that others may have. I personally would like to enable clang to be able to query a gcc installation to find out (and potentially cache) its multlib directories as I've been told for the Arm embedded toolchain that the directory structure is not fixed and may change at any point in the future.

Oct 1 2018, 4:15 AM
peter.smith added reviewers for D52149: add support functions for hard/soft float multilib distinction: compnerd, rengolin.

I've added another couple of reviewers.

Oct 1 2018, 3:26 AM

Sep 28 2018

peter.smith added a comment to D18086: Fix default processor name for armv6k..

I've committed D52594 and D52595. I can't abandon this revision as I didn't start it, but logically this review is closed.

Sep 28 2018, 2:10 AM

Sep 27 2018

peter.smith added a comment to D18086: Fix default processor name for armv6k..

Thanks. I'll commit the other two tomorrow when I get into work and close this one out.

Sep 27 2018, 8:54 AM
peter.smith created D52606: [LLD][COFF] Add missing Requires x86 to fix buildbot.
Sep 27 2018, 5:02 AM
peter.smith added a comment to D18086: Fix default processor name for armv6k..

I've created D52594 LLVM and D52595 dependent clang test update with my suggested changes. I can't upload the diff to this review unfortunately.

Sep 27 2018, 3:26 AM
peter.smith added a dependent revision for D52594: [ARM] Remove non-existent cpu arm1176j-s and use mpcore for v6k: D52595: [ARM] Alter test to account for change to armv6k default CPU.
Sep 27 2018, 3:21 AM
peter.smith added a dependency for D52595: [ARM] Alter test to account for change to armv6k default CPU: D52594: [ARM] Remove non-existent cpu arm1176j-s and use mpcore for v6k.
Sep 27 2018, 3:21 AM
peter.smith created D52595: [ARM] Alter test to account for change to armv6k default CPU.
Sep 27 2018, 3:21 AM
peter.smith created D52594: [ARM] Remove non-existent cpu arm1176j-s and use mpcore for v6k.
Sep 27 2018, 3:13 AM

Sep 26 2018

peter.smith added a comment to D52525: [AArch64] Fix range check of R_AARCH64_TLSLE_ADD_TPREL_HI12.

This change looks correct to me. The docs link is http://arminfo.emea.arm.com/help/topic/com.arm.doc.ihi0056b/IHI0056B_aaelf64.pdf search for relocation code 549 in Table 4-18, Local Exec TLS relocations. I've checked the other TPREL relocations that LLD has implemented and they are all of the NC (non range checked) kind so it looks like there aren't any others of this form.

Sep 26 2018, 2:09 AM

Sep 25 2018

peter.smith added a reviewer for D52144: use __ARM_FP instead of __VFP_FP__: compnerd.

I've checked with the gcc sources and it is VFP_FP is unconditionally set by gcc. This might be a relatively recent change so I suspect that clang will have been following early behaviour of gcc. It will be worth updating the tests to use __ARM_FP as well. It will also be worth working out why the tests didn't fail for you as well. I'd only expect to see a difference in behaviour if you were using gcc without floating point support though.

Sep 25 2018, 3:18 AM

Sep 20 2018

peter.smith added a comment to D51854: [Arm builtins] Remove non-necessary IS check.

Ping! I do not mind updating the patch to use 1 instruction instead of 2, as Peter said, if anyone prefers that.

Sep 20 2018, 8:45 AM

Sep 19 2018

peter.smith accepted D50297: Align AArch64 and i386 image base to superpage.

Thanks for the updates. LGTM.

Sep 19 2018, 8:04 AM

Sep 18 2018

peter.smith added a comment to D52156: [LLD] [COFF] Alternative ARM range thunk algorithm.

Thanks for the updates. I don't have any more comments.

Sep 18 2018, 11:13 AM

Sep 17 2018

peter.smith added a comment to D52156: [LLD] [COFF] Alternative ARM range thunk algorithm.

My best suggestion for a test that will need exceed the margin is to use input sections with high alignment requirements. If these are on a natural boundary prior to adding thunks then adding a thunk will result in a large amount of bytes inserted to maintain section alignment. As long as these padding bytes are in between the source and destination of a branch and exceed the margin then a test case can be constructed without needing to insert hundreds of Thunks.

Sep 17 2018, 6:08 PM
peter.smith added a comment to D51089: [LLD] [COFF] Add support for creating range extension thunks for ARM.

Interesting. I think that the implementation you have here will converge faster as it makes it more likely that pass 1 has all the thunks, at the expense of potentially generating more thunks than is strictly required. However if the goal is simplicity I think that you'll need to do everything in one pass, and accept that there will be corner cases that might not link if the margin isn't sufficient. For ELF and arbitrary linker scripts I thought the chance of failure too high, for COFF the chance of failure may be low enough. I think the most likely edge case in COFF will be the presence of sections with high alignment requirements as inserting thunks could cause a lot of bytes of alignment padding to be added.

Sep 17 2018, 4:53 PM
peter.smith added a comment to D51089: [LLD] [COFF] Add support for creating range extension thunks for ARM.

Interesting. I think that the implementation you have here will converge faster as it makes it more likely that pass 1 has all the thunks, at the expense of potentially generating more thunks than is strictly required. However if the goal is simplicity I think that you'll need to do everything in one pass, and accept that there will be corner cases that might not link if the margin isn't sufficient. For ELF and arbitrary linker scripts I thought the chance of failure too high, for COFF the chance of failure may be low enough. I think the most likely edge case in COFF will be the presence of sections with high alignment requirements as inserting thunks could cause a lot of bytes of alignment padding to be added.

Sep 17 2018, 4:19 PM
peter.smith added a comment to D51089: [LLD] [COFF] Add support for creating range extension thunks for ARM.

If I understand it, to get a test case we'd need to have a branch that is just in range (including margin) such that no thunk is generated, but adding sufficient thunks causes that branch to go out of range. The brute force way to do it would be to generate greater than (margin/thunk-size) thunks but even with macros that would be a large tedious test to write. One possibility in ELF that I don't know would transition into COFF is to have some sections with high alignment so that inserting a thunk could displace one of these sections off an alignment boundary and hence add much more size than just the Thunk.

Sep 17 2018, 3:34 PM
peter.smith added a comment to D51792: [AArch64] Attempt to parse expressions as adr/adrp operands.

Yes I'm happy with this too.

Sep 17 2018, 2:32 PM
peter.smith added a comment to D50297: Align AArch64 and i386 image base to superpage.

Does FreeBsd use 4Kb granules for the page size or the LLD default of 64Kb. From what I can see from the MMU documentation when 64Kb pages are used the equivalent of the Super Page size is 512MiB and not 2 MiB? If I'm right then aligning to 2MiB is only going to be beneficial when 4Kb pages are used. On the assumption that it doesn't do much harm to people using 64Kb pages I don't have any objections in changing it though. Aligning to 512MiB for 64kb pages sounds a bit extreme though.

Sep 17 2018, 2:30 PM
peter.smith added a reviewer for D50297: Align AArch64 and i386 image base to superpage: srhines.

I've added Stephen Hines as a reviewer to see if this affects Android in a significant way. I'd like to run this by a few people in the OS community and also check that are no adverse code/file size increases as these could be significant on low-end mobile devices. Will hopefully get back to you soon.

Sep 17 2018, 8:40 AM

Sep 14 2018

peter.smith added a comment to D18086: Fix default processor name for armv6k..

If no-one else beats me to it, I'll take a look when I get back from a conference (24th September).

Sep 14 2018, 6:13 AM
peter.smith added a comment to D18086: Fix default processor name for armv6k..

I think the natural default CPU for ARMv6K is the "mpcore". The K stands for kernel extensions and includes the load,store and clear exclusive instructions but otherwise no effect on code generation. I would expect that ARMv6KZ includes the instructions for trustzone such as SMC (secure monitor call) but again, this doesn't have an effect on code generation.

Sep 14 2018, 5:56 AM
peter.smith added a comment to D18086: Fix default processor name for armv6k..

There isn't a processor called ARM1176J-S, only ARM1176JZ-S and ARM1176JZF-S. I've already left a comment about J and S. Z is for Trustzone and F is for hardware floating point. There isn't a good external source for all the possible arm 11 CPU names unfortunately. There is a page of trademarks that contains all the ones that I know about https://www.arm.com/company/policies/trademarks/arm-trademark-list/arm11-trademark.

Sep 14 2018, 2:16 AM

Sep 13 2018

peter.smith created D52038: [LLD][ELF][AArch64] Guard --fix-cortex-a53-843419 against --just-syms.
Sep 13 2018, 7:59 AM

Sep 12 2018

peter.smith added a comment to D51089: [LLD] [COFF] Add support for creating range extension thunks for ARM.

Sorry for the belated response. I was thinking of this patch for a while.

Every time I saw the code of thunk range extension, I wonder if we really need this multi-pass algorithm which add thunks iterative on each pass. I believe in almost all cases, the algorithm finishes on the first iteration, if we allow a very small margin when determining "reachability". As long as a margin is small, size increase by allowing a margin should be negligible.

For pathetic executables for which we need to generate tons of thunks (which enlarges distance between callers and callees and thus need multiple passes with the current algorithm), we can simply discard everything that we made in the previous iteration instead of keeping them, double the margin, and then try again from scratch. In practice, I believe that fallback doesn't happen too frequently.

What do you think of the algorithm? If it works, I prefer that algorithm because discarding everything and redo with a larger margin is simpler than keeping thunks created in previous passes.

Sep 12 2018, 8:24 AM
peter.smith added a comment to D51089: [LLD] [COFF] Add support for creating range extension thunks for ARM.

The algorithm looks good to me. For ELF I preferred to give the Thunk a name related to the destination as it makes it a bit easier to follow disassembled binaries, but it is not essential.

Sep 12 2018, 6:48 AM

Sep 10 2018

peter.smith added a comment to D51792: [AArch64] Attempt to parse expressions as adr/adrp operands.

I don't have much to add over Eli's existing comments. I think the general form of (symA - symB) + constant may not be handled well for all instructions though.

Sep 10 2018, 8:46 AM
peter.smith added a comment to D51854: [Arm builtins] Remove non-necessary IS check.

I agree that we should be using APSR_nzcvq rather than CPSR_f as both assemble to the same instruction encoding (they disassemble to APSR_nzcvq). I think it is worth seeing if there are any objections to removing the case for ARM state msr CPSR_f, #APSR_C as it saves one instruction. Overall looking good to me though.

Sep 10 2018, 5:59 AM

Sep 3 2018

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

Committed on behalf of Bharathi r341312.

Sep 3 2018, 5:38 AM

Aug 31 2018

peter.smith added a comment to D51536: [LLD] Do not set alignment to entsize for mergable sections if entsize is not a power of 2.

Changes look good to me, although I'd be happy to remove the max. The only reason I can think of for raising the alignment based on sh_entsize is that it would mean that (align n, shentsize s) could be merged into the same section (align m where m < n, shentsize s) but I'm struggling to think of a case where this would be significant. I think that if the compiler has taken advantage of the alignment of a data element or string in a SHF_MERGE section it sets the alignment of the section.

Aug 31 2018, 8:28 AM

Aug 30 2018

peter.smith added a comment to D48792: [ARM] Set execute-only flags in .text..

Thanks for the update. I still think this looks good to me.

Aug 30 2018, 5:40 AM
peter.smith closed D51413: [zorg] Disable sanitzers on clang-cmake-armv8-lld.

Committed r341059.

Aug 30 2018, 5:28 AM
peter.smith accepted D42930: [Aarch64] Fix linker emulation for Aarch64 big endian.

I've been referred back to this by a colleague trying to build with big-endian clang with binutils and is seeing an error message out of binutils. I've checked that binutils from the initial commit always used aarch64linuxb and I've verified that I cannot build a aarch64_be-linux-gnu executable with binutils without this change, and I can with this change.

Aug 30 2018, 2:41 AM