peter.smith (Peter Smith)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 11 2016, 8:56 AM (106 w, 22 h)

Recent Activity

Yesterday

peter.smith added a comment to D45164: [MC] Change AsmParser to leverage Assembler during evaluation.

FWIW I'm in favour of this approach, and of merging the Asm and ObjectStreamer as I've recently found a case where this would have been useful (deriving a MCSubtargetInfo when emitting constant pools). There were others with stronger objections though.

Mon, Apr 23, 8:43 AM
peter.smith added a comment to D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

I've added reviews for the support of writeNops(). My apologies for the size of the changes, the first three make sure the STI is recorded in the fragment so that we can retrieve it when calling writeNops. The final one passes STI through to writeNops and removes STI from the X86 and ARM AsmBackends.
1 [MC] Move bundling and MCSubtargetInfo to MCEncodedFragment [NFC] D45959
2 [MC] Add MCSubtargetInfo to MCPaddingFragment [NFC] D45960
3 [MC] Add MCSubtargetInfo to MCAlignFragment [NFC] D45961
4 [MC] Use local MCSubtargetInfo in writeNops D45962

Mon, Apr 23, 7:35 AM
peter.smith added a dependent revision for D45960: [MC] Add MCSubtargetInfo to MCPaddingFragment [NFC]: D45962: [MC] Use local MCSubtargetInfo in writeNops.
Mon, Apr 23, 7:29 AM
peter.smith created D45962: [MC] Use local MCSubtargetInfo in writeNops.
Mon, Apr 23, 7:29 AM
peter.smith added a dependent revision for D45959: [MC] Move bundling and MCSubtargetInfo to MCEncodedFragment [NFC]: D45962: [MC] Use local MCSubtargetInfo in writeNops.
Mon, Apr 23, 7:29 AM
peter.smith added a dependent revision for D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup: D45962: [MC] Use local MCSubtargetInfo in writeNops.
Mon, Apr 23, 7:29 AM
peter.smith added a dependent revision for D45961: [MC] Add MCSubtargetInfo to MCAlignFragment [NFC]: D45962: [MC] Use local MCSubtargetInfo in writeNops.
Mon, Apr 23, 7:29 AM
peter.smith created D45961: [MC] Add MCSubtargetInfo to MCAlignFragment [NFC].
Mon, Apr 23, 7:24 AM
peter.smith added a dependent revision for D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup: D45961: [MC] Add MCSubtargetInfo to MCAlignFragment [NFC].
Mon, Apr 23, 7:24 AM
peter.smith created D45960: [MC] Add MCSubtargetInfo to MCPaddingFragment [NFC].
Mon, Apr 23, 7:13 AM
peter.smith added a dependent revision for D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup: D45960: [MC] Add MCSubtargetInfo to MCPaddingFragment [NFC].
Mon, Apr 23, 7:13 AM
peter.smith added a dependent revision for D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup: D45959: [MC] Move bundling and MCSubtargetInfo to MCEncodedFragment [NFC].
Mon, Apr 23, 7:08 AM
peter.smith created D45959: [MC] Move bundling and MCSubtargetInfo to MCEncodedFragment [NFC].
Mon, Apr 23, 7:08 AM
peter.smith added a comment to D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

Thanks for the review. I'll wait a bit to see if there is a second opinion, I too would value more input on instruction bundling as I've not used it myself. I've completed a set of patches that will pass through the subtarget to writeNops() and remove STI from the X86 and ARM AsmBackends, these have some further reorganisation of the position of instruction bundling inside the MCFragment hierarchy. I'll add these as dependent reviews to this one and link them in a comment.

Mon, Apr 23, 3:49 AM

Thu, Apr 19

peter.smith added a comment to D45164: [MC] Change AsmParser to leverage Assembler during evaluation.

Thanks for adding the test, it looks like it will cover my concern.

Thu, Apr 19, 2:14 AM
peter.smith added a comment to D45791: Cache getSymVA.

As well as Thunks and errata patching, I'm guessing that any future generic (not just Android) uses of packed dynamic relocs might also need symbol addresses to be recalculated (https://sourceware.org/ml/gnu-gabi/2017-q2/msg00000.html)

