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

Recent Activity

Today

peter.smith accepted D99600: [arm][compiler-rt] add armv8m.main and arv8.1m.main targets.

LGTM, sorry for the delay.

Wed, Apr 14, 8:04 AM · Restricted Project
peter.smith added a comment to D99600: [arm][compiler-rt] add armv8m.main and arv8.1m.main targets.

It's now been a bit more than a week since the last reply. I suggest we merge this. If we want to add the architectures to the Apple-specific targets, that can always be done later.

Wed, Apr 14, 6:32 AM · Restricted Project
peter.smith accepted D100372: [Clang][ARM] Define __VFP_FP__ macro unconditionally.

I think this is the right thing to do. GCC changed to unconditionally set the macro with https://gcc.gnu.org/legacy-ml/gcc-patches/2016-10/msg01025.html my understanding is that the macro means that floating point representation uses the VFP format and not the FPA format (predecessor to VFP, not supported by clang, found in very old systems), it does not mean that there is a VFP unit present.

Wed, Apr 14, 5:27 AM · Restricted Project

Fri, Apr 9

peter.smith accepted D98916: [ARM] support symbolic expression as immediate in memory instructions.

Thanks for the update LGTM. It sounds like you have all the cases covered. Apologies for the noise.

Fri, Apr 9, 1:26 AM · Restricted Project

Wed, Apr 7

peter.smith accepted D99417: [AArch64][v8.5A] Add BTI to all function starts.

This LGTM, although please give a few days for MaskRay to check the patchable functions case.

Wed, Apr 7, 3:02 AM · Restricted Project
peter.smith added a comment to D98916: [ARM] support symbolic expression as immediate in memory instructions.

I have a one major concern, and a number of minor nits that could easily be addressed prior to commit if necessary.

Wed, Apr 7, 1:35 AM · Restricted Project

Wed, Mar 31

peter.smith accepted D98333: [builtins] Avoid enum name conflicts with fenv.h.

LGTM. Seems sensible to avoid a name clash with a standard header.

Wed, Mar 31, 6:27 AM · Restricted Project
peter.smith added a comment to D99600: [arm][compiler-rt] add armv8m.main and arv8.1m.main targets.

From an ELF perspective this looks good to me, armv7e-m appears in the same places for these targets. Will be worth adding someone from Apple to see if it is worth adding to CompilerRTDarwinUtils.cmake.

Wed, Mar 31, 2:43 AM · Restricted Project

Tue, Mar 30

peter.smith added a comment to D99417: [AArch64][v8.5A] Add BTI to all function starts.

My guess is that this would only affect a small number of functions?

