peter.smith (Peter Smith)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 11 2016, 8:56 AM (114 w, 6 d)

Recent Activity

Fri, Jun 15

peter.smith added a comment to D48146: ELF: Implement --icf=safe using address-significance tables..

Thanks very much for the update. I don't have any more comments at this stage.

Fri, Jun 15, 1:25 AM

Thu, Jun 14

peter.smith added a comment to D45959: [MC] Move bundling and MCSubtargetInfo to MCEncodedFragment [NFC].

Thanks, I'll wait till tomorrow before committing to see if anyone in the US timezone has any objections.

Thu, Jun 14, 8:56 AM
peter.smith added a comment to D48146: ELF: Implement --icf=safe using address-significance tables..

Thanks for the patches. Out of curiosity did you have any thoughts about how the proposal to ELF was going? https://groups.google.com/forum/#!topic/generic-abi/MPr8TVtnVn4 . In particular compressed SHT_SYMATTR seemed promising.

Thu, Jun 14, 2:26 AM

Tue, Jun 12

peter.smith created D48077: [LNT] Allow --use-perf=profile and --run-under to work together.
Tue, Jun 12, 7:12 AM

Tue, Jun 5

peter.smith added reviewers for D47748: [lld] Add REQUIRES: x86 where needed to tests: ruiu, grimar.

This looks good to me. I've checked that each of the files has some requirement such as an x86 target triple. I've added the code owner Rui and a frequent contributor George to see if they have any objections.

Tue, Jun 5, 1:31 AM · lld

Mon, Jun 4

peter.smith updated the diff for D46305: [MC][ARM] Correct Thumb BL instruction range.

Thanks for the comments I've updated the diff to show:

  • No longer rebased on D44928
  • We now allow architecture v6m to have the long range BL (I checked in the Arm ARM and this is true for v6m)
  • Test updated for v6m
  • Removed extra tests for .arch switching as these are dependent on D44928 landing.
Mon, Jun 4, 4:19 AM

Fri, Jun 1

peter.smith added a comment to D46305: [MC][ARM] Correct Thumb BL instruction range.

Thanks for the review, the code here is rebased on top of D44928 (not landed yet) which passes STI in as a pointer. The existing code has STI as a member (const reference) so I'd need to change STI-> to STI. but nothing else would change.

Fri, Jun 1, 6:11 AM
peter.smith accepted D47373: [ELF] - Also use DW_AT_linkage_name when gathering information about variables for error messages..

This looks sensible to me. If I've got this right when the DW_AT_linkage name differs from the DW_AT_name the symbol name will match the DW_AT_linkage_name so the lookup by symbol name won't succeed if done via DW_AT_name.

Fri, Jun 1, 3:06 AM
peter.smith added a comment to D47507: [MC] [X86] Teach leaq _GLOBAL_OFFSET_TABLE(%rip), %r15 to use R_X86_64_GOTPC32 instead of R_X86_64_PC32.

Apologies I don't know x86 well enough to be authoritative here. It looks like at least the test case needs to use %rip to see a meaningful difference in the output of mc.

Fri, Jun 1, 2:18 AM

Tue, May 29

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

Rebased [NFC]

Tue, May 29, 9:37 AM
peter.smith updated the diff for D45961: [MC] Add MCSubtargetInfo to MCAlignFragment [NFC].

Rebased [NFC]

Tue, May 29, 9:37 AM
peter.smith updated the diff for D45960: [MC] Add MCSubtargetInfo to MCPaddingFragment [NFC].

Rebased [NFC]

Tue, May 29, 9:36 AM
peter.smith updated the diff for D45959: [MC] Move bundling and MCSubtargetInfo to MCEncodedFragment [NFC].

Rebased [NFC]

Tue, May 29, 9:35 AM
peter.smith added a comment to D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

Ping. Is anyone willing to approve or has any thoughts on a better design?

Tue, May 29, 9:19 AM
peter.smith added a comment to D47098: [ELF] Support R_X86_64_GOTPC{32,64}.

I wonder if the next would be the correct way to solve this:

As far I understand, _GLOBAL_OFFSET_TABLE_ seems intended to be used with special types of relocations (this needs clarification).
R_X86_64_GOTPC64/R_X86_64_GOTPC32 and you mentioned R_ARM_BASE_PREL for arm, for example (your D46319 seems to make sense to me)

