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 (233 w, 2 d)

Recent Activity

Apr 16 2020

peter.smith added a comment to D78301: [ARM][MC][Thumb] Revert relocation for some pc-relative fixups..

That is my understanding too from my comment in https://reviews.llvm.org/D72892#1923502
I have removed the tests from LLD that used the relocations coming from these instructions and have replaced them with .inst and .reloc. Will be worth making sure that ninja check-lld passes with this change applies. I'm happy for this to be applied. Will need to check with the other reviewers to make sure they are happy.

Apr 16 2020, 8:21 AM · Restricted Project

Feb 21 2020

peter.smith added a comment to D74927: [MC][ARM] make Thumb function also if type attribute is set.

I think this is close, but there are a couple of test failures with ninja check-llvm

Feb 21 2020, 5:47 AM · Restricted Project

Feb 18 2020

peter.smith accepted D74604: [LLD][ELF][ARM] Fix support for SBREL type relocations.

Thanks for the update. Apart from the extra / needed in some of the comment strings this looks good to me. Happy for you to add those prior to commit.

Feb 18 2020, 7:12 AM · Restricted Project, lld

Feb 17 2020

peter.smith added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

Just noticed that PR44929 https://bugs.llvm.org/show_bug.cgi?id=44929 had been traced to this change. I've put a suggestion on how we might be able to fix it in the PR, if you have any thoughts please comment there.

Feb 17 2020, 1:33 PM · Restricted Project

Feb 13 2020

peter.smith accepted D74537: [LLD][ELF][AArch64] Change the semantics of -z pac-plt..

One query aside, this looks good to me. The original idea for glibc was to automatically have the dynamic loader sign the .plt.got entries on the presence of AARCH64_PAC_PLT, it turns out that those patches haven't been accepted in glibc yet. Android does not use lazy binding and marks the .plt.got as RELRO so PAC isn't much use there. I've marked as approved, but please wait a day or so to see if there are any comments from other timezones.

Feb 13 2020, 6:17 AM · Restricted Project
peter.smith added reviewers for D74537: [LLD][ELF][AArch64] Change the semantics of -z pac-plt.: MaskRay, grimar.

Adding other regular reviewers.

Feb 13 2020, 6:17 AM · Restricted Project
peter.smith committed rG29c13615576b: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols. (authored by peter.smith).
[LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols.
Feb 13 2020, 1:46 AM
peter.smith closed D73542: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols..
Feb 13 2020, 1:46 AM · Restricted Project

Feb 11 2020

peter.smith updated the diff for D73542: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols..

Updated to split the logic for the warning from the output. This permits some simplification. Updated tests.

Feb 11 2020, 3:53 PM · Restricted Project

Feb 9 2020

peter.smith updated the diff for D73542: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols..

I've updated the diff to add a warning about no interworking for the cases when the behaviour changes from lld 10.0. I've set the details in a large comment in front of warnOnStateMismatch.

Feb 9 2020, 1:17 PM · Restricted Project
peter.smith reopened D73542: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols..

Reopening as the addition of the warning is a non-trivial change.

Feb 9 2020, 1:17 PM · Restricted Project

Feb 6 2020

peter.smith added a comment to D73542: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols..

My apologies for the breakage.

No worries! Thanks for being very responsive and fixing quickly :) (To both you and maskray.)

<del>Please cherry pick 5461fa2b1fcfcfcd8e28e3ac3383d2245d5d90bf to release/10.x :)</del>

edit: We don't need it. D73542 is not in release/10.x

Feb 6 2020, 1:58 AM · Restricted Project

Feb 5 2020

peter.smith added a comment to D73542: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols..

Hello, this causes a bunch of chromium binaries to crash during startup: https://bugs.chromium.org/p/chromium/issues/detail?id=1047531

Looks like most of them contain assembly code. Do you have any idea from this fuzzy description what might be up? What's data that I can collect that's useful for debugging?

Feb 5 2020, 11:32 AM · Restricted Project
peter.smith added a comment to D74006: [MC][ELF] Make linked-to symbol name part of ELFSectionKey.

