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 (319 w, 4 d)

Recent Activity

Fri, May 13

peter.smith accepted D125523: [ELF] Disallow input section description without a filename.

LGTM. GNU ld documentation for InputSection https://sourceware.org/binutils/docs/ld/Input-Section-Basics.html states that An input section description consists of a file name optionally followed by a list of section names in parentheses. I couldn't find anything about which characters in filenames were not permitted. It looks like both lld and gnu ld can handle these when quoted. For example for a file "file(o)" we can write { "file(o)"(.text) } I do hope parentheses in filenames are not common though. Maybe worth a test case if we've not got one already.

Fri, May 13, 1:52 AM · Restricted Project, Restricted Project

Thu, May 12

peter.smith accepted D125410: [ELF] Align the end of PT_GNU_RELRO to max-page-size instead of common-page-size.

LGTM thanks for making the change. It was useful to see the write up in the binutils PR as common page versus max page size isn't documented very well.

Thu, May 12, 12:43 AM · Restricted Project, Restricted Project

Fri, May 6

peter.smith accepted D125074: [ELF] Change (NOLOAD) type mismatch to use SHT_NOBITS instead of SHT_PROGBITS.

From doing some reading about NOLOAD, there is a use case for SHT_PROGBITS in NOLOAD. In an embedded system it can represent something like a ROM that is always present on the device at that address. This gets modelled as SHT_NOBITS so that addresses are calculated but not written to the file. Whomever writes the ZI initialization code will obviously have to make sure not to try and write that ZI.

Fri, May 6, 3:52 AM · Restricted Project, Restricted Project

Wed, May 4

peter.smith accepted D124656: [ELF] Support custom sections between DATA_SEGMENT_ALIGN and DATA_SEGMENT_RELRO_END.

LGTM, thanks for the update.

Wed, May 4, 12:50 AM · Restricted Project, Restricted Project

Tue, May 3

peter.smith accepted D124653: [ELF] Fix branch range computation when picking ThunkSection.

Looks good to me.

Tue, May 3, 5:42 AM · Restricted Project, Restricted Project
peter.smith added a comment to D124656: [ELF] Support custom sections between DATA_SEGMENT_ALIGN and DATA_SEGMENT_RELRO_END.

Looks sensible to me. One quick question, how does orphan placement relate to RELRO. For example if we make RELRO defined as [first relro section start, last relro section end) between DATA_SECTION_ALIGN and DATA_SECTION_RELRO_END, is there any danger of orphan placement putting a non-relro section inside that range? That case isn't safe for RW as it could be written to at runtime.

Tue, May 3, 4:01 AM · Restricted Project, Restricted Project

Thu, Apr 28

peter.smith added a comment to D124214: [libc][NOT FOR COMMIT] building LLVM-libc on 32 bit arm with gcc.

FWIW I was able to get at least the ninja llvmlibc working with the LLVM embedded toolchain with the following hacky bit of cmake

cmake ../../llvm -G Ninja -DLLVM_ENABLE_PROJECTS="libc" \
      -DCMAKE_TRY_COMPILE_TARGET_TYPE=STATIC_LIBRARY \
      -DCMAKE_BUILD_TYPE=Release \
      -DCMAKE_C_COMPILER=clang \
      -DCMAKE_CXX_COMPILER=clang++ \
      -DLLVM_LIBC_FULL_BUILD=OFF \
      -DCMAKE_C_COMPILER_TARGET=armv7-m-none-eabi \
      -DCMAKE_CXX_COMPILER_TARGET=armv7-m-none-eabi \
      -DCMAKE_C_FLAGS="--config armv7m_soft_nofp_rdimon_baremetal"\
      -DCMAKE_CXX_FLAGS="--config armv7m_soft_nofp_rdimon_baremetal"\
      -DCMAKE_AR=llvm-ar \
      -DLLVM_LIBC_ENABLE_LINTING=OFF -DLLVM_LIBC_INCLUDE_SCUDO=OFF \
      -DLLVM_DEFAULT_TARGET_TRIPLE="armv7-m-none-eabi" \
      -DCMAKE_SYSTEM_NAME=Linux -DCMAKE_SYSTEM_PROCESSOR=arm \
      -DLLVM_INCLUDE_BENCHMARKS=OFF
Thu, Apr 28, 9:05 AM · Restricted Project, Restricted Project, Restricted Project
peter.smith added a comment to D124214: [libc][NOT FOR COMMIT] building LLVM-libc on 32 bit arm with gcc.

Thanks very much for posting the patch. Just curious to know whether you've tried building with Clang? Is using GCC due to it having a default target of Arm? If I get some time I'll try to see if I can get it work with the https://github.com/ARM-software/LLVM-embedded-toolchain-for-Arm this approximates some of the multilib and specs files with clang config files, although we're hoping to add better support for multilib.