Thu, Apr 19, 1:59 AM

Wed, Apr 18

peter.smith updated the diff for D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

I've updated the diff to account for instruction bundling. When an instruction must be put in the same fragment for the purposes of bundling we now check that the MCSubtargetInfo for the fragment is consistent. I've added some tests to check that we detect an attempt to change subtarget within a bundle.

Wed, Apr 18, 9:38 AM

Tue, Apr 17

peter.smith added a comment to D45164: [MC] Change AsmParser to leverage Assembler during evaluation.

If I've understood correctly, this will evaluate the expression if there is something simple and unrelaxable such as (Armv7a)

Tue, Apr 17, 8:46 AM

Tue, Apr 10

peter.smith added a comment to D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

Thanks for the comments. I think your second one brings up a couple of wider problems that I'll need to address:

  • getOrCreateDataFragment() needs to start a new fragment when the original contains instructions and the new one has a different Subtarget. Otherwise the later Subtarget will overwrite the first, I don't think that this makes any practical difference for the MCAsmBackend functions changed in this patch, however it will do for the WriteNops().
  • For instruction bundling in cases where the previous fragment must be used then it must either have the same Subtarget, or we permit the later Subtarget to overwrite the first.
Tue, Apr 10, 7:32 AM

Fri, Apr 6

peter.smith updated the diff for D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

I've updated the diff, description and title to include passing through MCSubtargetInfo to applyFixup() as well. I think that it is worth combining these changes into one as it is possible to introduce a code generation fault with just fixupNeedsRelaxation() receiving the STI.

Fri, Apr 6, 5:44 AM
peter.smith added a comment to D45195: Add --check-library-dependency to maintain compatibility with other linkers.

--find-library-backreferences might be indeed better than --check-library-dependency, but both options are perhaps too long as a Unix linker option? How about --detect-backrefs? Maybe just --backref?

Fri, Apr 6, 1:32 AM

Thu, Apr 5

peter.smith added a comment to D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

I think that an EmitAssemblerFlag that is used in the same way as the flag to switch from Thumb to Arm can toggle the support for the Architectural NOP without having to do large amounts of source code changes, most of them outside the ARM backend.

This is really hacky... the fragments in question should really have an STI instead. Plus there are actually separate features; whether NOP is supported in ARM mode (which is what hasNOP queries), and whether NOP is supported in Thumb mode (i.e. whether Thumb2 is supported). Plus other architectures have similar problems (e.g. x86 changes the generated NOPs based on the target). If you're not going to fix it properly, please just leave it as-is for now, with a FIXME.

Thu, Apr 5, 9:24 AM
peter.smith added a comment to D45195: Add --check-library-dependency to maintain compatibility with other linkers.

Perhaps --check-library-dependency is too strong a name for the cases that this can catch. I think it is really a --find-library-backreferences that can be helpful as part of resolving differences in behaviour between lld and other unix linkers, but there are limitations.

Thu, Apr 5, 2:47 AM

Wed, Apr 4

peter.smith added a comment to D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

I've done a bit of thinking about how best to get the necessary information to ARMAsmBackend::writeNopData and ARMAsmBackend::applyFixup. I made an attempt at passing MCSubtargetInfo through the necessary layers so that STI could be removed. I think that this is impractical for writeNopData, although possible for applyFixup. What I propose to do:

  • The writeNopData() currently checks to see if the Architectural NOP instruction is supported, based on the MCSubtargetInfo in ARMAsmBackend. I think that an EmitAssemblerFlag that is used in the same way as the flag to switch from Thumb to Arm can toggle the support for the Architectural NOP without having to do large amounts of source code changes, most of them outside the ARM backend.
  • All the fixups that test the MCSubTargetInfo for target features are for instruction fixups, these are covered by the MCRelaxableFragment type that already has STI embedded in it. Rather than adding STI to the Data Fragments with relaxations I propose to pass a pointer to STI that will be null for Data Fragments.
Wed, Apr 4, 10:32 AM

Tue, Apr 3