Tue, May 29, 8:49 AM
peter.smith added a comment to D47098: [ELF] Support R_X86_64_GOTPC{32,64}.

I am not sure that the second way should work (if I understand everything correctly).
For x86 as you know we define _GLOBAL_OFFSET_TABLE_ in the .got.plt[0] now.
Then for example if we have the following TLS code:

addl $_GLOBAL_OFFSET_TABLE_, %ebx #add    $0x199f,%ebx, R_386_GOTPC
subl $8, %esp                                         #sub    $0x8,%esp
leal gd@tlsgd(%ebx), %eax                    #lea    -0x10(%ebx),%eax, R_386_TLS_GD

it expects the first line to be resolved to the end of .got (to the GOT base in ABI terminology).
If we redefine the relocations that use _FROM_END to use the location of the _GLOBAL_OFFSET_TABLE_ (I think that is what this patch tried to do),
then it will not help us I think as we will end up with the address of .got.plt at the first line. And the code will fail to find a proper TLS slot as a result since .got.plt
can be far away from .got in LLD.

Tue, May 29, 6:25 AM
peter.smith added a comment to D47098: [ELF] Support R_X86_64_GOTPC{32,64}.

Apologies for the delay in responding, Monday was a public holiday in the UK. I'm not sure if I have much to add over what has been already said. My understanding from when I looked at pr36555:

  • Each target has a convention on where _GLOBAL_OFFSET_TABLE_ is placed.
  • glibc uses _GLOBAL_OFFSET_TABLE_[0] to find where it has been loading, it expects the link time address of _DYNAMIC. This seems to be the case for all targets.
  • glibc reserves entries in the .got.plt to support lazy loading, these seem to be accessed via the dynamic tag DT_PLTGOT rather than relative to _GLOBAL_OFFSET_TABLE_ despite the comment mentioning _GLOBAL_OFFSET_TABLE_[1] and _GLOBAL_OFFSET_TABLE_[2], I suspect that some time ago these used _GLOBAL_OFFSET_TABLE_[0] but was at some point switched to .got.plt[1] and .got.plt[2] with .got.plt[0] possibly used for _GLOBAL_OFFSET_TABLE_[0] if the target sets _GLOBAL_OFFSET_TABLE_ to .got.plt.
  • Some targets have a convention of loading the value of _GLOBAL_OFFSET_TABLE_ and accessing variables using some offset from the _GLOBAL_OFFSET_TABLE_.
  • LLD initially set _GLOBAL_OFFSET_TABLE_ to 0 and defined some of the relocation expressions from the end of the .got.
    • This works for most programs as the precise value of _GLOBAL_OFFSET_TABLE_ isn't used, as long as the relocations that are calculated relative to it come out consistently. This obviously doesn't work for glibc as it uses _GLOBAL_OFFSET_TABLE_[0] directly.
Tue, May 29, 4:29 AM

May 24 2018

peter.smith accepted D47217: [cmake] [ARM] Check if VFP is supported before including any VFP builtins.

Thanks for the update. That looks good to me.

May 24 2018, 2:38 AM

May 23 2018

peter.smith added a comment to D47217: [cmake] [ARM] Check if VFP is supported before including any VFP builtins.

I think that this is a sensible thing to do and it also helps out when trying to build compiler-rt for armv7-m as compiler-rt currently assumes that you have a target with a VFP when many armv7-m devices do not.

May 23 2018, 3:59 AM

May 21 2018

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

Thanks very much for the comments. Although the STI is available at the time the instruction is encoded; due to relaxation being a separate pass resolved after all instructions have been emitted we don't have the STI to pass to applyFixups() as all we have is a list of fragments. For example the MCRelaxableFragment has to store the STI as it might need to re-encode the instruction during relaxation. Thinking about some possible alternatives:

  • Make another pass over the input during relaxation (and at other times we need the STI, such as emitting NOPS in alignment padding), so that we know what STI is in effect at the time. I think that this would be a much larger change, in effect making the assembler 2 pass.
  • Maintain some kind of mapping table between fragment and STI. I think that this is logically equivalent to storing the STI in the fragment though.
  • Store the STI per MCInst, this seems wasteful and I don't think it would work for writeNops() as these come from alignment and padding fragments.
May 21 2018, 3:27 AM