Thu, Apr 28, 3:25 AM · Restricted Project, Restricted Project, Restricted Project

Apr 21 2022

peter.smith accepted D124041: [ELF] Move SymbolUnion size assertion to source file.

This looks sensible to me. If the size of SymbolUnion is changed then Symbols.cpp will be recompiled so we'll always get the assertion if it is only changed.

Apr 21 2022, 3:10 AM · Restricted Project, Restricted Project

Apr 14 2022

peter.smith accepted D123750: [ELF][AArch64] Fix unneeded thunk for branches to hidden undefined weak.

LGTM. One small suggestion for the comment.

Apr 14 2022, 5:50 AM · Restricted Project, Restricted Project

Apr 8 2022

peter.smith accepted D123283: [MC] Improve st_size propagation rule.

Thanks for the update. Assuming zatrazz has no objections LGTM.

Apr 8 2022, 5:27 AM · Restricted Project, Restricted Project

Apr 7 2022

peter.smith added a comment to D123283: [MC] Improve st_size propagation rule.

I think this is an improvement. No objections from me. I had a quick thought about how we might be able to support the chained assignment case although it is only based on looking at this review so it could be flawed. Ideally we'd not wait to propagate the size in writeSymbol. I'm guessing that it is possible to write something like

.size x, 2
y = x
z = y
.size y, 1 // should also set size of z to 1.

So it is not as simple as propagating the size on assignment. Perhaps it would be some kind of size propagation step for all symbols prior to writing them.

Apr 7 2022, 3:50 AM · Restricted Project, Restricted Project

Apr 5 2022

peter.smith added a comment to D123105: [compiler-rt][builtins] Move DMB definition to syn-ops.h.

I'm not sure that this is right. From the Armv5/v6 ARM ARM https://developer.arm.com/documentation/ddi0100/i/ I think DMB was introduced in Armv6 (see B2.6.1 DataMemoryBarrier (DMB) CP15 register 7) . Armv5 did have an operation called Drain Write Buffer DWB that was renamed in Armv6 to DSB (see DataSynchronizationBarrier (DSB) CP15 register 7). From B6.6.5 Register 7: cache management functions this is opcode 4 but I don't think DWB is exactly the same as DSB is on Armv6.

Apr 5 2022, 1:32 AM · Restricted Project, Restricted Project

Mar 29 2022

peter.smith added a comment to D122450: [ELF] Default to --no-fortran-common.

Apologies for delay in responding, been OoO for a bit. I'm happy with the change. Do we have enough people that commented in favour of D86142 in the reviewer/subscriber list? If only to make sure they are aware of the change.

Mar 29 2022, 8:22 AM · Restricted Project, Restricted Project

Mar 15 2022

peter.smith added a comment to D119384: [ELF][MTE] Add --android-memtag-* options to synthesize ELF notes.

Some initial thoughts.

  • If this is likely to be rolled out to other platforms such as Linux/BSD etc. It would be useful to have something common written up in the ABI. As MaskRay suggests the best way forward would be to raise an issue or a pull request https://github.com/ARM-software/abi-aa . You are of course welcome to implement an Android specific platform ABI, but it would be good to a similar one across multiple platforms if possible.
  • For BTI, for better or worse, we chose to https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst#62program-property as this mechanism was going to be needed for Intel platforms. Ideally this would be documented in the SysVABI doc, but that didn't exist at the time the property was created.
  • Do you need per-object marking that a linker will aggregate? For example do you enable when at least one object needs MTE, or do you need all objects to be compatible with MTE? A per object marking that can be overridden at link time will give you a chance to diagnose objects that may not have the MTE support. It can help to have an assembler option to fabricate the note if objects are used.
  • A colleague mentioned that the choice of command line options were quite generic. If they are aarch64 and Android specific they probably ought to be behind -z and perhaps with AArch64 and Android in the name.
Mar 15 2022, 3:53 AM · Restricted Project, Restricted Project

Mar 14 2022

peter.smith accepted D120626: [ELF] Move section assignment from initializeSymbols to postParse.

Thanks for the updates. I've set approved as my comments have been addressed.

Mar 14 2022, 11:16 AM · Restricted Project, Restricted Project

Mar 9 2022

peter.smith added a comment to D120626: [ELF] Move section assignment from initializeSymbols to postParse.

Lowest cost option I can think of is to additionally set a flag when InputFiles.cpp:1163:

if (sym.file == this)
  sym.replace(Undefined{this, sym.getName(), sym.binding, sym.stOther,
                        sym.type, secIdx});
continue;

