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 (170 w, 6 d)

Recent Activity

Today

peter.smith added a comment to D65000: [ARM] Set default alignment to 64bits.

Thanks for the update. Will be worth adding some reviewers from Apple to see if this change should be IsAAPCS only. I've no more further comments myself besides a small nit on style.

Mon, Jul 22, 3:26 AM · Restricted Project
peter.smith added reviewers for D65000: [ARM] Set default alignment to 64bits: srhines, danalbert.

I think that this may not apply for Android as AFAIK their ABI still requires 128-bit alignment in some cases. Adding some more reviewers from Android.

Mon, Jul 22, 2:08 AM · Restricted Project

Fri, Jul 19

peter.smith added a comment to D64903: [ELF] Pad the last page of last PF_X PT_LOAD with traps when -z separate-code is specified.

Although I don't think that it applies to this current patch as I believe that only part of -z separate-code is implemented here. A colleague of mine mentioned that if we have a linker script similar to GNU ld's default, where the .text section is before .rodata then we need to be careful not to include the ELF headers in the executable segment. This won't be an issue for LLD's default image layout as the ro-data comes before the .text.

Fri, Jul 19, 3:56 AM · Restricted Project
peter.smith added a comment to D64906: [ELF][PPC] Allow PT_LOAD to have overlapping p_offset ranges.

I would like to do a bit more research about RELRO, as I can't see from this patch alone. I think it is fine if RELRO is double mapped into an RO page. However if RELRO is adjacent to RW segments I think it could be a bad idea to have something like

VA [0x10000, 0x10020).data.rel.roPA [0x10000, 0x10020)
VA [0x20020, ...).dataPA [0x10020, ...)