This is a deliberate choice so that we don't need to know the section where foo and bar are defined beforehand.

Just want to make sure this is well defined with respect to the description in ELF.

SHF_LINK_ORDER
     This flag adds special ordering requirements for link editors. The requirements apply if the sh_link field of this section's header references another section (the linked-to section). If this section is combined with other sections in the output file, it must appear in the same relative order with respect to those sections, as the linked-to section appears with respect to sections the linked-to section is combined with.
Feb 5 2020, 9:18 AM · Restricted Project

Feb 3 2020

peter.smith added a comment to D73904: [clang] stop baremetal driver to append .a to lib.

I think this is the right thing to do. According to https://sourceware.org/binutils/docs/ld/Options.html#Options

Add the archive or object file specified by namespec to the list of files to link. This option may be used any number of times. If namespec is of the form :filename, ld will search the library path for a file called filename, otherwise it will search the library path for a file called libnamespec.a.

An argument could be made that LLD could be a bit cleverer and only add the .a when it doesn't exist, although that would fail in the contrived case that someone had explicitly named their library lib name.a.a however it would be good to fix this in clang so that older versions of LLD will work.

Feb 3 2020, 10:02 AM · Restricted Project
peter.smith added a comment to D73865: [CodeGenModule] Assume dso_local for -fpic -fno-semantic-interposition.

If I've understood correctly this would make LLVM more aggressive for PIC relocation models, but perhaps more honest in an inter procedural optimisation context? The code changes look fine to me, I'm wondering if we've discussed this widely enough with the community to work out how to proceed here. For example do we have plan to test -fno-semantic-interposition well enough for it to be relied on. The consensus may already exist, and I don't know enough about it though.

Feb 3 2020, 4:14 AM · Restricted Project

Jan 29 2020

peter.smith added a comment to D73228: [AsmPrinter][ELF] Define local aliases (.Lfoo$local) for GlobalObjects.

Thanks for the update, I agree that informing the assembler of pic status isn't the right way to go, particularly if the assembler can't check that the code that you have written is actually pic and also the potentially incompatibility with GNU as. I've no objections.