Perhaps a new bit field entry in undefined. At the time of reporting undefined symbols we could check for that flag and report some additional context.
`Symbol <sym> was defined in non-prevailing group. Either the symbol was not present in the prevailing group, or the symbol in the prevailing group had STB_WEAK binding and this symbol had STB_GLOBAL binding. LLD does not support mixing of groups with STB_WEAK and STB_GLOBAL bindings for the same symbol name.'

Mar 9 2022, 2:19 AM · Restricted Project, Restricted Project

Mar 2 2022

peter.smith added a comment to D120626: [ELF] Move section assignment from initializeSymbols to postParse.

Not supporting comdat-binding: %t/w.o %t/g.o leads to an undesired undefined symbol. This is not ideal but acceptable because valid input cannot form this case. makes me a bit nervous. In a clang/gcc world this is not a problem, but there may be third party compilers that differ. For example arm's legacy compiler armcc from Arm Compiler 5 uses STB_GLOBAL for definitions in COMDAT groups, whereas GCC, at least used to, uses STB_WEAK. It is possible that this is illegal, but I can't find in the spec where it says that.

Perhaps it could be accounted for in resolve as I think we'd be passing a global with the discarded section. Perhaps we could prefer the existing symbol in that case?

Avoiding accessing sections[*] is what the patch strives to do: untangle section initialization and symbol initialization/resolution as much as possible.
I have a plan to merge section initialization and initializeLocalSymbols into initSectionsAndLocalSyms, which parallelizes section initialization.
In the long term, this is a necessary step to achieve parallel input file parsing (https://discourse.llvm.org/t/parallel-input-file-parsing/60164) (and perhaps important to keep lld relevant with the emerging of mold.)

Keeping if (sec == &InputSection::discarded) will address the %t/w.o %t/g.o dilemma, but that probably will not fly.
Personally I'd like to say such a dilemma is unsupported. If we have to, it doesn't seem like there is a good way supporting it...

Mar 2 2022, 2:30 AM · Restricted Project, Restricted Project
peter.smith accepted D120701: [ELF] Add -z pack-relative-relocs.

I agree with emaste's comment on the release notes. I'm guessing that can be fixed up prior to committing. LGTM otherwise.

Mar 2 2022, 1:59 AM · Restricted Project, Restricted Project

Mar 1 2022

peter.smith added a comment to D120626: [ELF] Move section assignment from initializeSymbols to postParse.

Not supporting comdat-binding: %t/w.o %t/g.o leads to an undesired undefined symbol. This is not ideal but acceptable because valid input cannot form this case. makes me a bit nervous. In a clang/gcc world this is not a problem, but there may be third party compilers that differ. For example arm's legacy compiler armcc from Arm Compiler 5 uses STB_GLOBAL for definitions in COMDAT groups, whereas GCC, at least used to, uses STB_WEAK. It is possible that this is illegal, but I can't find in the spec where it says that.

Mar 1 2022, 11:17 AM · Restricted Project, Restricted Project
peter.smith added a comment to D120650: [ELF] Don't use multiple inheritance for OutputSection. NFC.

Just one small comment on some function names, although that is not a strong opinion. No objections to making the changes, will wait a few days before setting approved to see if there are dissenting opinions.

Mar 1 2022, 9:23 AM · Restricted Project, Restricted Project
peter.smith added a comment to D120701: [ELF] Add -z pack-relative-relocs.

Code changes LGTM. Will be worth release noting and updating the ld.lld.1 man page as I think we've got an entry for pack-dyn-relocs.

Mar 1 2022, 9:15 AM · Restricted Project, Restricted Project

Feb 28 2022

peter.smith added a comment to D120640: [ELF] Set the priority of STB_GNU_UNIQUE the same as STB_WEAK.

Thanks I'll try and look at D120626 tomorrow.

Feb 28 2022, 10:38 AM · Restricted Project, Restricted Project
peter.smith added a comment to D120650: [ELF] Don't use multiple inheritance for OutputSection. NFC.

No objections for the change. I think it makes sense to logically separate OutputSection from SectionCommand. I'd like to see if anyone else has a strong opinion as there are quite a few changes and some more dynamic memory allocations. I'm wondering if there may be a way of avoiding those somehow, for example could an implicit conversion operator to OutputSection* be added?

Feb 28 2022, 10:36 AM · Restricted Project, Restricted Project
peter.smith added inline comments to D120640: [ELF] Set the priority of STB_GNU_UNIQUE the same as STB_WEAK.
Feb 28 2022, 10:07 AM · Restricted Project, Restricted Project

Feb 23 2022

peter.smith accepted D120389: [ELF] Check COMMON symbols for PROVIDE and don't redefine COMMON symbols edata/end/etext.

LGTM, makes sense to me.

Feb 23 2022, 6:29 AM · Restricted Project

Feb 17 2022

peter.smith accepted D118840: [ELF] Support (TYPE=<value>) to customize the output section type.

LGTM. At least if we start off without READONLY then we can put it in later if it is needed.

Feb 17 2022, 2:16 AM · Restricted Project, Restricted Project

Feb 15 2022

peter.smith accepted D119074: [ELF] Parse archives as --start-lib object files.

Thanks for the update. LGTM.

Feb 15 2022, 12:28 AM · Restricted Project

Feb 14 2022

peter.smith added a comment to D118840: [ELF] Support (TYPE=<value>) to customize the output section type.

One more section type SHT_GROUP that I think we should disallow, although if GNU ld would prefer to include that we should prefer compatibility.

Feb 14 2022, 5:18 AM · Restricted Project, Restricted Project
peter.smith added a comment to D119074: [ELF] Parse archives as --start-lib object files.

I've had a chance to look through this. Assuming the user-visible behaviour changes are limited to what has been mentioned in the description I'm in favour of the change. I've only made one suggestion surrounding a comment. To check my understanding all files in archives are handled with createLazyFile which would make their symbols LazyObject ? If I'm wrong then I'll need to re-read the code to make sure I understand first.

Feb 14 2022, 3:34 AM · Restricted Project

Feb 11 2022

peter.smith added a comment to D119074: [ELF] Parse archives as --start-lib object files.

Apologies, I ran out of time this Friday, will try again early next week.

Feb 11 2022, 9:28 AM · Restricted Project
peter.smith added a comment to D119108: [ELF] Remove obscure -dp and GNU ld incompatible --[no-]define-common, ignore -d/-dc.

FYI: Deprecation policy suggestion https://discourse.llvm.org/t/rfc-lld-deprecation-policy/60067

Feb 11 2022, 9:16 AM · Restricted Project
peter.smith added a comment to D119435: [Support][AArch64] Detect a few more host CPU features on AArch64.

You'll have to forgive me, I'm coming at this without a lot of context other than what is supported on what Arm architecture, and I didn't have time to reverse engineer it from the code. Thanks for the link, that makes it look like this is related to something like parsing proc/cpuinfo and nowhere else.

Feb 11 2022, 3:49 AM · Restricted Project, Restricted Project
peter.smith added a comment to D119435: [Support][AArch64] Detect a few more host CPU features on AArch64.

I think we need to be careful here. The extensions in that StringSwitch are for Armv8-a and hence applicable to all AArch64 CPUs. Things like sve are only available on later processors. We'll need to check to see what the intent was here before adding things. We'll also need to work out how to add tests for these, a good idea would be to see how the existing ones are tested.

Feb 11 2022, 1:58 AM · Restricted Project, Restricted Project

Feb 9 2022

peter.smith accepted D119108: [ELF] Remove obscure -dp and GNU ld incompatible --[no-]define-common, ignore -d/-dc.

LGTM for the changes. It will be worth giving the messages some time to see if there are any others affected.

Feb 9 2022, 1:21 AM · Restricted Project

Feb 8 2022

peter.smith added a comment to D118840: [ELF] Support (TYPE=<value>) to customize the output section type.

Thanks for the update. The intent is clearer now. I've left some small suggestions.

Feb 8 2022, 5:41 AM · Restricted Project, Restricted Project
peter.smith added a comment to D119074: [ELF] Parse archives as --start-lib object files.

Just come back from being out of office, will try and work my way through this but may take a bit of time. Hopefully by the end of the week.

Feb 8 2022, 1:48 AM · Restricted Project
peter.smith added a comment to D119108: [ELF] Remove obscure -dp and GNU ld incompatible --[no-]define-common, ignore -d/-dc.

While in this case I think you are right, and no-one using LLD will be depending on this. I'm a bit uncomfortable with taking options out without a bit more warning and chance for feedback. I think it could be worth using this to set out LLD's deprecation/removal process. For example for options that maintainers want to remove we've got several options, or even a combination depending on circumstances:

  • Option will be deprecated (with a silenceable warning) for a release, then removed in the subsequent release.
  • Option will be removed if there is no use of it in an open source project.
  • Option will be removed if a post on the community discourse/mailing lists, github issue has no one feedback with a good reason to keep it within a certain time frame.
  • Option will be set to silent ignore if a valid, if sub-optimal output can be produced without applying the option.
Feb 8 2022, 1:43 AM · Restricted Project
peter.smith accepted D119137: Replace Steve Klabnik with Josh Stone as one of the Rust Security Response WG representatives.
Feb 8 2022, 1:11 AM · Restricted Project, Restricted Project

Feb 3 2022

peter.smith added a comment to D118840: [ELF] Support (TYPE=<value>) to customize the output section type.

Not had a chance to look through the code, this is based on the description and the test so my apologies if the answers are in the code.

Feb 3 2022, 2:16 AM · Restricted Project, Restricted Project

Feb 2 2022

peter.smith added a comment to D118756: [ELF] Avoid wrapping unreferenced lazy symbols.

Implementation looks good to me. It may be worth writing a test involving LTO, for example a definition of lazy in a bitcode archive. As I understand the implementation this should work, but it may be worth having a regression test to catch it if it breaks.

Feb 2 2022, 7:00 AM · Restricted Project

Feb 1 2022

peter.smith added a comment to D118555: [ELF][WIP] Create --segregate-sections for outputting sections in their own segment .

I've only got a few drive by comments at the moment. I can see where MaskRay is coming from with being a bit nervous about this. This sounds like it could work well for profile counts but I'm not convinced its well specified enough to be applied generically, at least in the prospect of linker scripts. I may be misunderstanding the code, but IIUC if I wrote --segregate-sections=".text;.text.2" would it make sense to move them after the .bss sections? It would also interct badly with linker script commands like PHDRS.

Feb 1 2022, 2:24 AM
peter.smith accepted D118529: [ELF] Rename adjustSectionsBeforeSorting to adjustOutputSections and make it affect INSERT commands.

Thanks for the update. The adjustOutputSections still makes me a bit nervous as the name implies "do stuff" without any real idea of what just from the name. One possibility would be to split it into smaller functions, for example: setAlignment, removeFlagsFromEmptySection removeDiscardedOutputSections. Anyhow I don't want to let that get in the way of the actual fix.

Feb 1 2022, 1:58 AM · Restricted Project
peter.smith accepted D118577: [ELF] Deduplicate names of local symbols only with -O2.

LGTM thanks for the release note.

Feb 1 2022, 1:42 AM · Restricted Project

Jan 31 2022

peter.smith added a comment to D118529: [ELF] Rename adjustSectionsBeforeSorting to adjustOutputSections and make it affect INSERT commands.

Would it be possible to go a bit further and move all the functionality from adjustSections that isn't important for placing Orphan Sections to adjustSectionsAfterSorting. We'd then be able to give adjustSections a more specific name. For example adjustOutputSectionFlags. As it stands adjustSections could mean almost anything.

Jan 31 2022, 7:58 AM · Restricted Project
peter.smith added a comment to D118577: [ELF] Deduplicate names of local symbols only with -O2.

I'm OK with the change. It looks like a good trade off for non-release links. I think it will be worth an entry in the release notes. Something like: "LLD no longer deduplicates local symbol names at the default optimization level of -O1. This results in a larger output ELF file but a faster link time. Use optimization level -O2 to restore the deduplication."

Jan 31 2022, 7:17 AM · Restricted Project
peter.smith accepted D118530: [ELF] Update flag propagation rule to ignore discarded output sections.

LGTM too.

Jan 31 2022, 5:51 AM · Restricted Project

Jan 24 2022

peter.smith accepted D117734: [ELF] Fix the branch range computation when reusing a thunk.

LGTM thanks for the update.

Jan 24 2022, 1:44 AM · Restricted Project

Jan 21 2022

peter.smith added a comment to D54759: [LLD][ELF] Use more specific method to calculate DT_PLTRELSZ.

Created D117896 for the REQUIRES: update

Jan 21 2022, 8:31 AM
peter.smith requested review of D117896: [LLD][ELF][AArch64] Update test with incorrect REQUIRES line [NFC].
Jan 21 2022, 8:30 AM · Restricted Project
peter.smith added a comment to D117734: [ELF] Fix the branch range computation when reusing a thunk.

Here's a modification of your test case for AArch64 that will fail for Arm

# REQUIRES: arm
# RUN: rm -rf %t && split-file %s %t
# RUN: llvm-mc -filetype=obj -triple=armv7-a-none-eabi --arm-add-build-attributes %t/a.s -o %t/a.o
# RUN: ld.lld -pie -T %t/lds %t/a.o -o %t/a
# RUN: llvm-objdump -d --no-show-raw-insn %t/a | FileCheck %s
Jan 21 2022, 7:39 AM · Restricted Project
peter.smith added a comment to D117734: [ELF] Fix the branch range computation when reusing a thunk.

I'd like to use t->getThunkTargetSym()->getVA(getPCBias(rel.type)) but two arm-* tests would fail. I think that is the remaining getPCBias issue after D97550. Not fixing it is probably fine since the failing scenario seems extremely corner-case.

Jan 21 2022, 6:45 AM · Restricted Project
peter.smith accepted D117872: Add security group 2021 transparency report..

LGTM thanks for working on this.

Jan 21 2022, 6:31 AM · Restricted Project
peter.smith added a comment to D117853: [ELF] Parallelize --compress-debug-sections=zlib.

No objections from me. I think the speed up is worth the small amount of extra size. I've made a few small suggestions but are all subjective. I don't have a lot of large programs hanging around to test this on. I guess something like Chromium would give you another data point.

Jan 21 2022, 6:19 AM · Restricted Project

Jan 20 2022

peter.smith added a comment to D117734: [ELF] Fix the branch range computation when reusing a thunk.

Will need to have a think about this. I should have some time on Friday afternoon. I'd like to try and write a similar Arm case as we'll need to take into account the PC-bias stored in Addend in that case. It may be that we need a special case for Arm here.

Jan 20 2022, 2:11 AM · Restricted Project

Jan 19 2022

peter.smith added a comment to D117284: [ELF] Allow non-bitcode archive with an empty index.

No objections. Reading your blog post and my copy of Linkers and Loaders it does seem that early linkers would handle archives with indexes, it just seems like indexes were ubiquitous when GNU ld were written. So it does seem like rejecting archives without an index is a property of the implentation of GNU ld rather than something that should be relied upon.

Jan 19 2022, 7:14 AM · Restricted Project
peter.smith added a comment to D54759: [LLD][ELF] Use more specific method to calculate DT_PLTRELSZ.

Hi @peter.smith! The tests arm-combined-dynrel.s and arm-combined-dynrel-ifunc.s do not run because of inaccurate REQUIRES. Could you fix them, please?

Jan 19 2022, 3:34 AM

Jan 18 2022

peter.smith added a comment to D117233: [llvm-objcopy] Preserve ARM and AArch64 mapping symbols.

FWIW I'm happy with the proposed change.

Jan 18 2022, 2:13 AM · Restricted Project

Jan 17 2022

peter.smith added a comment to D117439: [ELF] Update .relr.dyn once in the absence of SECTIONS commands.

Will have to have a think to see if I can spot any problems.

Jan 17 2022, 1:55 AM · Restricted Project

Jan 14 2022

peter.smith added a comment to D117284: [ELF] Allow non-bitcode archive with an empty index.

I can see that something like this could be valuable for a program using archives for major subsystems, although less well for a set of loosely connected utility routines (C-library). Doing this transparently makes me a little bit nervous for the reasons you suggest in the description.

Jan 14 2022, 1:51 AM · Restricted Project

Jan 13 2022

peter.smith added a comment to D117233: [llvm-objcopy] Preserve ARM and AArch64 mapping symbols.

From https://sourceware.org/binutils/docs/binutils/objcopy.html

-x
--discard-all
Do not copy non-global symbols from the source file.
Jan 13 2022, 10:37 AM · Restricted Project

Jan 11 2022

peter.smith accepted D116838: [ELF] -Map --why-extract=: print despite errors.

Thanks for the update LGTM on my side. It could be worth adding a comment after finalizeAddressDependentContent() just before the error check, to say something like All information needed for OutputSection part of Map file available. That might help someone that adds an extra error check and sees a test fail.

Jan 11 2022, 9:04 AM · Restricted Project
peter.smith added a comment to D116881: [ELF] Add RelocationScanner. NFC.

LGTM too, thanks for the updates.

Jan 11 2022, 8:33 AM · Restricted Project

Jan 10 2022

peter.smith added a comment to D116881: [ELF] Add RelocationScanner. NFC.

In principle I'm in favour of having a class with a single public entry point. I agree with ikudrin's comments.

Jan 10 2022, 5:53 AM · Restricted Project
peter.smith added a comment to D116838: [ELF] -Map --why-extract=: print despite errors.

Presumably the relocation error is detected early, and not one detected late during writeTo()? I suspect that a late error would lead to a more complete map file being produced. In Arm's proprietary linker the ability to write a map file when a relocation out of range error has been detected has been really useful to debug problems in the thunk creation logic. I think it should be possible to detect if the OutputSections are incomplete, if that is the case it may be worth suppressing the OutputSections in the map file, or maybe putting a warning in the map file to state that output is incomplete.

Jan 10 2022, 5:28 AM · Restricted Project

Jan 5 2022

peter.smith accepted D116281: [ELF] Move gotIndex/pltIndex/globalDynIndex to SymbolAux.

Thanks for the updates and the data. I don't have any more comments. I've marked as approved from my side, but please wait a bit to see if the other reviewers have any comments or concerns.

Jan 5 2022, 1:12 AM · Restricted Project

Jan 4 2022

peter.smith added a comment to D116281: [ELF] Move gotIndex/pltIndex/globalDynIndex to SymbolAux.

I've made some suggestions in inline comments. This type of change does tend to make the implementation slightly harder to read and more fragile, maybe worth getting some data on how much effect this has on linking performance? If it is negligble and there are no plans to follow it up with more changes that depend on it, is it worth doing?

Jan 4 2022, 9:59 AM · Restricted Project

Dec 20 2021

peter.smith added a comment to D115993: [ELF] Optimize RelocationSection<ELFT>::writeTo.

No objections from me. It may be worth getting some figures from the lld speed test (https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171030/498843.html) as well, although I expect they won't show regressions.

Dec 20 2021, 5:56 AM · Restricted Project

Dec 15 2021

peter.smith accepted D115657: [Nomination] Adding Intel representatives to security group.

LGTM

Dec 15 2021, 1:27 AM · Restricted Project

Dec 13 2021

peter.smith accepted D115603: [ELF] Remove needsPltAddr in favor of needsCopy.

LGTM, although worth waiting a bit to see what others think.

Dec 13 2021, 3:54 AM · Restricted Project

Dec 6 2021

peter.smith added a comment to D114124: [clang][ARM] only check -mtp=cp15 for non-asm sources.

LGTM too.

Dec 6 2021, 1:55 AM · Restricted Project

Dec 3 2021

peter.smith accepted D114783: [ELF] Split scanRelocations into scanRelocations/postScanRelocations.

LGTM, I think this is a good simplification. The test changes all look related to GOT ordering, although I've concentrated my checking on AArch64. Maybe worth widening the reviewer pool to see if there are any alternative opinions.

Dec 3 2021, 3:52 AM · Restricted Project
peter.smith added a comment to D114830: [ELF] Hint -z nostart-stop-gc for __start_ undefined references.

I'm happy to approve in post; my apologies for not getting to it in time. Regardless of the opinion on the default value of start-stop-gc this particular change adds a useful warning and documentation.

Dec 3 2021, 2:07 AM · Restricted Project
peter.smith accepted D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases.

LGTM, thanks for the update.

Dec 3 2021, 1:40 AM · Restricted Project

Dec 2 2021

peter.smith added a comment to D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases.

Apologies, missed a couple of small things out. Otherwise looks good to me.

Dec 2 2021, 10:42 AM · Restricted Project
peter.smith added a comment to D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases.

I've made a suggestion to disallow v8-m.baseline (does not have Thumb 2 but has number > 7) and to simplify the expression.

Dec 2 2021, 10:28 AM · Restricted Project
peter.smith added a comment to D114910: [ELF] Discard input .note.gnu.build-id even with default --build-id=none.

LGTM too.

Dec 2 2021, 1:13 AM · Restricted Project

Dec 1 2021

peter.smith added a comment to D114783: [ELF] Split scanRelocations into scanRelocations/postScanRelocations.

Thanks for the update. Will try and take a look later this week.

Dec 1 2021, 11:06 AM · Restricted Project

Nov 30 2021

peter.smith added a comment to D114801: [ELF] Prevent internalizing used comdat symbol.

Looks good to me, just some suggestions on the comments.

Nov 30 2021, 11:12 AM · Restricted Project, lld
peter.smith added a comment to D114783: [ELF] Split scanRelocations into scanRelocations/postScanRelocations.

I'm personally in favour of postponing some of the calculations from scan relocations as I think it can give us more flexibility. One possible concern is performance, although I'd expect that to be relatively small. I've left some subjective comments, I understand that this is WIP so no need to follow them.

Nov 30 2021, 10:15 AM · Restricted Project
peter.smith accepted D114748: [ELF] Change -z unknown from error to warning.

LGTM too, good to match GNU ld behaviour where possible. One stale comment spotted but easy to fix up prior to commit.

Nov 30 2021, 2:49 AM · Restricted Project

Nov 25 2021

peter.smith added a comment to D113901: [lld] Add cet-report and bti-report flags.

One small comment on the error message for the command line checking, but otherwise looks good to me.

Nov 25 2021, 8:56 AM · Restricted Project

Nov 22 2021

peter.smith accepted D113771: [ELF] Support the "read-only" memory region attribute.

Thanks for the update and sorry for the delay in responding. LGTM. Will be worth waiting a few days before committing to see if @MaskRay has any comments.

Nov 22 2021, 9:50 AM · Restricted Project, lld
peter.smith accepted D114172: [ARM] implement support for ALU/LDR PC-relative group relocations.

LGTM, thanks for working on this. I've marked approved from an Arm architecture perspective. Would also be good to wait a few days to see if @MaskRay is happy from a code-owner perspective.

Nov 22 2021, 9:39 AM · Restricted Project

Nov 19 2021

peter.smith added inline comments to D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases.
Nov 19 2021, 8:29 AM · Restricted Project

Nov 18 2021

peter.smith added a comment to D114172: [ARM] implement support for ALU/LDR PC-relative group relocations.

At a glance this looks good to me. I've not gone through all the fine details of the calculation and the tests yet. I'll be out of office tomorrow so likely to be next week. Happy for someone else to approve in my absence. I agree with you that no-one should be relying on the wrap around case for an address calculation. The original code was taken from the ARM backend to make it easier to review as it had been used elsewhere in the code-base for many years.

Nov 18 2021, 10:31 AM · Restricted Project
peter.smith added a comment to D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases.

Apologies had to go on a bit of a dive through the documentation. I've put some comments inline and some links to other documents that may help.

Nov 18 2021, 1:26 AM · Restricted Project

Nov 17 2021

peter.smith added a comment to D113901: [lld] Add cet-report and bti-report flags.

BTW, how do we handle object files created by objcopy -I binary -O elfXXX a.txt a.o or ld -b binary a.txt -o a.o? Such object files don't have .note.gnu.property

Nov 17 2021, 2:03 AM · Restricted Project
peter.smith added a comment to D113901: [lld] Add cet-report and bti-report flags.

-z force-bti with -Werror has the same behaviour but not always possible to use -Werror and dangerous to rely on it.

GCC/Clang support -Werror. ld.bfd/gold/ld.lld support --fatal-warnings.

I'm in favour of adding the additional flag. It is useful when an effort has been made to get all input files marked, and we want to make sure that the program stays that way. I think that complements the existing force-bti flag which is useful when transitioning a program into compatibility.

One thought is that this could also be useful for ibt. Is there scope for -z fail-if-not=bti and -z fail-if-not=ibt which might make this more extensible for future property flags?

Does this have the same semantics as -z cet-report=error? (https://bugs.llvm.org/show_bug.cgi?id=45483)
If yes, we can use a similar tri-state option.

Nov 17 2021, 1:56 AM · Restricted Project

Nov 16 2021

peter.smith added a comment to D113901: [lld] Add cet-report and bti-report flags.

I'm in favour of adding the additional flag. It is useful when an effort has been made to get all input files marked, and we want to make sure that the program stays that way. I think that complements the existing force-bti flag which is useful when transitioning a program into compatibility.

Nov 16 2021, 4:36 AM · Restricted Project
peter.smith added a comment to D113771: [ELF] Support the "read-only" memory region attribute.

Overall looks good to me. Some small suggestions for comments based on my thoughts when trying to reason about the change. These are only suggestions though, no strong opinions.

Nov 16 2021, 4:15 AM · Restricted Project, lld

Nov 11 2021

peter.smith accepted D113509: [lld][ELF] Support for R_ARM_THM_JUMP8.

Thanks for the updates LGTM. In theory we can now rewrite arm-thumb-narrow-branch-check.s using .reloc but that could be done in a future patch.

Nov 11 2021, 3:15 AM · Restricted Project

Nov 10 2021

peter.smith accepted D112925: [ELF] Better resemble GNU ld when placing orphan sections into memory regions.

OK, thanks for the update, I've no further comments. I've set accepted, but please wait to see if MaskRay has any further comments.

Nov 10 2021, 10:42 AM · Restricted Project, lld
peter.smith added a comment to D113509: [lld][ELF] Support for R_ARM_THM_JUMP8.

Code looks good to me.

Nov 10 2021, 2:02 AM · Restricted Project

Nov 9 2021

peter.smith added a comment to D113228: [RFC][ELF] Refactor relocation processing.

On a first look I like the way this is going. There is more information in a single place. I've made a few suggestions but none are particularly important for an RFC. If I get time I'll keep thinking to see if I can think of any other suggestions.

Nov 9 2021, 11:23 AM · Restricted Project
peter.smith added a comment to D112925: [ELF] Better resemble GNU ld when placing orphan sections into memory regions.

I had a play around with ld.bfd and found it really hard to get anything like what I'd expect to work. I had the best luck when using ! due to the loose way the sections match and no direct assignment to memory regions.

Nov 9 2021, 10:17 AM · Restricted Project, lld

Nov 8 2021

peter.smith added a comment to D113228: [RFC][ELF] Refactor relocation processing.

Will take a look later this week, hopefully tomorrow. Just got back into the office and have a bit of a backlog of stuff to go through.

Nov 8 2021, 8:46 AM · Restricted Project
peter.smith added inline comments to D112925: [ELF] Better resemble GNU ld when placing orphan sections into memory regions.
Nov 8 2021, 8:41 AM · Restricted Project, lld

Nov 2 2021

peter.smith added a comment to D112925: [ELF] Better resemble GNU ld when placing orphan sections into memory regions.

I'm out of office this week. Will take a look next week if no-one else has.

Nov 2 2021, 7:25 AM · Restricted Project, lld

Oct 31 2021

peter.smith added a reviewer for D112811: [ARM] implement LOAD_STACK_GUARD for remaining targets: ostannard.

Adding ostannard as a reviewer, I'm on vacation this week and Oliver knows this area better than I do.

Oct 31 2021, 1:36 PM · Restricted Project