peter.smith added a comment to D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

Thanks for the comments and apologies for the delayed response (Easter break). The patch here is somewhat fortunate in that the MCRelaxableFragment already has a reference to the MCSubtargetInfo. The other instances ARMAsmBackend::writeNopData and ARMAsmBackend::applyFixup may be called on other Fragments that don't have the MCSubtargetInfo member such as MCAlignFragment and MCDataFragment. I'll do some investigation to see what the best options are.

Tue, Apr 3, 3:39 AM

Thu, Mar 29

peter.smith added a comment to D44963: ELF: Add support for short thunks on ARM..

Thanks for the update, I'm happy with the changes and the new test case.

Thu, Mar 29, 3:18 AM
peter.smith added a comment to D44962: ELF: Allow thunks to change size. NFCI..

Thanks, I'm happy with the changes.

Thu, Mar 29, 3:07 AM
peter.smith accepted D44815: [AArch64]: Add support for parsing rN registers..

Given that this is important for compiling the Linux kernel with clang I think that it is worth improving compatibility with GCC. LGTM.

Thu, Mar 29, 3:04 AM

Wed, Mar 28

peter.smith added a comment to D44969: ELF: Place ordered sections in the middle of the unordered section list on targets with limited-range branches..

That looks reasonable to me. Another heuristic that can be used in at least C programs is to look for the sections with the most number of calls to them and place these in the centre. This tends to reduce the number of thunks as many library functions are called from all over the program, however I think the performance loss of going through a thunk means that moving the hot functions makes more sense.

Wed, Mar 28, 10:31 AM
peter.smith added a comment to D44963: ELF: Add support for short thunks on ARM..

Thanks for putting in the effort to doing this, especially updating the tests! This can also apply to AArch64 as well. The only concern that I have right now are whether it is possible to construct an example that oscillates between short and long thunks and would infinite loop if it were not for the iteration count. In general transitioning from a short thunk to a large thunk will increase the distance between sections, however there is one case where adding a thunk, or increasing the distance can shorten a distance.

Wed, Mar 28, 10:17 AM
peter.smith added a comment to D44966: ELF: Try to create last thunk section at ThunkSectionSpacing bytes before the end..

This sounds reasonable to me. For large programs there will be sufficient thunks that the small benefit to readability (all your thunks can be found at the end of the section) won't apply.

Wed, Mar 28, 4:16 AM
peter.smith added inline comments to D44962: ELF: Allow thunks to change size. NFCI..
Wed, Mar 28, 4:01 AM
peter.smith added a comment to D44899: [ELF] - Print LMA in a -Map file..

With respect to parsing, at a recent conference one of the main requirements embedded developers had on ld.bfd was a machine readable (xml, json, etc.) map file. If ease of parsing is a concern, could we separate those concerns and have an alternative way to output the necessary information?

Wed, Mar 28, 2:27 AM

Tue, Mar 27

peter.smith added a comment to D44899: [ELF] - Print LMA in a -Map file..

It can be very useful when developing embedded systems. There is often a case where the load address of an output section is in flash and it is copied to a different execution address in sdram. Seeing the load address in the map file can be useful when debugging problems, especially when the device won't boot up. It is probably sufficient to give the LMA of each OutputSection as the LMA of the input sections can be derived from it.

Tue, Mar 27, 10:40 AM
peter.smith added a comment to D44815: [AArch64]: Add support for parsing rN registers..

Thanks for the clarification. With that in mind I'm much less concerned that adding "r" as an alias will make extra warnings more difficult. I agree that there should be at least a warning for register long x asm ("wn") although that is separate from this patch. Has anyone else got any objections?

Tue, Mar 27, 10:27 AM
peter.smith added a comment to D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

Thanks for pointing that out. I should check each use of the member to see if the information can be used incorrectly. I think it would be good if we can remove the MCSubtargetInfo, I have my suspicions that it might be used from Assembly and for BuildAttributes generation (on Arm), again some more checking is needed.

Tue, Mar 27, 8:23 AM
peter.smith created D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.
Tue, Mar 27, 6:34 AM