Jan 29 2020, 4:45 AM · Restricted Project
peter.smith committed rG0b4a047bfbd1: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols. (authored by peter.smith).
[LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols.
Jan 29 2020, 3:43 AM
peter.smith closed D73542: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols..
Jan 29 2020, 3:43 AM · Restricted Project
peter.smith added a comment to D73542: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols..

Thanks for the suggestions, I've applied them prior to commit.

Jan 29 2020, 2:43 AM · Restricted Project

Jan 28 2020

peter.smith added a comment to D73228: [AsmPrinter][ELF] Define local aliases (.Lfoo$local) for GlobalObjects.

To make sure I understand, this is adding a local alias as at assembly time that can be used as a fixup target. This is necessary as at assembly time all we have is a global default visibility symbol which we have to assume can be interposed, even if the code-generator has already assumed it. Are there other uses that I'm missing?
As it stands this sounds reasonable to me, I'm trying to think if there are any other alternatives and what the trade-offs would be. For example is there no way that the assembler could find out the information at assembly time, or is that more complex than adding local aliases?

Jan 28 2020, 10:30 AM · Restricted Project
peter.smith created D73542: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols..
Jan 28 2020, 4:05 AM · Restricted Project
peter.smith committed rG4f38ab250ff4: [LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols (authored by peter.smith).
[LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols
Jan 28 2020, 3:56 AM
peter.smith closed D73474: [LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols.
Jan 28 2020, 3:55 AM · Restricted Project
peter.smith committed rG3238b03c1977: [LLD][ELF][ARM] clang-format function signature [NFC] (authored by peter.smith).
[LLD][ELF][ARM] clang-format function signature [NFC]
Jan 28 2020, 3:55 AM
peter.smith added a comment to D73518: [ELF] Mention symbol name in reportRangeError().

I think this is step in the right direction. I tend to find that relocation out of range errors tend to be more useful to linker developers (via bug reports) as there is often little that a user can do about it, and it often represents either a fault in the linker or a misuse of a relocation in the compiler, I think we could tend to include information that would help us diagnose problems.

Jan 28 2020, 3:55 AM · Restricted Project
peter.smith added inline comments to D73474: [LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols.
Jan 28 2020, 3:28 AM · Restricted Project
peter.smith added inline comments to D73474: [LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols.
Jan 28 2020, 2:05 AM · Restricted Project
peter.smith added inline comments to D73474: [LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols.
Jan 28 2020, 1:37 AM · Restricted Project

Jan 27 2020

peter.smith added a comment to D73474: [LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols.

Thanks for the comments, I'll fix up those last ones tomorrow.

Jan 27 2020, 10:48 AM · Restricted Project
peter.smith updated the diff for D73474: [LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols.

Removed redundant ==1, added test for branch to ifunc symbol.

Jan 27 2020, 10:39 AM · Restricted Project
peter.smith added inline comments to D73474: [LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols.
Jan 27 2020, 10:30 AM · Restricted Project
peter.smith updated the diff for D73474: [LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols.

Uploaded revision with explicit STT_NOTYPE symbol. Will wait till tomorrow to commit so I can handle any failures (just about to leave the office).

Jan 27 2020, 10:09 AM · Restricted Project
peter.smith created D73474: [LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols.
Jan 27 2020, 7:07 AM · Restricted Project

Jan 24 2020

peter.smith added a comment to D72899: [MC] Set sh_link to 0 if the associated symbol is undefined.

I am at a loss how to proceed now.

Adding people listed in D29104 (add !associated) and D29110 (allowed metadata dropping transforms).

Jan 24 2020, 11:01 AM · Restricted Project
peter.smith accepted D73254: [ELF] Rename relocateOne() to relocate() and pass `Relocation` to it.

Thanks for the update, I'm happy with the changes, will be worth waiting for George's ack as well.

Jan 24 2020, 10:41 AM · Restricted Project
peter.smith added inline comments to D73254: [ELF] Rename relocateOne() to relocate() and pass `Relocation` to it.
Jan 24 2020, 2:42 AM · Restricted Project

Jan 23 2020

peter.smith added a comment to D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0].

If the patchable functions is intended for clang-10 we'll need to make sure any fix is merged to clang-10.

This commit was made before release/10.x branch. Maybe the easiest thing is to revert the driver change in release/10.x (CC @hans), before we had a better understanding of the problem.
(Eventually I think the Linux kernel should have a better configure time test than a simple whether the compiler accepts -fpatchable-function-entry=2,0?)

@peter.smith @nickdesaulniers What do you think?

Revert on the 10.0 release sounds reasonable to me. That would prevent the kernel from enabling the option and would prevent the build failure.

But if trunk is broken, shouldn't it be reverted there first, and then we can merge the revert to 10.x (and then trunk can be fixed eventually)?

I don't think the commit is to be blamed. The availability of the driver option -fpatchable-function-entry= enables CONFIG_DYNAMIC_FTRACE_WITH_REGS and CONFIG_LIVEPATCH, which were not tested before. There could be other issues in the code path.

% rg fpatchable-function-entry
arch/arm64/Kconfig
147:            if $(cc-option,-fpatchable-function-entry=2)

arch/arm64/Makefile
100:  CC_FLAGS_FTRACE := -fpatchable-function-entry=2

If we can explicitly disable the options in the CI, that will be very nice.

Jan 23 2020, 10:46 AM · Restricted Project
peter.smith added a comment to D73254: [ELF] Rename relocateOne() to relocate() and pass `Relocation` to it.

Thanks for the update, will be worth seeing what George thinks tomorrow.

Jan 23 2020, 10:27 AM · Restricted Project
peter.smith added a comment to D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0].

@peter.smith @nickdesaulniers What do you think?

Revert on the 10.0 release sounds reasonable to me. That would prevent the kernel from enabling the option and would prevent the build failure.

I should have been clearer, apologies; we're not blocked by the assertion failures, the CI won't halt and should pick up other build failures.

Jan 23 2020, 10:27 AM · Restricted Project
peter.smith added a comment to D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0].

If the patchable functions is intended for clang-10 we'll need to make sure any fix is merged to clang-10.

This commit was made before release/10.x branch. Maybe the easiest thing is to revert the driver change in release/10.x (CC @hans), before we had a better understanding of the problem.
(Eventually I think the Linux kernel should have a better configure time test than a simple whether the compiler accepts -fpatchable-function-entry=2,0?)

@peter.smith @nickdesaulniers What do you think?

Revert on the 10.0 release sounds reasonable to me. That would prevent the kernel from enabling the option and would prevent the build failure.

Jan 23 2020, 10:18 AM · Restricted Project
peter.smith added a comment to D73254: [ELF] Rename relocateOne() to relocate() and pass `Relocation` to it.

Looking at where we use relocateOne directly, I can see the PLT code, ARMErrataFix and AArch64ErrataFix , and Thunks that call relocateOne directly. For my selfish purposes this isn't a problem as for correcting ARM/Thumb interworking via BLX I'd only need to know the symbol type for R_ARM_CALL and R_ARM_THM_CALL which always have a symbol. I think that the R_ARM_THM_JUMP19, R_ARM_THM_JUMP24 and R_ARM_JUMP24 can be solved in needsThunk() which already has the information it needs.

Jan 23 2020, 9:52 AM · Restricted Project
peter.smith added inline comments to D73254: [ELF] Rename relocateOne() to relocate() and pass `Relocation` to it.
Jan 23 2020, 9:15 AM · Restricted Project
peter.smith added a comment to D73070: Add function attribute "patchable-function-prefix" to support -fpatchable-function-entry=N,M where M>0.

-fpatchable-function-entry=2,0 is included in the release/10.x branch. I don't want to push the N,M where M>0 changes to release/10.x.

Jan 23 2020, 8:56 AM · Restricted Project
peter.smith added a comment to D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0].

Although this particular commit will not be at fault, it is the option that enables the feature which is the earliest I can bisect the fault to. There are 3 files in linux that assert fail on the Implement the 'patchable-function attribute'. The files in question are kasan/quarantine.c, mm/slab_common.c and mm/slub.c .

Jan 23 2020, 8:47 AM · Restricted Project
peter.smith added a comment to D72959: Relative VTables ABI on Fuchsia.

There's two independently-useful things here for the relocation, right? First, it's useful to be able to express a relocation to a symbol that has the semantics of a particular function but doesn't necessarily have its address, and that concept could be used across many "physical" relocations; and second, it's potentially useful to have a shifted-by-two relative address relocation, at least on AArch64 where instructions (and v-table entries under this ABI) are always four-byte-aligned anyway. Is it possible to separate these concerns in ELF, e.g. by having a "symbol" that can be the target of any other relocation but which actually represents a function of unspecified address with the semantics of another function?

Jan 23 2020, 3:29 AM · Restricted Project, Restricted Project, Restricted Project

Jan 22 2020

peter.smith committed rGe727f39ec0b1: [LLD][ELF][ARM] Don't apply --fix-cortex-a8 to relocatable links. (authored by peter.smith).
[LLD][ELF][ARM] Don't apply --fix-cortex-a8 to relocatable links.
Jan 22 2020, 3:05 AM
peter.smith closed D73100: [LLD][ELF][ARM] Don't apply --fix-cortex-a8 to relocatable links..
Jan 22 2020, 3:05 AM · Restricted Project
peter.smith added a comment to D72959: Relative VTables ABI on Fuchsia.
In D72959#1832011, @pcc wrote:

On Aarch64, right now only CALL26 and JUMP26 instructions generate PLT relocations, so we manifest them with stubs that are just jumps to the original function.

I think it would be worth considering defining a new relocation type for this. I think we should make the new reloc type represent the relative address shifted right by 2, which would allow it to be used freely in the aarch64 small code model (which permits programs up to 4GB), but would require the target to be 4-byte aligned, which seems like a reasonable restriction. On aarch64, decoding this would not require any additional instructions either (we can fold the lsl into the add). We could use the same technique for a new GOTPCREL relocation to be used for the RTTI component. @peter.smith what do you think?

Jan 22 2020, 3:04 AM · Restricted Project, Restricted Project, Restricted Project

Jan 21 2020

peter.smith added a comment to D73070: Add function attribute "patchable-function-prefix" to support -fpatchable-function-entry=N,M where M>0.

-fpatchable-function-entry=N,M (where M>0) + -mbranch-protection=bti is quite clear (https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01185.html).

For -fpatchable-function-entry=N,0 + -mbranch-protection=bti/-fcf-protection=branch, if you have preference for placement (a) or (b), please make a comment:) and/or reply at https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01185.html
If you are not subscribed to the gcc-patches mailing list (like me), you will have to download the raw email file Other format: [Raw text] and reply...

(b) will require separate patches.

Jan 21 2020, 9:48 AM · Restricted Project
peter.smith updated the diff for D73100: [LLD][ELF][ARM] Don't apply --fix-cortex-a8 to relocatable links..

Added a check for lld -v to make sure a detection message isn't printed.

Jan 21 2020, 7:16 AM · Restricted Project
peter.smith added a comment to D73100: [LLD][ELF][ARM] Don't apply --fix-cortex-a8 to relocatable links..

It looks fine to me, I'll leave it for others, have a minor question though.

Jan 21 2020, 7:16 AM · Restricted Project
peter.smith created D73100: [LLD][ELF][ARM] Don't apply --fix-cortex-a8 to relocatable links..
Jan 21 2020, 6:13 AM · Restricted Project
peter.smith added a comment to D73047: [ELF] Refactor uses of getInputSections to improve efficiency NFC.

LGTM too. Thanks for the update.

Jan 21 2020, 4:11 AM · Restricted Project
peter.smith committed rGdbd0ad33668e: [LLD][ELF] Add support for INPUT_SECTION_FLAGS (authored by peter.smith).
[LLD][ELF] Add support for INPUT_SECTION_FLAGS
Jan 21 2020, 2:20 AM
peter.smith closed D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.
Jan 21 2020, 2:20 AM · Restricted Project

Jan 20 2020

peter.smith added a comment to D65656: AArch64: support @llvm.{return,frame}address in GlobalISel.

Re-closing.

Jan 20 2020, 3:50 PM · Restricted Project
peter.smith added a comment to D73047: [ELF] Refactor uses of getInputSections to improve efficiency NFC.

Not got any objections to the refactoring, I'm not sure we need to share the loop traversal between getInputSections and getFirstInputSections (see comment inline) but I'm willing to go with the consensus on that.

Jan 20 2020, 12:55 PM · Restricted Project
peter.smith accepted D72968: [lld][ELF] Don't apply --fix-cortex-a53-843419 to relocatable links..

Reading the BFD source it looks like stubs (which is what errata patches like this are generated) are not added for relocatable links, I couldn't find any ld tests for them and an empirical test with:

Jan 20 2020, 12:09 PM · Restricted Project
peter.smith added a comment to D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

Thanks for the comments. It has been a bit of a nightmare of a day and I've only just got to this. I'll apply the suggestions and commit tomorrow morning so I can catch any problems.

Jan 20 2020, 11:59 AM · Restricted Project
peter.smith added a comment to D72897: List implicit operator== after implicit destructors in a vtable..

It is also failing on the other 32-bit arm bots, example http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/13052/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Avirtual-compare.cpp
Looking at the failure

Jan 20 2020, 10:35 AM · Restricted Project

Jan 17 2020

peter.smith updated the diff for D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.
  • Removed support for corner case where there is just a filespec. This permits the INPUT_SECTION_FLAGS logic to be moved to readInputSectionDescription from readOutputSectionDescription.
  • Changed comment in test
Jan 17 2020, 10:22 AM · Restricted Project
peter.smith added a comment to D72899: [MC] Set sh_link to 0 if the associated symbol is undefined.

The implementation looks good to me. Will defer to others that know -fsanitize-coverage=inline-8bit-counters and LTO better. There was a comment in the PR about adding a dummy non-gcable section instead, I think that this would have the same effect as having the linker handle sh-link == 0, but it might be more portable to other linkers. Any thoughts on whether a dummy section would work better?

Jan 17 2020, 7:27 AM · Restricted Project
peter.smith added a comment to D72904: [ELF] Allow SHF_LINK_ORDER sections to have sh_link=0.

I'm reluctantly in favour of making the change on the basis that there exists a design using SHF_LINKORDER that lets this happen. My reservation is that in some use cases of SHF_LINKORDER such as .ARM.exidx sections where there is a 1 : 1 relationship between meta-data and "content" (for want of a better word) section then sh_link = 0 would be a sign that something had gone badly wrong. Having said that there isn't any thing in ELF that forbids the use case that this needs. I think that for .ARM.exidx which is the other main user of SHF_LINKORDER we handle it as a special case so this shouldn't make a difference in practice.

Jan 17 2020, 6:59 AM · Restricted Project
peter.smith added a comment to D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

I'll remove the support for the corner-case of when there is just a filepattern. Most likely won't get the time today, if I don't then I'll update on Monday. Thanks for the comments.

Jan 17 2020, 6:40 AM · Restricted Project
peter.smith added a comment to D72892: [MC][ARM] Resolve some pcrel fixups at assembly time.

Personally I'd prefer we either go for a permanent decision to always resolve some fixups without relocations, or just implement the relocations in LLD when they are missing, assuming they are also in ld.bfd. It is likely that ld.gold won't have implemented the relocations either but we may be less concerned about that.

Jan 17 2020, 6:30 AM · Restricted Project
peter.smith updated the diff for D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

Updated diff. Main changes:

  • Use stringswitch with enumerations
  • Incorporate simplification suggestions
Jan 17 2020, 4:28 AM · Restricted Project
peter.smith added a comment to D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

Thanks for the review. I'll upload another patch that I hope to have addressed all the comments.

Jan 17 2020, 4:25 AM · Restricted Project
peter.smith committed rG01ad4c838466: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS. (authored by peter.smith).
[LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS.
Jan 17 2020, 2:52 AM
peter.smith closed D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..
Jan 17 2020, 2:52 AM · Restricted Project
peter.smith added a comment to D72892: [MC][ARM] Resolve some pcrel fixups at assembly time.

As I understand it these relocation types are not often implemented is due to their short range. The Arm and Thumb2 ldr (literal) range (search for A8.8.64 in https://static.docs.arm.com/ddi0406/c/DDI0406C_C_arm_architecture_reference_manual.pdf) is:

Encoding T2 or A1 Any value in the range -4095 to 4095.

Outside of carefully crafted section orders, the chances of the relocation being in range if it went outside of a section boundary is very low. If the relocation is within a section boundary then the assembler can resolve it.

Jan 17 2020, 1:37 AM · Restricted Project

Jan 16 2020

peter.smith updated the diff for D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

Address code review comments so far. Main changes:

  • Rebased
  • Used EnumEntry as in ELFDumper.cpp
  • Updated parser with better diagnostic message
  • Updated tests
Jan 16 2020, 9:25 AM · Restricted Project
peter.smith added a comment to D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

Thanks for the comments, I'll upload a new patch shortly.

Jan 16 2020, 9:25 AM · Restricted Project
peter.smith updated the diff for D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..

Updated diff (and description) to simplify part of the check to os->size > target->getThunkSectionSpacing(). This is conservative, but simple. We can in theory do better, but it is difficult to do so without using heuristics. In effect it is a judgement that ThunkSections close to, but not necessarily at the end of the OutputSection have less of a chance of generating sufficient extra thunks and patches to cause non-convergence.

Jan 16 2020, 6:53 AM · Restricted Project
peter.smith added a comment to D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..

Thanks for the comments. I'll post an update with a simplification as I don't think I can justify the heuristic without being too hand wavy.

Jan 16 2020, 3:31 AM · Restricted Project
peter.smith added a comment to D72819: [ELF] Decrease alignment of ThunkSection on 64-bit targets from 8 to 4.

I'm happy to go ahead with this. The maximum alignment for instructions on Arm and AArch64 is 4, which is what the compiler uses for code sections.

Jan 16 2020, 1:40 AM · Restricted Project

Jan 15 2020

peter.smith added a comment to D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

Thanks for the comments, I'll make an update tomorrow. I'll plan to:

  • Try the flagsMask + withMask to see if it looks any better.
  • Limit the recognised flags to what BFD accepts, I'd like to continue to recognise integers.
  • Look at the error reporting in the parser.
  • Update the tests.
Jan 15 2020, 11:18 AM · Restricted Project
peter.smith created D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.
Jan 15 2020, 2:46 AM · Restricted Project

Jan 14 2020

peter.smith updated the diff for D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..

Updated the fix, and description after testing on a real linux kernel allyesconfig build. The description contains the full details, which should also be present in the comment. In summary we round up the size of a thunk section to nearest 4KiB when:

  • The erratum fix is being used.
  • The thunk section may be placed before the end of Output Section so there may be code after it.
  • The InputSectionDescription is larger than 4KiB.
Jan 14 2020, 9:55 AM · Restricted Project
peter.smith added a comment to D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..

Thanks for the comments. I've decided to rework this In light of it not actually fixing the problem in Linux. In the allyesconfig build the overall .text OutputSection is larger than thunkSectionSpacing, but the assert was for in effect an InputSectionDescription within .text so I've had to think again. I'm nearly ready to submit again, just got to test in on the kernel itself first.

Jan 14 2020, 4:09 AM · Restricted Project

Jan 13 2020

peter.smith added a comment to D71688: [AArch64] Add -mtls-size option for ELF targets.

Committed as 10c11e4e2d05cf0e8f8251f50d84ce77eb1e9b8d , I've used the new attribution process https://llvm.org/docs/DeveloperPolicy.html so you should show up as the author of the patch in the commit log. I'll comment here if there are any problems with buildbots.

Jan 13 2020, 2:25 AM · Restricted Project, Restricted Project
peter.smith committed rG10c11e4e2d05: This option allows selecting the TLS size in the local exec TLS model, which is… (authored by kawashima-fj).
This option allows selecting the TLS size in the local exec TLS model, which is…
Jan 13 2020, 2:22 AM
peter.smith closed D71688: [AArch64] Add -mtls-size option for ELF targets.
Jan 13 2020, 2:22 AM · Restricted Project, Restricted Project

Jan 10 2020

peter.smith accepted D72474: [ELF] Make TargetInfo::writeIgotPlt a no-op.

LGTM. ld.bfd, at least on AARCH64 will write the address of PLT[0] into the .got.plt, but this may just be out of convenience as it will be overwritten by the IRELATIVE relocation before it can be used.

Jan 10 2020, 4:33 AM · Restricted Project
peter.smith accepted D72215: [AArch64] Add function attribute "patchable-function-entry" to add NOPs at function entry.

LGTM thanks for the update.

Jan 10 2020, 3:58 AM · Restricted Project
peter.smith added a comment to D71688: [AArch64] Add -mtls-size option for ELF targets.

I can do it early next week, unfortunately a little too busy today. If anyone else can do it earlier please go ahead. To obtain commit access it is worth following https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Jan 10 2020, 2:35 AM · Restricted Project, Restricted Project
peter.smith accepted D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

I'm personally strongly enough in favour to approve given no strong objections so far. Will be worth holding off to next week to see if the approval provokes any.

Jan 10 2020, 2:11 AM · Restricted Project

Jan 8 2020

peter.smith added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

If I could summarise the comments so far:

  • There is some concern that this is not the full solution and there maybe other things that can break interposing.
  • The response is that fixing the assembler will be a necessary part of that work.
  • There maybe some extra via-PLT calls in shared libraries if -ffunction-sections is not used, but these can be mitigated by giving the symbols non-default visibility, and in any case these would exist with -ffunction-sections.
Jan 8 2020, 8:56 AM · Restricted Project
peter.smith added a comment to D72215: [AArch64] Add function attribute "patchable-function-entry" to add NOPs at function entry.

I think GCC's __patchable_function_entries has severe design flaws. https://gcc.gnu.org/ml/gcc/2020-01/msg00067.html

I have tried a different design in this patch, temporarily naming it __patchable_function_entry.

Jan 8 2020, 2:02 AM · Restricted Project
peter.smith accepted D71688: [AArch64] Add -mtls-size option for ELF targets.

Thanks for the update, looks good to me.

Jan 8 2020, 1:20 AM · Restricted Project, Restricted Project

Jan 7 2020

peter.smith accepted D71794: [ELF] Don't special case weak symbols for pie with no shared objects.

Ok LGTM. May be worth waiting a day to see if there are any final objections.

Jan 7 2020, 10:35 AM · Restricted Project
peter.smith added a comment to D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..

On looking again at the linux kernel makefile

Jan 7 2020, 10:07 AM · Restricted Project
peter.smith created D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..
Jan 7 2020, 9:27 AM · Restricted Project
peter.smith added a comment to D72215: [AArch64] Add function attribute "patchable-function-entry" to add NOPs at function entry.

Address comments.

The section __patchable_function_entries needs more thoughts so it is not
implemented. I'll probably use SHF_LINK_ORDER when -ffunction-sections, and add
SHF_WRITE (to prevent text relocations; apparently GCC does not consider these
issues).

Jan 7 2020, 6:09 AM · Restricted Project
peter.smith added a comment to D71688: [AArch64] Add -mtls-size option for ELF targets.

Apologies for the delay in responding, just come back from vacation. I've checked the implementation against GCC and it looks like it will give the same behaviour. I've got one minor suggestion surrounding the clamping of TLSSize to its maximum value. It looks like it would only need to be done once, is there a convenient place, such as AArch64TargetMachine where it can be done once?

No need to apologize. I know many people are in vacation last two or three weeks.

I'm not sure whether I understand your suggestion correctly. Do you mean the following code in the LowerELFTLSLocalExec function should be in a function in AArch64TargetMachine or somewhere which is called only once because LowerELFTLSLocalExec is called for every thread local variable? If so, it makes sense. I'll find a convenient place.

if (TLSSize == 0) // default
  TLSSize = 24;
if ((CModel == CodeModel::Small || CModel == CodeModel::Kernel) &&
    TLSSize > 32)
  // for the small (and kernel) code model, the maximum TLS size is 4GiB
  TLSSize = 32;
else if (CModel == CodeModel::Tiny && TLSSize > 24)
  // for the tiny code model, the maximum TLS size is 1MiB (< 16MiB)
  TLSSize = 24;
Jan 7 2020, 6:09 AM · Restricted Project, Restricted Project
peter.smith added a comment to D71688: [AArch64] Add -mtls-size option for ELF targets.

Apologies for the delay in responding, just come back from vacation. I've checked the implementation against GCC and it looks like it will give the same behaviour. I've got one minor suggestion surrounding the clamping of TLSSize to its maximum value. It looks like it would only need to be done once, is there a convenient place, such as AArch64TargetMachine where it can be done once?

Jan 7 2020, 4:19 AM · Restricted Project, Restricted Project
peter.smith added a comment to D71794: [ELF] Don't special case weak symbols for pie with no shared objects.

Apologies for the delay in responding, the original came in when I was on vacation.

  • Am I correct in thinking that the extra dynamic symbols that will result with -pie and no shared libraries are just wasted space, but won't cause anything to happen at run time as there are no dynamic relocations generated against them?
  • The main argument for removing is that this our ld.bfd is ad-hoc and this part doesn't help much, removing it would simplify here and permit a simplification in D71795. Is there a plan beyond D71795 to more closely follow ld.bfd behaviour, or is this just an area where we expect cosmetic inconsistencies?
Jan 7 2020, 2:46 AM · Restricted Project
peter.smith committed rG051c4d5b7bcf: [LLD][ELF][AArch64] Do not use thunk for undefined weak symbol. (authored by peter.smith).
[LLD][ELF][AArch64] Do not use thunk for undefined weak symbol.
Jan 7 2020, 2:00 AM
peter.smith closed D72267: [LLD][ELF][AArch64] Do not use thunk for undefined weak symbol..
Jan 7 2020, 1:59 AM · Restricted Project

Jan 6 2020

peter.smith created D72267: [LLD][ELF][AArch64] Do not use thunk for undefined weak symbol..
Jan 6 2020, 6:51 AM · Restricted Project