I got some numbers on the NDK samples (https://github.com/android/ndk-samples). Of a total of 6746 functions, this patch inserts a landing pad for 22 additional functions, which is ~0.3% of them. I haven't done proper benchmarking since this is a bugfix rather than an optimization, but I agree that this patch should affect a small number of functions.

Tue, Mar 30, 6:16 AM · Restricted Project

Fri, Mar 26

peter.smith added a comment to D99417: [AArch64][v8.5A] Add BTI to all function starts.

This looks reasonable to me, while the linker moving a local function far away from its callers in the same object file is unlikely, it is at least theoretically possible with linker scripts or a symbol ordering file. My guess is that this would only affect a small number of functions?

Fri, Mar 26, 8:51 AM · Restricted Project

Wed, Mar 24

peter.smith accepted D99232: [Nomination] Adding new Google representative to security group.

LGTM too.

Wed, Mar 24, 11:58 AM · Restricted Project
peter.smith added a comment to D98332: [builtins] Raise FE_INVALID/FE_DIVBYZERO for 128-bit float math functions.

Thanks for the update. Test changes look good to me. Just wanted to check to see if you had any changes planned for lifting some of the common tests into a function? Or if that can be left to another patch?

Wed, Mar 24, 8:13 AM · Restricted Project

Fri, Mar 19

peter.smith added a comment to D98916: [ARM] support symbolic expression as immediate in memory instructions.

The fixup code looks reasonable to me as it is mostly reusing the same logic as the pc-relative fixup. The names are internal to LLVM so there isn't any particular standard. I think this fixup would correspond to the relocation R_ARM_ABS12 https://github.com/ARM-software/abi-aa/blob/master/aaelf32/aaelf32.rst#relocation-codes so it may be worth using fixup_arm_ldst_abs_12 but that is only a suggestion. I wouldn't expect the assembler to use the relocation as the range is so short that any attempt to use it would likely result in an out of range error.

Fri, Mar 19, 6:40 AM · Restricted Project

Wed, Mar 17

peter.smith added a comment to D98445: [ELF] Special case --shuffle-sections=-1 to reverse input sections.

I'm fine with the change.

Wed, Mar 17, 5:02 AM · Restricted Project

Mar 12 2021

peter.smith added a comment to D98332: [builtins] Raise FE_INVALID/FE_DIVBYZERO for 128-bit float math functions.

One small concern related to the tests on a target that doesn't implement __fe_raise_invalid and friends.

Mar 12 2021, 3:00 AM · Restricted Project
peter.smith added a comment to D98445: [ELF] Special case --shuffle-sections=-1 to reverse input sections.

No fundamental objections, although I'm wondering if static constructor order is the main problem it would be better to do something specific. For example reversing the order of the constructors of the same priority while holding the remainder of the program constant. A smaller local change would help rule out other layout changes due to section order change.

Mar 12 2021, 1:56 AM · Restricted Project

Mar 11 2021

peter.smith accepted D98306: [ELF] Support . and $ in symbol names in expressions.

Fair enough, I think '$' and '.' are the important ones. LGTM.

Mar 11 2021, 12:12 AM · Restricted Project

Mar 10 2021

peter.smith accepted D98353: [builtins] Fix value of ARM_INEXACT.

LGTM, although give Yi some time to respond in case there is something I've missed about the original context.

Mar 10 2021, 9:49 AM · Restricted Project
peter.smith added a comment to D98332: [builtins] Raise FE_INVALID/FE_DIVBYZERO for 128-bit float math functions.

From another non-floating point expert, this looks reasonable to me. I'll see if I can find anyone internally that knows more and would like to take a look.

Mar 10 2021, 7:53 AM · Restricted Project
peter.smith added a comment to D98306: [ELF] Support . and $ in symbol names in expressions.

Noticed that ld.bfd will also accept ~ which may be worth adding as well; other than that looks reasonable to me.

Mar 10 2021, 2:27 AM · Restricted Project

Mar 5 2021

peter.smith added a comment to D97976: [MC] Change ELFOSABI_NONE to ELFOSABI_GNU for SHF_GNU_RETAIN.

Thanks for the explanation and the comment, I missed the part about the anonymous namespace hiding it. I'll check back next week to see if there are any other comments, if not I can set approve. Happy for someone else to do that too.

Mar 5 2021, 10:15 AM · Restricted Project
peter.smith added a comment to D97976: [MC] Change ELFOSABI_NONE to ELFOSABI_GNU for SHF_GNU_RETAIN.

No objections.

Mar 5 2021, 6:56 AM · Restricted Project

Mar 1 2021

peter.smith updated the diff for D97550: [LLD][ELF][ARM] Refactor inBranchRange to use addend for PC Bias.

Changes:

  • Clang format run over changed code
  • Improved comment
  • Added test case for specific Arm/Thumb reuse
Mar 1 2021, 3:24 AM · Restricted Project
peter.smith added a comment to D97550: [LLD][ELF][ARM] Refactor inBranchRange to use addend for PC Bias.

Thanks, I'll upload another patch.

Mar 1 2021, 3:23 AM · Restricted Project

Feb 26 2021

peter.smith requested review of D97550: [LLD][ELF][ARM] Refactor inBranchRange to use addend for PC Bias.
Feb 26 2021, 6:04 AM · Restricted Project

Feb 23 2021

peter.smith added a comment to D96914: [ELF] Add -z start-stop-gc to let __start_/__stop_ not retain C identifier name sections.

Thanks for the update. I'm happy for this to be an option that defaults to off. That won't break any existing code that is depending on the behavior.

Feb 23 2021, 7:51 AM · Restricted Project

Feb 22 2021

peter.smith added a comment to D96914: [ELF] Add -z start-stop-gc to let __start_/__stop_ not retain C identifier name sections.

These types of reference can be difficult as it is difficult to know what the users intention with respect to gc_root is. I think that the majority of programs don't depend on all sections being marked live but there are definitely programs out there that do. It may not be easy to migrate all these programs to use alternative means such as SHF_RETAIN. Would it be useful to make this a selectable option for users with lots of existing code that depends on these sections being marked live?

Feb 22 2021, 3:00 AM · Restricted Project

Feb 8 2021

peter.smith accepted D95985: [ELF] Resolve defined symbols before undefined symbols.

I've put my approval down, as I think it is a worthwhile change to make regardless of whether the test case that had problems is strictly legal.

Feb 8 2021, 7:47 AM · Restricted Project
peter.smith added a comment to D96211: [llvm-objdump] Support PLT decoding for aarch64_be.

LGTM too, thanks for updating for BE.

Feb 8 2021, 4:59 AM · Restricted Project
peter.smith accepted D96188: [ELF] Support aarch64_be.

Changes LGTM.

Feb 8 2021, 4:58 AM · Restricted Project
peter.smith accepted D96214: [ELF] Inspect -EL & -EB for OUTPUT_FORMAT(default, big, little).

LGTM, I've made some small suggestions.

Feb 8 2021, 4:49 AM · Restricted Project

Feb 7 2021

peter.smith added a comment to D95199: [ELF] Improve compatibility with ld.bfd linker scripts.

Thanks for the updates. My day job has been very busy of late so I've not got a lot of spare time to look. As it stands I'm still a bit nervous about the test changes, it is also worth mentioing that I'd want at least one of grimar and MaskRay to agree on any changes in this area as they were both initially hesitant.

Feb 7 2021, 4:18 AM · Restricted Project

Feb 4 2021

peter.smith added a comment to D95985: [ELF] Resolve defined symbols before undefined symbols.

OK. Had a chance to go through this in more detail. To make sure I understand; if the undefined symbols are in object files/libraries yet to be processed then there is no difference, but if there is a lazy symbol for an undefined then the object will be fetched immediately on seeing

// Handle global undefined symbols.
   if (eSym.st_shndx == SHN_UNDEF) {

Resulting in globals being loaded from a recursive chain of lazy loads before the globals from this object file have been loaded.

Feb 4 2021, 11:52 AM · Restricted Project
peter.smith added a comment to D95586: [ARM] permit PC as destination of MOVS.

FWIW UNPREDICTABLE in the Architecture, is essentially anything can happen. There is a more specific CONSTRAINED UNPREDICTABLE, which for each case states a number of possible behaviours for the implementation.

The term UNPREDICTABLE describes a number of cases where the architecture has a feature that software must not
use. For execution in AArch32 state, where previous versions of the architecture define behavior as
UNPREDICTABLE, the Armv8-A architecture specifies a narrow range of permitted behaviors. This range is the range
of CONSTRAINED UNPREDICTABLE behavior. All implementations that are compliant with the architecture must
follow the CONSTRAINED UNPREDICTABLE behavior
Feb 4 2021, 2:59 AM · Restricted Project
peter.smith added a comment to D95985: [ELF] Resolve defined symbols before undefined symbols.

The real world case is reduced from a Fuchsia PGO usage: a.a(a.o) has a constructor within COMDAT group C5
while a.a(b.o) has a constructor within COMDAT group C2. Because they use
different group signatures, they are not de-duplicated.

Feb 4 2021, 1:30 AM · Restricted Project
peter.smith accepted D95994: [ELF] Allow R_386_GOTOFF from .debug_info.

Change LGTM. For a future change, would it be worth looking at this from another direction and saying what can we disallow in non SHF_ALLOC sections, for example PC-relative? There can be an allow-list for known relocations and perhaps a warning for unknown ones. This should make us a little more resilient to changes in objects.

Feb 4 2021, 1:07 AM · Restricted Project

Feb 3 2021

peter.smith added a comment to D95199: [ELF] Improve compatibility with ld.bfd linker scripts.

I've had a look at a couple of the tests (not looked at the others) there look to be some ordering problems with some special cases like partition sections and .tbss. To find these I checked the order of writing sections with the new and old. As mentioned in the comment I don't know much about the partition mechanism, as such I'd recommend making sure that there were no explainable differences in those tests. Will try and find some more time to look at the others later in the week/weekend.

Feb 3 2021, 10:22 AM · Restricted Project

Feb 1 2021

peter.smith added a comment to D95199: [ELF] Improve compatibility with ld.bfd linker scripts.

Thanks for the update, I'll try and take a look later in the week. Will have to have a think about the zero size section case. If we haven't assigned it to a PT_LOAD and it defines no symbol it may be possible to not put it in the object file.

Feb 1 2021, 11:09 AM · Restricted Project
peter.smith added a comment to D95730: [MC] Support SHF_GNU_RETAIN as section flag 'R'.

Is there any feature triggering ELFOSABI_ARM / ELFOSABI_ARM_AEABI? If there is and such an object file also uses STT_GNU_IFUNC, the single value EI_OSABI is indeed insufficient.

Feb 1 2021, 11:04 AM · Restricted Project

Jan 26 2021

peter.smith updated subscribers of D95429: Deprecate build cache servers for aarch64 and aarch32 from Buildbot.
Jan 26 2021, 2:19 AM
peter.smith added a comment to D95429: Deprecate build cache servers for aarch64 and aarch32 from Buildbot.

LGTM from the perspective of removing the workers. Worth mentioning that you are the maintainer of the machines.

Jan 26 2021, 2:16 AM
peter.smith added a comment to D94612: [LLD][ELF][AArch64] Add support for R_AARCH64_LD64_GOTPAGE_LO15 relocation.

Thanks for the updates. LGTM too.

Jan 26 2021, 1:07 AM · Restricted Project

Jan 25 2021

peter.smith added a comment to D95198: [ELF] Fix program header alloc when first PT_LOAD is not at lowest VMA.

I'm not clear on what the requirements are here. I would be very interested to see a reference to placing headers in the first loadable program segment. That sounds like it could be a convention of some linker/platform, but I can't remember seeing that in any specification.

Jan 25 2021, 2:50 AM · Restricted Project

Jan 4 2021

peter.smith added a comment to D93948: [MC] Merge section flags for user defined sections.

Another change that is worth highlighting in the description is: https://reviews.llvm.org/D73999 [MC][ELF] Error for sh_type, sh_flags or sh_entsize change with its associated LLVM-DEV message: https://lists.llvm.org/pipermail/llvm-dev/2020-February/139390.html

Jan 4 2021, 4:21 AM · Restricted Project

Dec 14 2020

peter.smith added a comment to D93045: [ELF] AArch64: Handle DT_AARCH64_VARIANT_PCS.

Updated patch based on previous comments. For in.relaIplt / R_AARCH64_IRELATIVE
I decided to not include since it seems to what binutils is done currently. I will raise with
them if this is the intended behaviour or if R_AARCH64_IRELATIVE should also create
the dynamic tag (and fix is accordingly on lld if it were the case).

Dec 14 2020, 1:49 AM · Restricted Project

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