Mon, Mar 26

peter.smith added a comment to D44815: [AArch64]: Add support for parsing rN registers..

I think that we may have a bit of a conceptual difference with GCC here. As far as I can tell in GCC it doesn't matter if "rn", "wn", "xn" or even "bn" is used, this refers to register 0. The Operand constraint such as %w0 is used to control the substitution so the r registers aren't strictly aliases like the other registers. The comment

The S/D/Q and W/X registers overlap, but aren't really aliases; we
don't want to substitute one of these for a different-sized one.

suggests that this may have been the intent that Clang behave differently to GCC in this respect. For example in Clang there appears to be a bit more significance placed on the "wn" or "xn", for example clang will warn in the following example, whereas gcc will not:

Mon, Mar 26, 11:46 AM
peter.smith added a comment to D43860: [AArch64] DWARF: do not generate AT_location for thread local.

I had a quick look at the PR last week after some people mentioned that they were seeing link errors in their project and were blaming the linker and not the compiler. I'm not a debug expert so it would be best to get someone else to look over the test case. I think the source code changes are probably the best that we can do right now.

Mon, Mar 26, 1:49 AM
peter.smith accepted D44819: [ARM] Simplify constructing the ARMArchFeature string. NFC..

Looks good to me.

Mon, Mar 26, 1:16 AM

Mar 21 2018

peter.smith added a comment to D43005: [ARM] Error out on .arm assembler directives on windows.

No performance implications that I know of, the source code history might contain some clues. I'm guessing that the function wouldn't be significant in the context of a compile so I personally think it would be good to clean it up.

Mar 21 2018, 1:58 AM
peter.smith added a comment to D43005: [ARM] Error out on .arm assembler directives on windows.

Thanks, I'm happy with that. Will be worth waiting to see if there are any comments from the US timezones.

Mar 21 2018, 1:15 AM

Mar 20 2018

peter.smith added a comment to D43005: [ARM] Error out on .arm assembler directives on windows.

Looks a lot better than before. One possible alternative to making the subtarget then subtracting ARM, could we use the same way that nacl adds the nacl-trap feature in ParseARMTriple?

Mar 20 2018, 11:30 PM

Mar 14 2018

peter.smith added a comment to D44355: [AArch64] Fold adds with tprel_lo12_nc and secrel_lo12 into a following ldr/str.

https://sourceware.org/bugzilla/show_bug.cgi?id=22969 and https://sourceware.org/bugzilla/show_bug.cgi?id=22970 raised on gold and ld.bfd respectively.

Mar 14 2018, 7:20 AM
peter.smith added a comment to D44355: [AArch64] Fold adds with tprel_lo12_nc and secrel_lo12 into a following ldr/str.

I've raised https://bugs.llvm.org/show_bug.cgi?id=36727 to get the relocation implemented in LLD. Will need to raise issues in Gold and BFD as well.

Mar 14 2018, 7:08 AM
peter.smith added a comment to D44355: [AArch64] Fold adds with tprel_lo12_nc and secrel_lo12 into a following ldr/str.

Unfortunately this change looks like it is going to be incompatible with binutils and LLD for the ELF targets as the ldr . The new output via llvm-objdump is:

Mar 14 2018, 5:21 AM

Mar 13 2018

peter.smith added a comment to D44422: [ELF] - Never create .gnu_hash with NBuckets == 0..

I think that this is the simplest change that will pass through Bionic loaders check. The other possibility I thought of which might be a bit more honest is to add one of the symbols to the table even if it were a reference, that would minimise the risk of some other tool from complaining about hash table structure problems.

Mar 13 2018, 8:18 AM

Mar 12 2018

peter.smith added a comment to D44377: [ELF] - Drop SHF_LINK_ORDER flag from the output..

Looks ok to me. I agree that the GNU strip warning is not helpful for executables or shared libraries. When I initially put in support for SHT_ARM_EXIDX I left the flag in as I remembered seeing one of the gnu tools give such a warning but I couldn't remember which one, looks like it was strip.

Mar 12 2018, 5:54 AM

Mar 9 2018

