Page MenuHomePhabricator

peter.smith (Peter Smith)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Thu, Jan 17

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

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

Thu, Jan 17, 1:26 AM

Wed, Jan 16

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

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

Wed, Jan 16, 8:55 AM

Tue, Jan 15

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

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

Tue, Jan 15, 3:37 AM

Mon, Jan 14

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

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

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

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

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

Thu, Jan 10

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

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

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

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

Thu, Jan 10, 8:14 AM

Wed, Jan 9

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

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

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

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

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

Tue, Jan 8

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

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

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

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

Tue, Jan 8, 3:35 AM

Mon, Jan 7

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

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

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

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

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

Generally looking good.

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

Mon, Jan 7, 3:19 AM

Wed, Jan 2

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

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

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

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

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

Wed, Jan 2, 11:13 AM

Dec 20 2018

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

LGTM as well

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

LGTM as well

Dec 20 2018, 1:51 AM

Dec 18 2018

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

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

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

I have several reservations about doing this:

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

Dec 17 2018

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

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

Dec 17 2018, 3:54 AM

Dec 14 2018

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

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

Dec 14 2018, 3:25 AM

Dec 13 2018

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

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

Dec 13 2018, 2:10 AM

Dec 12 2018

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

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

Dec 12 2018, 12:59 AM

Dec 11 2018

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

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

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

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

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

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

Dec 11 2018, 3:52 AM

Dec 10 2018

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

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

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

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

Dec 10 2018, 3:37 AM

Dec 7 2018

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

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

Dec 7 2018, 5:54 AM

Dec 6 2018

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

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

Dec 6 2018, 9:19 AM

Dec 3 2018

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

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

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

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

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

Nov 30 2018

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

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

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

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

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

LGTM. Please commit.

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

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

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

Nov 30 2018, 2:27 AM

Nov 29 2018

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

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

Nov 29 2018, 2:34 AM

Nov 28 2018

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

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

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

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

Nov 28 2018, 3:11 AM

Nov 27 2018

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

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

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

Thanks, superfluous parentheses removed.

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

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

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

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

Nov 27 2018, 2:05 AM

Nov 23 2018

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

Nov 22 2018

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

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

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

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

Nov 22 2018, 1:40 AM

Nov 21 2018

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

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

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

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

Nov 21 2018, 3:00 AM
peter.smith created D54786: [LLD][ARM] Change REQUIRES: ARM to Requires: arm.
Nov 21 2018, 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.
Nov 21 2018, 2:32 AM

Nov 20 2018

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

Nov 19 2018

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.

Nov 19 2018, 8:48 AM

Nov 16 2018

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.

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

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

Nov 13 2018

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.

Nov 13 2018, 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.

Nov 13 2018, 7:21 AM
peter.smith created D54474: [LLD][AArch64] Fix resolution of R_PLT_PAGE RelExpr .
Nov 13 2018, 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.

Nov 13 2018, 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:

Nov 13 2018, 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.

Nov 13 2018, 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).

Nov 13 2018, 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).

Nov 13 2018, 2:42 AM

Nov 12 2018

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.

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

Uploaded rebased diff, no functional changes.

Nov 12 2018, 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
Nov 12 2018, 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.

Nov 12 2018, 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?

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

Thanks for the update. Looks good to me.

Nov 12 2018, 3:21 AM

Nov 9 2018

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

I 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.

Nov 9 2018, 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.

Nov 9 2018, 3:12 AM

Nov 8 2018

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).

Nov 8 2018, 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.

Nov 8 2018, 6:19 AM

Nov 7 2018

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).

Nov 7 2018, 6:26 AM

Nov 6 2018

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.

Nov 6 2018, 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.

Nov 6 2018, 7:07 AM

Nov 5 2018

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.

Nov 5 2018, 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.

Nov 5 2018, 2:25 AM

Nov 1 2018

peter.smith added a comment to D53906: [ARM][AArch64] Increase TLS alignment to reserve space for Android's TCB.

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).

Nov 1 2018, 3:32 AM

Oct 31 2018

peter.smith added a comment to D53906: [ARM][AArch64] Increase TLS alignment to reserve space for Android's TCB.

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?

Oct 31 2018, 5:19 AM

Oct 30 2018

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.

Oct 30 2018, 3:59 AM

Oct 23 2018

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)

Oct 23 2018, 5:19 AM
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