May 18 2018

peter.smith accepted D46703: [MC] Relax .fill size requirements.

Thanks for the update, looks good to me.

May 18 2018, 1:28 AM

May 17 2018

peter.smith added a comment to D46703: [MC] Relax .fill size requirements.

Thanks for the update, typo aside they look fine to me.

May 17 2018, 3:01 AM

May 16 2018

peter.smith created D46932: [AArch64] Correct inline assembly test case for S modifier [NFC].
May 16 2018, 2:49 AM
peter.smith updated the diff for D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

I've rebased the patch against current trunk. The littleEndian parameter was removed from applyFixup, NFC for the patch.

May 16 2018, 1:57 AM

May 15 2018

peter.smith added a comment to D46703: [MC] Relax .fill size requirements.

Thanks for the update. I've left some minor comments but I'm overall happy with the change.

May 15 2018, 7:31 AM
peter.smith added a comment to D46755: [LLD][ELF] Implement --keep-unique option.

Thank you. I've made the suggested changes and committed under r332332

May 15 2018, 2:01 AM

May 14 2018

peter.smith added a comment to D46502: [ELF] - Fix for "LLD can create incorrect debug PC ranges for functions in Comdat groups.".

I'd agree that there might be some alternative way of doing this. I thought that, if there's some way to obtain a replacing comdat section from here

https://github.com/llvm-project/llvm-project-20170507/blob/master/lld/ELF/Symbols.cpp#L54

then we can get an address for a replaced comdat section which might solve the issue. But with the data structure it doesn't seem to be easy thing to do (and that's not lld's fault as it is for a compiler bug workaround).

May 14 2018, 9:01 AM
peter.smith updated the diff for D46755: [LLD][ELF] Implement --keep-unique option.

Thanks very much for the comments. I've made the following changes:

  • Used a bitfield for KeepUnique
  • moved findKeepUniqueSections to the driver
  • Simplified test case with nops, and removed absolute paths.
May 14 2018, 7:32 AM

May 11 2018

peter.smith created D46755: [LLD][ELF] Implement --keep-unique option.
May 11 2018, 6:59 AM
peter.smith added a comment to D43126: [LLD][ELF] Do not error for missing version when symbol has local version..

Thanks, I'll wait till next Monday before committing.

May 11 2018, 6:10 AM
peter.smith updated the diff for D43126: [LLD][ELF] Do not error for missing version when symbol has local version..

Rebased against master. I'd like to see if I can progress this a bit further as there is now interest in Android to switch to LLD and this issue is preventing some of Bionic from being linked with LLD.

May 11 2018, 4:06 AM
peter.smith created D46745: [AArch64] Support "S" inline assembler constraint.
May 11 2018, 3:35 AM
peter.smith added inline comments to D46703: [MC] Relax .fill size requirements.
May 11 2018, 2:57 AM
peter.smith added a comment to D46502: [ELF] - Fix for "LLD can create incorrect debug PC ranges for functions in Comdat groups.".

I tried this patch locally, experimented some other ideas, and do some benchmark. My feeling is that this patch is too much as a workaround for a buggy input. This is too intrusive, and it creates too many objects at runtime. For instance, Firefox contains almost 1 million discarded comdat sections, and creating "proxy" objects for them isn't negligible. You probably should fix the compiler instead of implementing a workaround to the linekr.

Thanks for looking, Rui. I agree that it this functionality is too hacky probably.

Let's see for people other opinions. If there will be no strong arguments to do something like that in the linker, I'll abandon this patch then.

May 11 2018, 2:30 AM

May 10 2018

peter.smith added a comment to D46502: [ELF] - Fix for "LLD can create incorrect debug PC ranges for functions in Comdat groups.".

I don't think there's a hack that you can easily remove. Once something lands to the linker's source code, that becomes a part of the implicit ABI, and you can never remove it unless it becomes really obsolete. Basically, you should assume that landing some feature or a workaround is a commitment that we will very likely to keep it for a long period of time.

I like this position. Then my question is do we want it? I feel it is a dirty hack. I do not know how much it is important to have/do not have for us though.

May 10 2018, 10:33 AM
peter.smith added a comment to D46502: [ELF] - Fix for "LLD can create incorrect debug PC ranges for functions in Comdat groups.".