peter.smith updated the diff for D44259: [LLD][ELF] Use start of .got.plt as the location for _GLOBAL_OFFSET_TABLE_.

Thanks for the comment, and apologies for my misreading of the binutils source. I've updated the patch to keep Mips as it was previously.

Mar 9 2018, 3:10 AM
peter.smith added a comment to D44284: ELF: Do not create multiple thunks for the same virtual address..

Thanks for the optimisation, LGTM too.

Mar 9 2018, 2:12 AM

Mar 8 2018

peter.smith created D44259: [LLD][ELF] Use start of .got.plt as the location for _GLOBAL_OFFSET_TABLE_.
Mar 8 2018, 10:07 AM

Feb 27 2018

peter.smith added a comment to D43005: [ARM] Error out on .arm assembler directives on windows.

To clarify further - if I invoke the assembler as in the testcase I'm adding here, llvm-mc -triple=thumbv7-win32 < %s (or clang -target armv7-win32 -c file.S, which internally remaps it to thumbv7-win32 before it hits llvm), without manually setting the flag, what class/file should set it?

Feb 27 2018, 9:30 AM
peter.smith added a comment to D43005: [ARM] Error out on .arm assembler directives on windows.

Apologies to take so long to get back to you. I've had a chance to take a look. While I've not got a magic solution I do have some questions/suggestions.

We already set the NoARM variable in ARMSubtarget but we don't clear the FeatureNoARM feature bit. Clearing it from there doesn't seem to be enough when assembling .s files from clang though.

If I set the noarm feature string directly for example --triple=armv7-none-eabi -mattr=+noarm then I get the desired behaviour on your test case. It might be that setting the feature bit directly is insufficient as it may get recalculated later from the triple and mattr, but if you can add +noarm to the mattr feature string this should keep the noarm feature set.
The NoARM and corresponding hasARMOps() in its current form looks like it could and probably should be removed.

  • NoARM is only set when isTargetWindows(), so hasARMOps() is effectively !isTargetWindows(), to me this makes the name hasARMOps() misleading. If it is truly hasARMOps() then it should be calculated from the feature bits.
  • hasARMOps() is only used in a few places and could be easily replaced by !isTargetWindows() if that is the intention.
Feb 27 2018, 8:23 AM
peter.smith accepted D43665: Take SHF_ARM_PURECODE into consideration when setting the program header flags.

Thanks very much. Looks good to me.

Feb 27 2018, 2:44 AM

Feb 23 2018

peter.smith added a comment to D43665: Take SHF_ARM_PURECODE into consideration when setting the program header flags.

Thanks for doing this. It is close, but I think that it will clear the PF_R flag too aggressively at the moment. In the comments I've put a link to a very short white paper and a link to the entry in Arm ELF which unfortunately uses an old name for the flag SHF_ARM_NOREAD, first introduced by Arm's proprietary compiler but later changed to SHF_ARM_PURECODE when implemented in gcc.

Feb 23 2018, 2:10 AM

Feb 22 2018

peter.smith added a comment to D43005: [ARM] Error out on .arm assembler directives on windows.

I agree that we should try to fix this generally rather than hardcode this to Windows ARM.

Any concrete suggestions on how to go about it? The flag ARM::FeatureNoARM is generally assigned based on the cpu in ARM.td.

Apologies I haven't got ideas off the top of my head. I may be able to find some time tomorrow or early next week to take a look and make some suggestions.

@peter.smith - any ideas?

My apologies, I've not had a chance to look yet, my current task is taking rather longer than I'd like. I'm intending to take a look when I've finished, hopefully next week, although don't let that stop you taking a look yourself.

Feb 22 2018, 3:00 AM

Feb 12 2018

peter.smith added a comment to D43126: [LLD][ELF] Do not error for missing version when symbol has local version..

Peter Smith via Phabricator <reviews@reviews.llvm.org> writes:
Index: ELF/Symbols.cpp
===================================================================

    • ELF/Symbols.cpp +++ ELF/Symbols.cpp @@ -206,8 +206,10 @@ It is an error if the specified version is not defined. Usually version script is not provided when linking executable, // but we may still want to override a versioned symbol from DSO,
  • // so we do not report error in this case.
  • if (Config->Shared) + so we do not report error in this case. We also do not error + if the symbol has a local version as it won't be in the dynamic + // symbol table. + if (Config->Shared && VersionId != VER_NDX_LOCAL)