As in theory (I'm not sure about how this works in the OS/loader so I could have this wrong) if the physical contents of .data was mapped RW from 0x10000 -> 0x20000 we'd have an ability to write to the .data.rel.ro via .data.

Is there some other part of the code that prevents this or does some other mechanism in the loader/OS prevent this from happening?

Fri, Jul 19, 3:40 AM · Restricted Project
peter.smith added a comment to D64906: [ELF][PPC] Allow PT_LOAD to have overlapping p_offset ranges.

As I understand it this will only affect the non-linker script case as fixSectionAlignments() will only be called when there is no script->hasSectionsCommand. It will be worth making this clear in the description. I expect to get equivalent behaviour in the linker script we'd need to alter the implementation of DATA_SEGMENT_ALIGN in ScriptParser.cpp.

Fri, Jul 19, 3:20 AM · Restricted Project

Thu, Jul 18

peter.smith added a comment to D64903: [ELF] Pad the last page of last PF_X PT_LOAD with traps when -z separate-code is specified.

Would it also be possible to define what -zseparate-code is supposed to do in LLD?

Update description to explain -z separate-code

Thu, Jul 18, 5:24 AM · Restricted Project
peter.smith added a comment to D64903: [ELF] Pad the last page of last PF_X PT_LOAD with traps when -z separate-code is specified.

Would it be possible to update the description. It started as why we should not pad the end of the last PF_X program segment, and looks like we have added -z[no-]separate code. Would it also be possible to define what -zseparate-code is supposed to do in LLD? A mini spec I guess. For example should -zno-separate-code affect our default image layout? I'm finding it difficult to check the code without knowing what it is supposed to do.

Thu, Jul 18, 3:30 AM · Restricted Project
peter.smith added inline comments to D64880: ELF: Allow forward references to linked sections..
Thu, Jul 18, 2:58 AM · Restricted Project
peter.smith accepted D64466: AArch64: Unify relocation restrictions between MOVK/MOVN/MOVZ..

LGTM. I think that the change is worth making as no linker strictly interprets the ABI and checks the instruction encoding, and the calculation is good for all variants. I'm hoping to get the ABI wording relaxed in the next version.

Thu, Jul 18, 2:35 AM · Restricted Project

Wed, Jul 17

peter.smith added a comment to D64854: [ELF] Delete redundant pageAlign at PT_GNU_RELRO boundaries after D58892.

Currently we align p_vaddr to the next multiple of max-page-size, instead of ALIGN(CONSTANT (MAXPAGESIZE)) + (. & (CONSTANT (MAXPAGESIZE) - 1)) as ld.bfd does in its -z noseparate-code mode and some cases in its -z separate-code mode. See the comment below for a ld.lld -z max-page-size=0x200000 case:

  [11] .rodata           PROGBITS        0000000000000558 000558 000004 04  AM  0   0  4                        
  [12] .eh_frame_hdr     PROGBITS        000000000000055c 00055c 00002c 00   A  0   0  4                        
  [13] .eh_frame         PROGBITS        0000000000000588 000588 0000cc 00   A  0   0  8             
////// gap due to separated R-- and R-X   
///// This gap can be saved in --no-rosegment mode.   
  [14] .text             PROGBITS        0000000000200000 200000 000161 00  AX  0   0 16                       
  [15] .init             PROGBITS        0000000000200164 200164 000017 00  AX  0   0  4                       
  [16] .fini             PROGBITS        000000000020017c 20017c 000009 00  AX  0   0  4                       
  [17] .plt              PROGBITS        0000000000200190 200190 000020 00  AX  0   0 16 
                      
  [18] .fini_array       FINI_ARRAY      0000000000400000 400000 000008 08  WA  0   0  8                       
  [19] .init_array       INIT_ARRAY      0000000000400008 400008 000008 08  WA  0   0  8                       
  [20] .dynamic          DYNAMIC         0000000000400010 400010 0001a0 10  WA  8   0  8                       
  [21] .got              PROGBITS        00000000004001b0 4001b0 000028 00  WA  0   0  8                       
  [22] .bss.rel.ro       NOBITS          00000000004001d8 4001d8 000000 00  WA  0   0  1         
/// Gap due to PT_GNU_RELRO. It wasts almost 0x200000 bytes.
/// If we change p_vaddr of the RW PT_LOAD from 0x600000 to 0x6001d8, its p_offset doesn't need to be aligned, and we can save nearly 0x200000 bytes in the file.
  [23] .data             PROGBITS        0000000000600000 600000 000010 00  WA  0   0  8                       
  [24] .tm_clone_table   PROGBITS        0000000000600010 600010 000000 00  WA  0   0  8                       
  [25] .got.plt          PROGBITS        0000000000600010 600010 000020 00  WA  0   0  8                       
  [26] .bss              NOBITS          0000000000600030 600030 000001 00  WA  0   0  1

We perform several max-page-size alignments for this file and each costs nearly 0x200000 bytes. If we do the optimization as described in the comment, we can save a lot of disk space. This is particularly relevant to targets with a large defaultMaxPageSize (AArch64, MIPS (@atanasyan), and PPC (@sfertile): 65536).

What do you think of this trick?

Wed, Jul 17, 3:20 AM · Restricted Project

Mon, Jul 15

peter.smith added a comment to D64466: AArch64: Unify relocation restrictions between MOVK/MOVN/MOVZ..

I think that this is reasonable given that the wording in the ABI was recently changed to relax some of the wording surrounding MOVZ and MOVK. In particular Table 4-7 and Table 4-8 where modified in version E, the latest version is F https://developer.arm.com/docs/ihi0056/latest/elf-for-the-arm-64-bit-architecture-aarch64-abi-2019q2-documentation

Unfortunately it looks like Table 4-11 and Table 4-12 still have the distinction between MOVZ and MOVK. I think that this is likely an oversight rather than a deliberate decision, but I'd like to check. I'm on holiday this week, so I'll need to wait till Monday to follow this up. I've added a couple of my colleagues who may be able to help in my absence.

Mon, Jul 15, 6:53 AM · Restricted Project
peter.smith accepted D64683: MC: AArch64: Add support for prel_g* relocation specifiers..

LGTM

Mon, Jul 15, 6:48 AM · Restricted Project
peter.smith added a comment to D64685: ELF: Add support for remaining R_AARCH64_MOVW* relocations..

Functionally looks good. To my eyes it matches what is in the Arm ARM and ELF for the 64-bit architecture:

Mon, Jul 15, 4:17 AM · Restricted Project
peter.smith added a comment to D64415: Consistent types and naming for AArch64 target features (NFC).

FWIW I think this looks reasonable. The ARM equivalent uses bitfields such as

unsigned CRC : 1;
unsigned Crypto : 1;
unsigned DSP : 1;
unsigned Unaligned : 1;
unsigned DotProd : 1;

Which would make more sense than using unsigned for each individual field. Several other targets that I looked at used bool and the Has<feature> convention so I think this brings AArch64 more in line with non ARM Targets at the cost of losing a little coherence with ARM where the name Crypto remains. I don't think that this is particularly important as the two have already started to diverge with HasDotProd.

Mon, Jul 15, 2:10 AM · Restricted Project

Wed, Jul 10

peter.smith added a comment to D64456: ELF: Add support for R_AARCH64_ADR_PREL_PG_HI21_NC relocation..

I've accepted D64455, looks good to me too.

Wed, Jul 10, 1:08 AM · Restricted Project
peter.smith accepted D64455: MC: AArch64: Add support for pg_hi21_nc relocation specifier..

Looks good to me. The pg_hi21_nc matches what is used in GNU as. For completeness, at some point we may want to add :pg_hi21: which maps to R_AARCH64_ADR_PREL_PG_HI21 but given that is what we get with no modifier it doesn't seem like it is used in practice.

Wed, Jul 10, 1:07 AM · Restricted Project
peter.smith added reviewers for D64466: AArch64: Unify relocation restrictions between MOVK/MOVN/MOVZ.: ostannard, LukeCheeseman.

I think that this is reasonable given that the wording in the ABI was recently changed to relax some of the wording surrounding MOVZ and MOVK. In particular Table 4-7 and Table 4-8 where modified in version E, the latest version is F https://developer.arm.com/docs/ihi0056/latest/elf-for-the-arm-64-bit-architecture-aarch64-abi-2019q2-documentation

Wed, Jul 10, 12:40 AM · Restricted Project

Tue, Jul 9

peter.smith added a comment to D64121: Rename variables so that they start with a lowercase letter..

Agreed. I think now is as good a time as any to go for it, LGTM too.

Tue, Jul 9, 1:34 AM · Restricted Project

Thu, Jul 4

peter.smith accepted D64200: [ELF] Allow placing non-string SHF_MERGE sections with different alignments into the same MergeSyntheticSection.

Based on the closeness to D63432 and Dave's testing LGTM.

Thu, Jul 4, 6:23 AM · Restricted Project
peter.smith added a comment to D64200: [ELF] Allow placing non-string SHF_MERGE sections with different alignments into the same MergeSyntheticSection.

That would (1) waste space and (2) create unaligned sections when tail merge (-O2) is enabled. MOVAPS on such unaligned strings can raise SIGSEGV.

I haven't figured out the root cause of (2) yet.

(1) justifies a change, otherwise we will get something like a.......b.......c......., which wastes space.

Thu, Jul 4, 5:06 AM · Restricted Project
peter.smith added a comment to D64121: Rename variables so that they start with a lowercase letter..

Thanks for the updates. I've scanned through the whole patch and only 2 things jumped out at me and they are minor and subjective; I've put comments inline. It is certainly possible that I missed something, but overall I think that this is good enough and we could fix up any other mistakes later on.

Thu, Jul 4, 3:14 AM · Restricted Project
peter.smith added a comment to D63432: [ELF] Allow placing SHF_MERGE sections with different alignments into the same MergeSyntheticSection.

FWIW: The buildbot http://lab.llvm.org:8011/builders/clang-lld-x86_64-2stage/builds/8556/steps/test-stage2-check-all/logs/stdio build did eventually fail with the same errors. I am able to reproduce locally with:

  • Build clang, lld, compiler-rt with this change. Strictly speaking it might be enough to build just LLD, but by doing clang as well it makes stage 2 easier. I used
  • Install (probably not necessary, as this can probably be done out of the build directory)
  • Make new build directory
  • Do ninja check-llvm
Thu, Jul 4, 2:57 AM · Restricted Project
peter.smith added a comment to D64136: [ELF] resolveUndefined: ignore undefined symbols in SharedFile for Undefined and SharedSymbol.

Thanks for the update and the extra test. Functionally I think this looks good.

Thu, Jul 4, 1:35 AM · Restricted Project

Wed, Jul 3

peter.smith added a comment to D64130: [LLD][ELF] - Linkerscript: add a support for expressions for section's filling.

Thanks for the update. Looks ok to me, will be worth seeing what the others say.

Wed, Jul 3, 8:57 AM · Restricted Project
peter.smith added a comment to D63432: [ELF] Allow placing SHF_MERGE sections with different alignments into the same MergeSyntheticSection.

I can try -M (a.k.a. --print-map). Is there a simple failing test from the LLVM or clang test suite that would be enlightening to try it with?

Unfortunately I don't think so. Given where the test failures are (clang linked with LLD), I think that this may be a problem in the clang executable or shared-library itself causing it to miscompile the tests. If I'm right I think we'll need to find where the differences are in the before and after build then track back to the merge sections in the original objects.

Wed, Jul 3, 8:53 AM · Restricted Project
peter.smith added a comment to D63432: [ELF] Allow placing SHF_MERGE sections with different alignments into the same MergeSyntheticSection.

I'm trying to think of possible problems that this change could have caused. I think that almost by definition of a merge section, you can't rely on the address of any entry to be unique. The one thing I can think of is something where the alignment requirements are higher than the entry size. By the definition of merge sections this doesn't make a lot of sense to me and might be considered a code-gen or user error.

Wed, Jul 3, 8:41 AM · Restricted Project
peter.smith added inline comments to D64136: [ELF] resolveUndefined: ignore undefined symbols in SharedFile for Undefined and SharedSymbol.
Wed, Jul 3, 8:34 AM · Restricted Project
peter.smith added a comment to D64130: [LLD][ELF] - Linkerscript: add a support for expressions for section's filling.

It looks like gold and bfd disagree on whether a symbol is allowed in practice.

Wed, Jul 3, 7:48 AM · Restricted Project
peter.smith added a comment to D63432: [ELF] Allow placing SHF_MERGE sections with different alignments into the same MergeSyntheticSection.

This change breaks a ton of clang tests, albeit in the Swift fork of clang. Was this expected? If not, can we revert this for now?

I've confirmed that reverting this change allows top-of-tree LLVM+clang+lld to build/test top-of-tree Swift (with forks of llvm, clang, etc)

Wed, Jul 3, 6:43 AM · Restricted Project
peter.smith added a comment to D64121: Rename variables so that they start with a lowercase letter..

Currently, IS is just renamed is, but variables starting with IS (such as ISAddr) is renamed isecAddr by a special rule. Otherwise it would have looked like a predicate. I made a few special rules other than that, which you can see at https://reviews.llvm.org/D64123 (look for lowercase function).

Wed, Jul 3, 3:45 AM · Restricted Project
peter.smith added a comment to D63432: [ELF] Allow placing SHF_MERGE sections with different alignments into the same MergeSyntheticSection.

🤖: ping

Wed, Jul 3, 2:36 AM · Restricted Project
peter.smith added a comment to D64121: Rename variables so that they start with a lowercase letter..

In general I think a move to camelCase or snake_case is an improvement, don't have a particularly strong opinion over which one. I suspect that the camelCase transition is easier to automate. With just a quick scan of some of the output there are a few cases where some of the variable names are used as acronyms ISD -> InputSectionDescription. Or IS -> InputSection. I don't think that these survive the transition well, in particular IS -> is. I've not got a great idea for what to do about these right now, some possible options:

  • Do the full renaming as above and change the variable names later. Presumably it would help to be consistent.
  • Don't rename common acronyms like IS and change the variable names later.
  • Allow common acronyms to stay capitalised.
  • Have some custom renames for acronyms like IS.

I don't have a particularly strong opinion on which option. It would be helpful to be consistent though.

Wed, Jul 3, 2:28 AM · Restricted Project

Tue, Jul 2

peter.smith added a comment to D64077: [ELF] Assert sizeof(SymbolUnion) <= 80.

No objections to adding the assert, will be worth adding a bit more context as I can see someone modifying Symbol getting an assert fail and not knowing why the assert is there.

Tue, Jul 2, 7:06 AM · Restricted Project

Mon, Jul 1

peter.smith added a comment to D63974: [ELF] Only allow the binding of SharedSymbol to change for the first undef ref.

Thanks for the update, looks easier to understand to now.

Mon, Jul 1, 9:49 AM · Restricted Project
peter.smith accepted D63826: [docs][llvm-readelf] Expand llvm-readelf documentation.

Thanks for the update. I'm happy with the changes.

Mon, Jul 1, 8:06 AM · Restricted Project
peter.smith added inline comments to D63974: [ELF] Only allow the binding of SharedSymbol to change for the first undef ref.
Mon, Jul 1, 7:17 AM · Restricted Project

Thu, Jun 27

peter.smith added a comment to D63826: [docs][llvm-readelf] Expand llvm-readelf documentation.

I'll post something on the mailing list to see if anybody else has any thoughts.

Done: http://lists.llvm.org/pipermail/llvm-dev/2019-June/133384.html

Thu, Jun 27, 4:41 AM · Restricted Project

Wed, Jun 26

peter.smith added a comment to D63826: [docs][llvm-readelf] Expand llvm-readelf documentation.

Overall no objections. Only thing I can think of that could be surprising is the presence of mach-o and coff options in llvm-readelf. A case could be made for leaving these out as people migrating won't need to use them, but I don't have a strong opinion.

Wed, Jun 26, 9:45 AM · Restricted Project
peter.smith added a comment to D63810: Change the default arm-linux-gnueabihf target to armv7.

The arm1176jfz-s is what is used on the first revision of the Raspberry Pi and I think some of the later reduced size versions. This corresponds to the first mass market ARM linux distribution incorporating HF. Changing the default to ARMv7 would mean that people using clang for a Raspberry Pi using the default would end up with programs that likely would not run due to use of Arm v7 instructions. It is difficult to know how many people actually care about this as I suspect that Distros that still support the first revision of Raspberry Pi can change the default back. I think Sjoerd is right and a question on llvm-dev to canvas opinion would be a good idea.

Wed, Jun 26, 2:33 AM · Restricted Project

Tue, Jun 25

peter.smith added a comment to D63719: [docs][llvm-readobj] Improve llvm-readobj documentation.

Thanks for the update LGTM too.

Tue, Jun 25, 5:45 AM · Restricted Project

Mon, Jun 24

peter.smith added a comment to D63719: [docs][llvm-readobj] Improve llvm-readobj documentation.

Thanks for doing this, this is a big improvement. One mild suggestion, but no objections from me.

Mon, Jun 24, 9:14 AM · Restricted Project
peter.smith added a comment to D63280: [llvm-objdump] Use <first-symbol>-<offset> as the section start symbol.

To help writing up the discussion thread, I'm trying to gather some data on how each target handle the case with GNU objdump. Weird that I could not reproduce the results @MaskRay @peter.smith was able to obtain. This is not to show favor for either choice but to understand the current situation on GNU side so we make a sensible decision.

Mon, Jun 24, 1:58 AM · Restricted Project

Jun 21 2019

peter.smith added a comment to D63280: [llvm-objdump] Use <first-symbol>-<offset> as the section start symbol.

@MaskRay, @peter.smith, is it just the PLT that is specially handled? Perhaps we could just special-case that in llvm-objdump? I think there's clear precedence for this for other versions of objdump, so I'm okay with that difference in behaviour.

It seems to me like other cases of this syntax appearing are going to be very rare overall.

Jun 21 2019, 2:28 AM · Restricted Project
peter.smith added a comment to D63280: [llvm-objdump] Use <first-symbol>-<offset> as the section start symbol.

May I ask you to start a thread on https://lists.llvm.org/pipermail/llvm-dev/2019-June to discuss this?

I agree that this needs a wider audience. @ychen, could you go ahead and start that email, please. Feel free to run a draft by me first if you want.

Jun 21 2019, 1:59 AM · Restricted Project

Jun 18 2019

peter.smith added a comment to D63333: [ELF][ARM] Merge handleARMTlsRelocation() into handleTlsRelocation().

Happy with the changes, thanks for the update.

Jun 18 2019, 9:05 AM · Restricted Project
peter.smith accepted D60927: [llvm-objdump] Switch between ARM/Thumb based on mapping symbols..

Thanks for the update. Looks good to me. I've made one comment about the v7r test but not knowing what the original intention behind the test was I'm not sure if it is worth changing.

Jun 18 2019, 4:13 AM · Restricted Project
peter.smith added a comment to D63432: [ELF] Allow placing SHF_MERGE sections with different alignments into the same MergeSyntheticSection.

A small nit in the test, but otherwise looks good to me.

Jun 18 2019, 2:05 AM · Restricted Project

Jun 17 2019

peter.smith added a comment to D63383: [ELF][ARM][AARCH64][MIPS][PPC] Simplify the logic to create R_*_RELATIVE for absolute relocation types in writable sections.

From the Arm and AArch64 side this looks good to me, probably best if the Mips and PPC experts confirm their parts.

Jun 17 2019, 6:28 AM · Restricted Project

Jun 14 2019

peter.smith added a comment to D63333: [ELF][ARM] Merge handleARMTlsRelocation() into handleTlsRelocation().

I've got some stylistic suggestions; otherwise this looks to do the same thing as the previous code.

Jun 14 2019, 8:06 AM · Restricted Project
peter.smith added a comment to D63191: [lld][ELF] Check length of subsection in .ARM.attributes.

would be to drop the size field to 0xFE, 0xFF, 0xFF, 0xFF.

This would cause problems on arm-be, no?

Jun 14 2019, 6:23 AM · Restricted Project, lld
peter.smith added a comment to D63191: [lld][ELF] Check length of subsection in .ARM.attributes.

Strange, I haven't got notification. Are buildbot notifications working?
Anyway I think making Offset uint64_t instead of size_t will fix the issue.

Jun 14 2019, 6:14 AM · Restricted Project, lld
peter.smith added a comment to D63191: [lld][ELF] Check length of subsection in .ARM.attributes.
I've not worked out why yet, it could be a 32/64-bit host issue.
Jun 14 2019, 5:59 AM · Restricted Project, lld
peter.smith added a comment to D63191: [lld][ELF] Check length of subsection in .ARM.attributes.

Ironically bad-arm-attributes2.s test is failing on an Arm buildbot http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/4/steps/ninja%20check%201/logs/FAIL%3A%20lld%3A%3Abad-arm-attributes2.s

Jun 14 2019, 5:56 AM · Restricted Project, lld

Jun 13 2019

peter.smith added a comment to D63121: [ELF] Make the rule to create relative relocations in a writable section stricter.

I'll have a think to see if there are any other relocation types besides R_ARM_TARGET1 that LLD supports that can be made into a R_ARM_RELATIVE. I don't know PPC very well unfortunately. The other case I thought of that could be difficult is ILP32, where even on something like X86_64, or AArch64 the symbolic rel for a pointer would be the 32-bit relocation. I know LLD doesn't support the AArch64 ILP32 so this isn't a problem right now.

Jun 13 2019, 2:45 AM · Restricted Project
peter.smith added a comment to D63121: [ELF] Make the rule to create relative relocations in a writable section stricter.

Was difficult to spot what was wrong with the bot as there were lots of commits and a non specific error message.

Jun 13 2019, 2:27 AM · Restricted Project
peter.smith added a comment to D63244: Add --undefined-glob which is an --undefined with wildcard pattern match..

Looks good to me as well.

Jun 13 2019, 1:35 AM · Restricted Project

Jun 12 2019

peter.smith added inline comments to D63121: [ELF] Make the rule to create relative relocations in a writable section stricter.
Jun 12 2019, 10:51 AM · Restricted Project
peter.smith added a comment to D63121: [ELF] Make the rule to create relative relocations in a writable section stricter.

Reproducer:
cat init.cpp

template <class T>
struct Bar {    
    T X;
    Bar(T X) : X(X) {}
};
Jun 12 2019, 10:34 AM · Restricted Project
peter.smith added a comment to D63121: [ELF] Make the rule to create relative relocations in a writable section stricter.

A look at the differences between libclang.so with and without the patch is that what used to be a R_ARM_RELATIVE relocation is now a R_ARM_ABS32 relocation to an undefined symbol. The latter will likely resolve to 0. All of the difference is concentrated in the .init_array section which uses the R_ARM_TARGET1 relocation, which is treated as R_ARM_ABS32 by default but with an option be treated as R_ARM_REL32 (some embedded systems use offsets rather than addresses).
For example:
Good

Jun 12 2019, 9:10 AM · Restricted Project
peter.smith added a comment to D63121: [ELF] Make the rule to create relative relocations in a writable section stricter.

This commit has caused the arm lld buildbot to fail. I can reproduce it on an Arm machine and a bisect found this one as the change. If I revert this change then the specific failure goes away. Unfortunately I can't tell you exactly what is wrong yet, the failure is in the libclang unit tests which are a shared object linked by LLD. I haven't yet had a chance to isolate the segfault and work out what is causing it. I'll keep looking to see if I can work out what specifically is wrong.

Jun 12 2019, 7:14 AM · Restricted Project
peter.smith added a comment to D63190: Add -gnu-map option to emit a map file in the GNU-tsyle format..

I've been through to see if I could spot any significant differences. I've recorded what I found although I'm not sure how significant they are to the person attempting to parse the map file though.

Jun 12 2019, 6:26 AM · Restricted Project
peter.smith added a comment to D63190: Add -gnu-map option to emit a map file in the GNU-tsyle format..

Not had a chance to go through the gold source yet to see if there is anything obviously missing. From a quick experiment using a linker-script derived from ld.bfd it seems like gold's map file remains much the same, whereas ld.bfd will give information about symbol resolution etc. Will take a look at the gold source this afternoon.

Jun 12 2019, 3:46 AM · Restricted Project
peter.smith added a comment to D63190: Add -gnu-map option to emit a map file in the GNU-tsyle format..

Yes, we have an internal user who has a proprietary tool to process a -Map output. As to the difference between bfd and gold, they don't look too big, so perhaps most tools can consume both. In this output, I modeled gold.

Jun 12 2019, 3:09 AM · Restricted Project

Jun 10 2019

peter.smith committed rG386f3a27db87: [COFF][X86] Add REQUIRES: x86 to a couple of tests (authored by peter.smith).
[COFF][X86] Add REQUIRES: x86 to a couple of tests
Jun 10 2019, 3:07 AM
peter.smith created D63071: [LLD][COFF] Add REQUIRES: x86 to a couple of tests.
Jun 10 2019, 2:57 AM · Restricted Project

Jun 7 2019

peter.smith committed rGe208208a3132: [ELF][AArch64] Support for BTI and PAC (authored by peter.smith).
[ELF][AArch64] Support for BTI and PAC
Jun 7 2019, 5:58 AM
peter.smith added inline comments to D62609: [LLD][ELF][AArch64] Support for BTI and PAC.
Jun 7 2019, 5:49 AM · Restricted Project

Jun 6 2019

peter.smith updated the diff for D62609: [LLD][ELF][AArch64] Support for BTI and PAC.

Updated diff to split BtiNeeded into BtiHeader and BtiEntry, and removed Pre in favour of incrementing Buf.

Jun 6 2019, 2:56 AM · Restricted Project
peter.smith added a comment to D62609: [LLD][ELF][AArch64] Support for BTI and PAC.

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

Jun 6 2019, 2:56 AM · Restricted Project

Jun 5 2019

peter.smith committed rGe12334a0f248: [ELF] Allow reading of more than one FEATURE_1_AND in same object. (authored by peter.smith).
[ELF] Allow reading of more than one FEATURE_1_AND in same object.
Jun 5 2019, 2:31 AM

Jun 4 2019

peter.smith edited parent revisions for D62609: [LLD][ELF][AArch64] Support for BTI and PAC, added: 2; removed: 1.
Jun 4 2019, 10:14 AM · Restricted Project
peter.smith removed a child revision for D59780: Support Intel Control-flow Enforcement Technology: D62609: [LLD][ELF][AArch64] Support for BTI and PAC.
Jun 4 2019, 10:14 AM · Restricted Project
peter.smith added a child revision for D62853: Read .note.gnu.property sections and emit a merged .note.gnu.property section.: D62609: [LLD][ELF][AArch64] Support for BTI and PAC.
Jun 4 2019, 10:14 AM · Restricted Project
peter.smith added a child revision for D62862: [ELF][LLD] Allow reading of more than one FEATURE_1_AND in same object.: D62609: [LLD][ELF][AArch64] Support for BTI and PAC.
Jun 4 2019, 10:14 AM · Restricted Project
peter.smith updated the diff for D62609: [LLD][ELF][AArch64] Support for BTI and PAC.

Updated patch to rebase against parents D62853 and D62862

Jun 4 2019, 10:13 AM · Restricted Project
peter.smith added a comment to D62853: Read .note.gnu.property sections and emit a merged .note.gnu.property section..

I've created D62862 which is independent of X86/AArch64.

D62862 is x86-specific right now?

Jun 4 2019, 9:46 AM · Restricted Project
peter.smith committed rGf15e3d856fdd: [AArch64][ELF] Add support for PLT decoding with BTI instructions present (authored by peter.smith).
[AArch64][ELF] Add support for PLT decoding with BTI instructions present
Jun 4 2019, 9:33 AM
peter.smith added a parent revision for D62862: [ELF][LLD] Allow reading of more than one FEATURE_1_AND in same object.: D62853: Read .note.gnu.property sections and emit a merged .note.gnu.property section..
Jun 4 2019, 9:20 AM · Restricted Project
peter.smith added a child revision for D62853: Read .note.gnu.property sections and emit a merged .note.gnu.property section.: D62862: [ELF][LLD] Allow reading of more than one FEATURE_1_AND in same object..
Jun 4 2019, 9:20 AM · Restricted Project
peter.smith added a comment to D62853: Read .note.gnu.property sections and emit a merged .note.gnu.property section..

Thanks very much for extracting the .note.gnu.property parsing code. There was one modification I'd made in D62609 that permitted multiple GNU_PROPERTY_X86_FEATURE_1_AND properties in the same .note.gnu.property section as this was observed in the ld.bfd testsuite. I'll submit a patch with just these modifications in.

Jun 4 2019, 9:20 AM · Restricted Project
peter.smith created D62862: [ELF][LLD] Allow reading of more than one FEATURE_1_AND in same object..
Jun 4 2019, 9:20 AM · Restricted Project
peter.smith accepted D62853: Read .note.gnu.property sections and emit a merged .note.gnu.property section..

Thanks very much for extracting the .note.gnu.property parsing code. There was one modification I'd made in D62609 that permitted multiple GNU_PROPERTY_X86_FEATURE_1_AND properties in the same .note.gnu.property section as this was observed in the ld.bfd testsuite. I'll submit a patch with just these modifications in.

Jun 4 2019, 9:05 AM · Restricted Project
peter.smith added a comment to D62598: [AArch64][ELF][llvm-objdump] Add support for PLT decoding with BTI instructions present.

Thanks for the review.

Jun 4 2019, 8:14 AM · Restricted Project
peter.smith committed rG49d7221f7195: [AArch64][ELF][llvm-readobj] Add support for BTI and PAC dynamic tags (authored by peter.smith).
[AArch64][ELF][llvm-readobj] Add support for BTI and PAC dynamic tags
Jun 4 2019, 4:42 AM
peter.smith committed rG580c6d31c00d: [AARCH64][ELF][llvm-readobj] Support for AArch64 .note.gnu.property (authored by peter.smith).
[AARCH64][ELF][llvm-readobj] Support for AArch64 .note.gnu.property
Jun 4 2019, 4:27 AM
peter.smith updated the diff for D62609: [LLD][ELF][AArch64] Support for BTI and PAC.

Updated diff with single unified PLT that optionally adds in the BTI prefix and AUTIA1716 before branch. This is less code overall as we don't have 4 similar PLT writing functions at the expense of being slightly more complicated.

Jun 4 2019, 3:42 AM · Restricted Project
peter.smith added a comment to D62609: [LLD][ELF][AArch64] Support for BTI and PAC.

Peter, do you want me to separate code to read AndFeatures from https://reviews.llvm.org/D59780 and submit?

Jun 4 2019, 1:57 AM · Restricted Project

Jun 3 2019

peter.smith updated the diff for D62596: [AARCH64][ELF][llvm-readobj] Add support for BTI and PAC dynamic tags.

I've updated printDynamicEntry as suggested. There are an awful lot of MIPS tags! I've kept them with the same value as before, although I'd think that the NO tags would probably best be output as decimal. I'll leave that decision to the MIPS maintainers.

Jun 3 2019, 10:51 AM · Restricted Project
peter.smith added a comment to D62768: [ELF] Don't create an output section named `/DISCARD/` if it is assigned to the special phdr `NONE`.

Sorry I didn't follow the discussion in D61186 and didn't notice D61251. It seems @grimar and @peter.smith were fine with D61251 (this patch is essentially D61251). @andrewng if you are ok with letting this somewhat contrived example fail, we can probably still mark this as resolved (wontfix).

Jun 3 2019, 9:27 AM · Restricted Project
peter.smith updated the diff for D62598: [AArch64][ELF][llvm-objdump] Add support for PLT decoding with BTI instructions present.

Corrected matching of "BTI c" to only match "BTI c".

Jun 3 2019, 9:20 AM · Restricted Project
peter.smith added inline comments to D62598: [AArch64][ELF][llvm-objdump] Add support for PLT decoding with BTI instructions present.
Jun 3 2019, 9:18 AM · Restricted Project
peter.smith updated the diff for D62609: [LLD][ELF][AArch64] Support for BTI and PAC.

New patch to address review comments. Main changes:

  • Updated Driver.cpp to check that --pac-plt and --force-bti are aarch64 only, add test.
  • Correct number of dashes in docs
  • Fix some comments
  • Update tests to use ## for comments, use --no-show-raw-insn.
Jun 3 2019, 8:45 AM · Restricted Project
peter.smith added a comment to D62609: [LLD][ELF][AArch64] Support for BTI and PAC.

Thanks very much for the comments and questions. I'll upload a new patch in a second.

Jun 3 2019, 8:41 AM · Restricted Project
peter.smith added a comment to D62609: [LLD][ELF][AArch64] Support for BTI and PAC.

I am reading some binutils-gdb/bfd/elfnn-aarch64.c code as a reference. It doesn't use bti c for -pie (ET_DYN). <del>Is it intentional?</del>

It is intentional, because only indirect branch/call targets need BTI. A PIE PLT is not taken address so no BTI is needed.

Jun 3 2019, 6:31 AM · Restricted Project
peter.smith updated the diff for D62598: [AArch64][ELF][llvm-objdump] Add support for PLT decoding with BTI instructions present.

Incorporated George's test case suggestions (Thank You!)

Jun 3 2019, 5:52 AM · Restricted Project
peter.smith added inline comments to D62598: [AArch64][ELF][llvm-objdump] Add support for PLT decoding with BTI instructions present.
Jun 3 2019, 4:15 AM · Restricted Project

May 31 2019

peter.smith added a comment to D62555: [TailDuplicator] prevent tail duplication for INLINEASM_BR.

so probably a bug in if conversion. Maybe the same issue as https://bugs.llvm.org/show_bug.cgi?id=41121 .

Interesting, and does look related. While I try to make If Converter more robust in the face of INLINEASM_BR, I still think curtailing Tail Duplicator from duplicating INLINEASM_BR (and INLINEASM) is probably also worthwhile, and kind of makes robustness changes to If Converter less important. I'm going to spend the rest of the day trying to improve If Converter, but ultimately this is holding up asm goto from working perfectly (which is more important to me), so if I don't have anything working by EOD, then tomorrow I will make the suggested edits here and focus on this patch.

May 31 2019, 5:30 AM · Restricted Project
peter.smith added a comment to D62596: [AARCH64][ELF][llvm-readobj] Add support for BTI and PAC dynamic tags.

Thanks for the suggestion, I'll update the patch on Monday.

May 31 2019, 3:50 AM · Restricted Project
peter.smith added a comment to D62598: [AArch64][ELF][llvm-objdump] Add support for PLT decoding with BTI instructions present.

Thanks for the suggestion, I'll update on Monday.

May 31 2019, 3:46 AM · Restricted Project

May 30 2019

peter.smith updated the diff for D62609: [LLD][ELF][AArch64] Support for BTI and PAC.

Updated tests to align instructions and use readelf -x .got.plt.

May 30 2019, 10:00 AM · Restricted Project