My understanding is that it depends on what action the debugger is trying to do. If it is single-stepping through a function from the selected comdat group there will be at least one set of .debug sections that have the correct PC range so mappings from Address to Source will work. The difficulty is with the Source to Address mapping for the objects where the group was discarded. For example show me all the symbols defined in this source file will not find the comdat group as the PC range won't be correct.

I don't know if showing all symbols defined by some file is a common operation. Is it?

May 10 2018, 10:28 AM
peter.smith added a comment to D46502: [ELF] - Fix for "LLD can create incorrect debug PC ranges for functions in Comdat groups.".

I wonder if we can simply discard debug records that refer discarded sections. If comdat section is discarded because there's another copy of it, there should be debug records for that section, so we have two copies of debug records, thus we can discard debug records for a discarded comdat section, no?

May 10 2018, 9:51 AM
peter.smith added a comment to D46502: [ELF] - Fix for "LLD can create incorrect debug PC ranges for functions in Comdat groups.".

If I've understood correctly (from test/ELF/comdat.s) this will affect relocations from all sections to discarded local symbols. If I'm right I think that this is too wide a use case. My understanding is that in gold and bfd this case is specifically for relocations from Dwarf debug sections

Yes, but should we follow?

Such objects are broken by the specification. For broken objects, it is ok to produce broken output usually.

May 10 2018, 7:15 AM

May 8 2018

peter.smith added a comment to D46502: [ELF] - Fix for "LLD can create incorrect debug PC ranges for functions in Comdat groups.".

If I've understood correctly (from test/ELF/comdat.s) this will affect relocations from all sections to discarded local symbols. If I'm right I think that this is too wide a use case. My understanding is that in gold and bfd this case is specifically for relocations from Dwarf debug sections to discarded local symbols. Relocations from non-dwarf debug sections to discarded sections will be either an error or ignored depending on the use case.

May 8 2018, 6:04 AM

May 4 2018

peter.smith created D46429: [LLD][ELF][AArch64] Add aarch64_elf64_le_vec emulation.
May 4 2018, 6:13 AM

May 3 2018

peter.smith updated the diff for D46255: [ELF][AArch64] Implement the AArch64 TLSLD_LDST_LO12 family of relocs.