Is the Config->Shared part still required?

Feb 12 2018, 5:41 AM

Feb 9 2018

peter.smith created D43126: [LLD][ELF] Do not error for missing version when symbol has local version..
Feb 9 2018, 6:34 AM
peter.smith accepted D43116: [ELF] Print the .type assembly directive correctly for STT_NOTYPE.

LGTM. I can confirm that gas uses notype and that the other .type values also match those in gas.

Feb 9 2018, 5:26 AM
peter.smith added a comment to D43071: [ELF] - Support COPY, INFO, OVERLAY output sections attributes..

I mean, for any language, if you google for some extremely minor feature of that language, I'm pretty sure that you can always find one. So the fact that you can find a use case on the internet isn't very convincing that we should support it.

Feb 9 2018, 3:44 AM

Feb 8 2018

peter.smith added a comment to D43071: [ELF] - Support COPY, INFO, OVERLAY output sections attributes..

For what it is worth COPY and OVERLAY are used quite a bit in embedded systems. The Arm GCC Embedded toolchain linker scripts in the sample directory use COPY to reserve an area for the heap and stack https://developer.arm.com/open-source/gnu-toolchain/gnu-rm/downloads

Feb 8 2018, 9:59 AM
peter.smith added a comment to D43005: [ARM] Error out on .arm assembler directives on windows.

I agree that we should try to fix this generally rather than hardcode this to Windows ARM.

Any concrete suggestions on how to go about it? The flag ARM::FeatureNoARM is generally assigned based on the cpu in ARM.td.

Feb 8 2018, 8:42 AM

Feb 7 2018

peter.smith added a comment to D43005: [ARM] Error out on .arm assembler directives on windows.

My initial thought is that there must be a better way than hard-coding a windows OS choice in the hasARM() field, particularly as all the others work entirely from the feature bits. Do you happen to know why clearing the feature bits is insufficient? I haven't touched this part of the code for a long time so I haven't got an answer for why myself. I can see that the assembler might recalculate them from the CPU description when particular directives are used.

Feb 7 2018, 2:40 AM

Feb 6 2018

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

cpirker, as the original author do you have any concerns about changing the big-endian emulation name to match binutils?

Feb 6 2018, 2:11 AM
peter.smith added a comment to D42930: [Aarch64] Fix linker emulation for Aarch64 big endian.

I can confirm that binutils use aarch64linuxb as their target linux emulation and not aarch64_be_linux. I checked the original commit http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20140310/101365.html , this has references to ld-linux-aarch64_be.so which does seem to be mentioned in binutils and the directory "/lib/aarch64_be-linux-gnu" that I couldn't find any reference to, or any close equivalent of that directory mentioned in binutils. It may be worth checking that as well.

Feb 6 2018, 2:09 AM

Feb 2 2018

peter.smith updated the diff for D42797: [LLD][ELF] Implement --[no-]apply-dynamic-relocs option..

Thanks to both for the comments. I've incorporated them in the diff.

Feb 2 2018, 2:08 AM

Feb 1 2018

peter.smith created D42797: [LLD][ELF] Implement --[no-]apply-dynamic-relocs option..
Feb 1 2018, 8:00 AM

Jan 24 2018

peter.smith added a comment to D42471: [ARM] Fix lld crash introduced by r321154.

Thanks for the change. I've no further comments.

Jan 24 2018, 10:00 AM · lld
peter.smith added a comment to D42471: [ARM] Fix lld crash introduced by r321154.

Thanks for adding the test. I've added a small suggestion in an inline comment.

Jan 24 2018, 9:10 AM · lld
peter.smith added a comment to D42471: [ARM] Fix lld crash introduced by r321154.