Updated diff to remove relocations from usesOnlyLowPageBits(). While this is true for these relocations the usesOnlyLowPageBits() function is never called for local exec TLS so by the same logic as applied to D46249 (don't add dead code) I've removed them from the patch.

May 3 2018, 7:10 AM
peter.smith abandoned D46249: [LLD][ELF][AArch64] Add missing LO12 relocation to usesOnlyLowPageBits.

Ok I'll abandon this one and I'll update D46255 to remove the new TLSLE relocations I added to usesOnlyLowPageBits as the same argument applies.

May 3 2018, 6:54 AM
peter.smith added a comment to D46249: [LLD][ELF][AArch64] Add missing LO12 relocation to usesOnlyLowPageBits.

Perhaps it is worth a comment instead as the function name implies a complete list. For example "// not a complete list, excludes relocations that can always be resolved statically such as TLS local exec."

Yeah, maybe. I guess it already contains entries it never uses, btw?

May 3 2018, 6:42 AM
peter.smith added a comment to D46249: [LLD][ELF][AArch64] Add missing LO12 relocation to usesOnlyLowPageBits.

Perhaps it is worth a comment instead as the function name implies a complete list. For example "// not a complete list, excludes relocations that can always be resolved statically such as TLS local exec."

May 3 2018, 6:21 AM

May 2 2018

peter.smith accepted D46355: [COFF, ARM64] Hook up a few remaining relocations.

Looks good to me. I checked the relocation against the table in https://msdn.microsoft.com/en-us/library/windows/desktop/ms680547(v=vs.85).aspx#base_relocation_types

May 2 2018, 6:11 AM
peter.smith updated the diff for D46305: [MC][ARM] Correct Thumb BL instruction range.

Thanks for the comments. I've rebased on top of D44928, only change to the source is STI. goes to STI->. I've added .arch directives to the test to check that we can switch and keep the correct check. By the way I'd be really grateful if you could accept D44928, I've been threatening to commit it unless there are any objections, but I'd feel better if the review was accepted.

May 2 2018, 4:02 AM

May 1 2018

peter.smith created D46319: [MC][ARM] Emit R_ARM_BASE_PREL for _GLOBAL_OFFSET_TABLE_ expressions.
May 1 2018, 9:11 AM
peter.smith created D46306: [MC][ARM] Add range checking for Thumb2 resolved fixups.
May 1 2018, 4:28 AM
peter.smith created D46305: [MC][ARM] Correct Thumb BL instruction range.
May 1 2018, 4:17 AM

Apr 30 2018

peter.smith created D46263: [LLD][ELF][AArch64] Increase test coverage of AArch64ErrataFix [NFC].
Apr 30 2018, 6:11 AM
peter.smith created D46261: [LLD][ELF] Add test cases to improve code coverage of Thunks [NFC].
Apr 30 2018, 4:20 AM
peter.smith created D46255: [ELF][AArch64] Implement the AArch64 TLSLD_LDST_LO12 family of relocs.
Apr 30 2018, 2:34 AM
peter.smith created D46249: [LLD][ELF][AArch64] Add missing LO12 relocation to usesOnlyLowPageBits.
Apr 30 2018, 1:51 AM
peter.smith created D46247: [LLD][ELF][AArch64] Simplify relocations sharing same encoding [NFC].
Apr 30 2018, 1:48 AM

Apr 27 2018

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

I've not had any more comments this week on the approach to instruction bundling. Would anyone have any objections to committing this next week as this will fix pr36542, and will at least not make the writeNops case any worse?

Apr 27 2018, 9:54 AM
peter.smith accepted D45164: [MC] Change AsmParser to leverage Assembler during evaluation.

I'm happy to LGTM. Apologies for the delay.

Apr 27 2018, 1:49 AM

Apr 23 2018

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.

Apr 23 2018, 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

Apr 23 2018, 7:35 AM
peter.smith added a dependent revision for D45960: [MC] Add MCSubtargetInfo to MCPaddingFragment [NFC]: D45962: [MC] Use local MCSubtargetInfo in writeNops.
Apr 23 2018, 7:29 AM
peter.smith created D45962: [MC] Use local MCSubtargetInfo in writeNops.
Apr 23 2018, 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.
Apr 23 2018, 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.
Apr 23 2018, 7:29 AM
peter.smith added a dependent revision for D45961: [MC] Add MCSubtargetInfo to MCAlignFragment [NFC]: D45962: [MC] Use local MCSubtargetInfo in writeNops.
Apr 23 2018, 7:29 AM
peter.smith created D45961: [MC] Add MCSubtargetInfo to MCAlignFragment [NFC].
Apr 23 2018, 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].
Apr 23 2018, 7:24 AM
peter.smith created D45960: [MC] Add MCSubtargetInfo to MCPaddingFragment [NFC].
Apr 23 2018, 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].
Apr 23 2018, 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].
Apr 23 2018, 7:08 AM
peter.smith created D45959: [MC] Move bundling and MCSubtargetInfo to MCEncodedFragment [NFC].
Apr 23 2018, 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.

Apr 23 2018, 3:49 AM

Apr 19 2018

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.

Apr 19 2018, 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)

Apr 19 2018, 1:59 AM

Apr 18 2018

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.

Apr 18 2018, 9:38 AM

Apr 17 2018

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)

Apr 17 2018, 8:46 AM

Apr 10 2018

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.
Apr 10 2018, 7:32 AM

Apr 6 2018

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.

Apr 6 2018, 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?

Apr 6 2018, 1:32 AM

Apr 5 2018

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.

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

Apr 5 2018, 2:47 AM

Apr 4 2018

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.
Apr 4 2018, 10:32 AM

Apr 3 2018

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.

Apr 3 2018, 3:39 AM

Mar 29 2018

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.

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

Thanks, I'm happy with the changes.

Mar 29 2018, 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.

Mar 29 2018, 3:04 AM

Mar 28 2018

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.

Mar 28 2018, 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.

Mar 28 2018, 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.

Mar 28 2018, 4:16 AM
peter.smith added inline comments to D44962: ELF: Allow thunks to change size. NFCI..
Mar 28 2018, 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?

Mar 28 2018, 2:27 AM

Mar 27 2018

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.

Mar 27 2018, 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?

Mar 27 2018, 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.

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

Mar 26 2018

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:

Mar 26 2018, 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.

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

Looks good to me.

Mar 26 2018, 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