Thanks for sending the patch. What I'd like to know first is in what circumstances the synthetic section has no parent and is this expected to be possible? If it isn't expected to be possible then it would be better to fix that and not try to account for the no parent case. The implementation has changed quite a bit since I last looked so I'm not sure of the intention.

Jan 24 2018, 5:55 AM · lld
peter.smith accepted D42196: [compiler-rt] [builtins] Align addresses to cache lines in __clear_cache for aarch64.

Yes I'm happy to approve.

Jan 24 2018, 2:11 AM

Jan 23 2018

peter.smith updated the diff for D42421: [LLD][ELF] Make --fix-cortex-a53-843419 work on big endian hosts.

Spoke to soon, forgot to correct the last ulittle32_t that was hiding from me. Now updated.

Jan 23 2018, 8:32 AM
peter.smith updated the diff for D42421: [LLD][ELF] Make --fix-cortex-a53-843419 work on big endian hosts.

+ const ulittle32_t *InstBuf = reinterpret_cast<const ulittle32_t *>(Buf + Off);

This is OK.

+ ulittle32_t Instr1 = *InstBuf++;
+ ulittle32_t Instr2 = *InstBuf++;
+ ulittle32_t Instr3 = *InstBuf++;

But the values should still be uint32_t. The endian.h types should only
ever be used to form pointers.

Cheers,
Rafael

Jan 23 2018, 8:28 AM
peter.smith created D42421: [LLD][ELF] Make --fix-cortex-a53-843419 work on big endian hosts.
Jan 23 2018, 7:25 AM
peter.smith added a comment to D42196: [compiler-rt] [builtins] Align addresses to cache lines in __clear_cache for aarch64.

I guess this comes down to the interpretation of (begin, end] when you can only clear a cache line at a time. I think that this makes sense as it matches the powerpc64 below, Linux also follows this approach. I'm not sure how this is done on BSD and Darwin.

Jan 23 2018, 3:00 AM

Dec 15 2017

peter.smith updated the diff for D41246: [LLD][ELF] Optimize Arm PLT entries.

I've implemented the suggestion to choose the form of PLT entry based on whether the offset to the .got.plt can be represented by the shorter encoding. To maintain 16-byte alignment of the entries I've increased the size of the header to 32-bytes. As the --long-plt doesn't do anything any more I've updated all the tests to use the new encoding. Apologies for the size of the diff.

Dec 15 2017, 9:12 AM
peter.smith abandoned D35413: [LLD][ELF] Add DefinedInThunk to SymbolBody to remove need for hash lookup.

I don't think that there was consensus over whether this change was a good idea. I'm abandoning it for now, it can be resurrected if needed.

Dec 15 2017, 3:19 AM
peter.smith abandoned D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR.

This revision was split up and fixed in D40359 D40365 and D40735 abandoning as it is no longer needed.

Dec 15 2017, 3:17 AM

Dec 14 2017

peter.smith added a comment to D41246: [LLD][ELF] Optimize Arm PLT entries.
In D41246#955551, @pcc wrote:

Instead of making the behaviour conditional on the flag, could we always emit the long PLT header and then conditionally emit short or long PLTs depending on what the offset is?

Dec 14 2017, 10:29 AM
peter.smith created D41246: [LLD][ELF] Optimize Arm PLT entries.
Dec 14 2017, 9:32 AM
peter.smith updated the diff for D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.

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

Dec 14 2017, 8:47 AM
peter.smith added a comment to D41234: [ELF] Fix placement of a sentinel entry in the .ARM.exidx section..

This looks sensible to me. One minor nit with the comment as the ABI does not mention a sentinel. We add it as some recent versions of libunwind depend on it being there (due to gold and ld.bfd generating one).

Dec 14 2017, 7:11 AM · lld
peter.smith updated the diff for D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.

I think you forgot to upload th new patch.

Cheers,
Rafael

Dec 14 2017, 1:56 AM

Dec 13 2017

peter.smith added a comment to D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.

Comments from Rafael via [llvm-commits]

+ struct ExidxEntry {
+ uint32_t Fn;
+ uint32_t Unwind;
+ };
+
+ // Get the last table Entry from the previous .ARM.exidx section.
+ const ExidxEntry PrevEntry = *reinterpret_cast<const ExidxEntry *>(
+ Prev->Data.data() + Prev->getSize() - sizeof(ExidxEntry));
+ if (IsExtabRef(PrevEntry.Unwind))
+ return false;

This is not safe in a big endian host, no?

Dec 13 2017, 3:49 AM
peter.smith added a comment to D41105: [ELF] Prevent crash in writing an .ARM.exidx sentinel entry..

That looks correct to me. In the test I think it would be useful to check the contents of the table llvm-objdump -s or llvm-readobj -u to check that the sentinel is written in the correct place.

Dec 13 2017, 3:09 AM · lld

Dec 12 2017

peter.smith updated the diff for D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.

Thanks very much for the review comments. I've updated the diff to apply them. The one I haven't done yet is std::unique as I don't think I can make the predicate an equivalence relation without missing opportunities. For an example consider two consecutive (made up) unwind entries { 0xabababab, 0x1 } and { 0x1, 0x1, 0x1 }; I can merge the second into the first but not the first into the second. My guess is that the libstdc++ and libc++ implementations of std::unique will let us get away with the predicate not being symmetric but I have memories of the Visual Studio debug implementation actively checking all the preconditions required by the standard.

Dec 12 2017, 3:15 AM
peter.smith added a comment to D41105: [ELF] Prevent crash in writing an .ARM.exidx sentinel entry..

I think that this is the right thing to do for .ARM.exidx sections. I'm pretty sure the original implementation did do something like that, but it must have been refactored sometime later to use addSection(). I don't think that this is expected for orphans in general though. For example consider the silly example: SECTIONS { .text : { t.o(.text) foo = .; } }
If I provide an object t.o with a .text section and an object t2.o with a .text section, the t2.o will be an orphan that will match in .text. The bfd linker places this after the assignment to foo = .

Dec 12 2017, 2:09 AM · lld

Dec 11 2017

peter.smith added a comment to D38361: [ELF] Stop setting output section size early.

No objections from me. I've successfully rebased D40964 onto this change.

Dec 11 2017, 9:12 AM
peter.smith updated the diff for D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC].

Rebased in light of changes to D38361. We know use the OutSecOff field for comparisons of .ARM.exidx sections.

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

Comments from Rafael

Dec 11 2017, 8:16 AM

Dec 8 2017

peter.smith updated the diff for D36749: [LLD][ELF][AArch64] Complete implementation of -fix-cortex-a53-843419.

Updated patch to remove the assumption that a single .text section is less than 128Mb, and added test case to cover. The change just inserts patches at roughly branch range intervals rather than always putting them at the end.

Dec 8 2017, 3:15 AM

Dec 7 2017

peter.smith added a dependent revision for D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC]: D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.
Dec 7 2017, 9:30 AM
peter.smith added a dependency for D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections: D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC].
Dec 7 2017, 9:30 AM
peter.smith updated the summary of D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.
Dec 7 2017, 9:29 AM
peter.smith created D40967: [LLD][ELF] Remove Duplicate .ARM.exidx sections.
Dec 7 2017, 9:29 AM
peter.smith added a dependent revision for D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC]: D40966: [LLD][ELF] Refactor to remove loop copying all Sections in OS->finalize() [NFC].
Dec 7 2017, 9:21 AM
peter.smith added a dependency for D40966: [LLD][ELF] Refactor to remove loop copying all Sections in OS->finalize() [NFC]: D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC].
Dec 7 2017, 9:21 AM
peter.smith updated the summary of D40966: [LLD][ELF] Refactor to remove loop copying all Sections in OS->finalize() [NFC].
Dec 7 2017, 9:21 AM
peter.smith created D40966: [LLD][ELF] Refactor to remove loop copying all Sections in OS->finalize() [NFC].
Dec 7 2017, 9:19 AM
peter.smith added a dependency for D40964: [LLD][ELF] Move SHF_LINK_ORDER processing earlier in Writer.cpp [NFC]: D38361: [ELF] Stop setting output section size early.
Dec 7 2017, 9:17 AM