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

Recent Activity

Yesterday

peter.smith updated the diff for D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.
  • Removed support for corner case where there is just a filespec. This permits the INPUT_SECTION_FLAGS logic to be moved to readInputSectionDescription from readOutputSectionDescription.
  • Changed comment in test
Fri, Jan 17, 10:22 AM · Restricted Project
peter.smith added a comment to D72899: [MC] Set sh_link to 0 if the associated symbol is undefined.

The implementation looks good to me. Will defer to others that know -fsanitize-coverage=inline-8bit-counters and LTO better. There was a comment in the PR about adding a dummy non-gcable section instead, I think that this would have the same effect as having the linker handle sh-link == 0, but it might be more portable to other linkers. Any thoughts on whether a dummy section would work better?

Fri, Jan 17, 7:27 AM · Restricted Project
peter.smith added a comment to D72904: [ELF] Allow SHF_LINK_ORDER sections to have sh_link=0.

I'm reluctantly in favour of making the change on the basis that there exists a design using SHF_LINKORDER that lets this happen. My reservation is that in some use cases of SHF_LINKORDER such as .ARM.exidx sections where there is a 1 : 1 relationship between meta-data and "content" (for want of a better word) section then sh_link = 0 would be a sign that something had gone badly wrong. Having said that there isn't any thing in ELF that forbids the use case that this needs. I think that for .ARM.exidx which is the other main user of SHF_LINKORDER we handle it as a special case so this shouldn't make a difference in practice.

Fri, Jan 17, 6:59 AM · Restricted Project
peter.smith added a comment to D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

I'll remove the support for the corner-case of when there is just a filepattern. Most likely won't get the time today, if I don't then I'll update on Monday. Thanks for the comments.

Fri, Jan 17, 6:40 AM · Restricted Project
peter.smith added a comment to D72892: [MC][ARM] Resolve some pcrel fixups at assembly time.

Happy to submit a patch to implement these relocations in LLD, on the initial implementation I concentrated on what llvm-mc could output. Now, we'd be able to write a test in yaml2elf.

Fri, Jan 17, 6:30 AM · Restricted Project
peter.smith updated the diff for D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

Updated diff. Main changes:

  • Use stringswitch with enumerations
  • Incorporate simplification suggestions
Fri, Jan 17, 4:28 AM · Restricted Project
peter.smith added a comment to D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

Thanks for the review. I'll upload another patch that I hope to have addressed all the comments.

Fri, Jan 17, 4:25 AM · Restricted Project
peter.smith committed rG01ad4c838466: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS. (authored by peter.smith).
[LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS.
Fri, Jan 17, 2:52 AM
peter.smith closed D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..
Fri, Jan 17, 2:52 AM · Restricted Project
peter.smith added a comment to D72892: [MC][ARM] Resolve some pcrel fixups at assembly time.

As I understand it these relocation types are not often implemented is due to their short range. The Arm and Thumb2 ldr (literal) range (search for A8.8.64 in https://static.docs.arm.com/ddi0406/c/DDI0406C_C_arm_architecture_reference_manual.pdf) is:

Encoding T2 or A1 Any value in the range -4095 to 4095.

Outside of carefully crafted section orders, the chances of the relocation being in range if it went outside of a section boundary is very low. If the relocation is within a section boundary then the assembler can resolve it.

Fri, Jan 17, 1:37 AM · Restricted Project

Thu, Jan 16

peter.smith updated the diff for D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

Address code review comments so far. Main changes:

  • Rebased
  • Used EnumEntry as in ELFDumper.cpp
  • Updated parser with better diagnostic message
  • Updated tests
Thu, Jan 16, 9:25 AM · Restricted Project
peter.smith added a comment to D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

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

Thu, Jan 16, 9:25 AM · Restricted Project
peter.smith updated the diff for D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..

Updated diff (and description) to simplify part of the check to os->size > target->getThunkSectionSpacing(). This is conservative, but simple. We can in theory do better, but it is difficult to do so without using heuristics. In effect it is a judgement that ThunkSections close to, but not necessarily at the end of the OutputSection have less of a chance of generating sufficient extra thunks and patches to cause non-convergence.

Thu, Jan 16, 6:53 AM · Restricted Project
peter.smith added a comment to D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..

Thanks for the comments. I'll post an update with a simplification as I don't think I can justify the heuristic without being too hand wavy.

Thu, Jan 16, 3:31 AM · Restricted Project
peter.smith added a comment to D72819: [ELF] Decrease alignment of ThunkSection on 64-bit targets from 8 to 4.

I'm happy to go ahead with this. The maximum alignment for instructions on Arm and AArch64 is 4, which is what the compiler uses for code sections.

Thu, Jan 16, 1:40 AM · Restricted Project

Wed, Jan 15

peter.smith added a comment to D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.

Thanks for the comments, I'll make an update tomorrow. I'll plan to:

  • Try the flagsMask + withMask to see if it looks any better.
  • Limit the recognised flags to what BFD accepts, I'd like to continue to recognise integers.
  • Look at the error reporting in the parser.
  • Update the tests.
Wed, Jan 15, 11:18 AM · Restricted Project
peter.smith created D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS.
Wed, Jan 15, 2:46 AM · Restricted Project

Tue, Jan 14

peter.smith updated the diff for D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..

Updated the fix, and description after testing on a real linux kernel allyesconfig build. The description contains the full details, which should also be present in the comment. In summary we round up the size of a thunk section to nearest 4KiB when:

  • The erratum fix is being used.
  • The thunk section may be placed before the end of Output Section so there may be code after it.
  • The InputSectionDescription is larger than 4KiB.
Tue, Jan 14, 9:55 AM · Restricted Project
peter.smith added a comment to D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..

Thanks for the comments. I've decided to rework this In light of it not actually fixing the problem in Linux. In the allyesconfig build the overall .text OutputSection is larger than thunkSectionSpacing, but the assert was for in effect an InputSectionDescription within .text so I've had to think again. I'm nearly ready to submit again, just got to test in on the kernel itself first.

Tue, Jan 14, 4:09 AM · Restricted Project

Mon, Jan 13

peter.smith added a comment to D71688: [AArch64] Add -mtls-size option for ELF targets.

Committed as 10c11e4e2d05cf0e8f8251f50d84ce77eb1e9b8d , I've used the new attribution process https://llvm.org/docs/DeveloperPolicy.html so you should show up as the author of the patch in the commit log. I'll comment here if there are any problems with buildbots.

Mon, Jan 13, 2:25 AM · Restricted Project, Restricted Project
peter.smith committed rG10c11e4e2d05: This option allows selecting the TLS size in the local exec TLS model, which is… (authored by kawashima-fj).
This option allows selecting the TLS size in the local exec TLS model, which is…
Mon, Jan 13, 2:22 AM
peter.smith closed D71688: [AArch64] Add -mtls-size option for ELF targets.
Mon, Jan 13, 2:22 AM · Restricted Project, Restricted Project

Fri, Jan 10

peter.smith accepted D72474: [ELF] Make TargetInfo::writeIgotPlt a no-op.

LGTM. ld.bfd, at least on AARCH64 will write the address of PLT[0] into the .got.plt, but this may just be out of convenience as it will be overwritten by the IRELATIVE relocation before it can be used.

Fri, Jan 10, 4:33 AM · Restricted Project
peter.smith accepted D72215: [AArch64] Add function attribute "patchable-function-entry" to add NOPs at function entry.

LGTM thanks for the update.

Fri, Jan 10, 3:58 AM · Restricted Project
peter.smith added a comment to D71688: [AArch64] Add -mtls-size option for ELF targets.

I can do it early next week, unfortunately a little too busy today. If anyone else can do it earlier please go ahead. To obtain commit access it is worth following https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Fri, Jan 10, 2:35 AM · Restricted Project, Restricted Project
peter.smith accepted D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

I'm personally strongly enough in favour to approve given no strong objections so far. Will be worth holding off to next week to see if the approval provokes any.

Fri, Jan 10, 2:11 AM · Restricted Project

Wed, Jan 8

peter.smith added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

If I could summarise the comments so far:

  • There is some concern that this is not the full solution and there maybe other things that can break interposing.
  • The response is that fixing the assembler will be a necessary part of that work.
  • There maybe some extra via-PLT calls in shared libraries if -ffunction-sections is not used, but these can be mitigated by giving the symbols non-default visibility, and in any case these would exist with -ffunction-sections.
Wed, Jan 8, 8:56 AM · Restricted Project
peter.smith added a comment to D72215: [AArch64] Add function attribute "patchable-function-entry" to add NOPs at function entry.

I think GCC's __patchable_function_entries has severe design flaws. https://gcc.gnu.org/ml/gcc/2020-01/msg00067.html

I have tried a different design in this patch, temporarily naming it __patchable_function_entry.

Wed, Jan 8, 2:02 AM · Restricted Project
peter.smith accepted D71688: [AArch64] Add -mtls-size option for ELF targets.

Thanks for the update, looks good to me.

Wed, Jan 8, 1:20 AM · Restricted Project, Restricted Project

Tue, Jan 7

peter.smith accepted D71794: [ELF] Don't special case weak symbols for pie with no shared objects.

Ok LGTM. May be worth waiting a day to see if there are any final objections.

Tue, Jan 7, 10:35 AM · Restricted Project
peter.smith added a comment to D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..

On looking again at the linux kernel makefile

Tue, Jan 7, 10:07 AM · Restricted Project
peter.smith created D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS..
Tue, Jan 7, 9:27 AM · Restricted Project
peter.smith added a comment to D72215: [AArch64] Add function attribute "patchable-function-entry" to add NOPs at function entry.

Address comments.

The section __patchable_function_entries needs more thoughts so it is not
implemented. I'll probably use SHF_LINK_ORDER when -ffunction-sections, and add
SHF_WRITE (to prevent text relocations; apparently GCC does not consider these
issues).

Tue, Jan 7, 6:09 AM · Restricted Project
peter.smith added a comment to D71688: [AArch64] Add -mtls-size option for ELF targets.

Apologies for the delay in responding, just come back from vacation. I've checked the implementation against GCC and it looks like it will give the same behaviour. I've got one minor suggestion surrounding the clamping of TLSSize to its maximum value. It looks like it would only need to be done once, is there a convenient place, such as AArch64TargetMachine where it can be done once?

No need to apologize. I know many people are in vacation last two or three weeks.

I'm not sure whether I understand your suggestion correctly. Do you mean the following code in the LowerELFTLSLocalExec function should be in a function in AArch64TargetMachine or somewhere which is called only once because LowerELFTLSLocalExec is called for every thread local variable? If so, it makes sense. I'll find a convenient place.

if (TLSSize == 0) // default
  TLSSize = 24;
if ((CModel == CodeModel::Small || CModel == CodeModel::Kernel) &&
    TLSSize > 32)
  // for the small (and kernel) code model, the maximum TLS size is 4GiB
  TLSSize = 32;
else if (CModel == CodeModel::Tiny && TLSSize > 24)
  // for the tiny code model, the maximum TLS size is 1MiB (< 16MiB)
  TLSSize = 24;
Tue, Jan 7, 6:09 AM · Restricted Project, Restricted Project
peter.smith added a comment to D71688: [AArch64] Add -mtls-size option for ELF targets.

Apologies for the delay in responding, just come back from vacation. I've checked the implementation against GCC and it looks like it will give the same behaviour. I've got one minor suggestion surrounding the clamping of TLSSize to its maximum value. It looks like it would only need to be done once, is there a convenient place, such as AArch64TargetMachine where it can be done once?

Tue, Jan 7, 4:19 AM · Restricted Project, Restricted Project
peter.smith added a comment to D71794: [ELF] Don't special case weak symbols for pie with no shared objects.

Apologies for the delay in responding, the original came in when I was on vacation.

  • Am I correct in thinking that the extra dynamic symbols that will result with -pie and no shared libraries are just wasted space, but won't cause anything to happen at run time as there are no dynamic relocations generated against them?
  • The main argument for removing is that this our ld.bfd is ad-hoc and this part doesn't help much, removing it would simplify here and permit a simplification in D71795. Is there a plan beyond D71795 to more closely follow ld.bfd behaviour, or is this just an area where we expect cosmetic inconsistencies?
Tue, Jan 7, 2:46 AM · Restricted Project
peter.smith committed rG051c4d5b7bcf: [LLD][ELF][AArch64] Do not use thunk for undefined weak symbol. (authored by peter.smith).
[LLD][ELF][AArch64] Do not use thunk for undefined weak symbol.
Tue, Jan 7, 2:00 AM
peter.smith closed D72267: [LLD][ELF][AArch64] Do not use thunk for undefined weak symbol..
Tue, Jan 7, 1:59 AM · Restricted Project

Mon, Jan 6

peter.smith created D72267: [LLD][ELF][AArch64] Do not use thunk for undefined weak symbol..
Mon, Jan 6, 6:51 AM · Restricted Project
peter.smith added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

I'm not sure of the history of LLVM policy on this. I note that as of today it is possible to get different program behaviour (either performance regressions due to extra PLT entries, or interposing) with -ffunction-sections and without as there will always be a relocation for a branch to another global STT_FUNC symbol with -ffunction-sections as it will be in a different section.

Mon, Jan 6, 4:07 AM · Restricted Project

Dec 19 2019

peter.smith added a comment to D71688: [AArch64] Add -mtls-size option for ELF targets.

Apologies, I'm out of office for the next couple of weeks. If you don't get any other volunteers to review can you ping again in January, I'll be back on the 6th.

Dec 19 2019, 1:52 AM · Restricted Project, Restricted Project

Dec 16 2019

peter.smith added a comment to D71519: [ELF] Add IpltSection.

Looks good to me. One minor nit about a stale comment. I'll be out of office for the next few weeks, I'll check to make sure there isn't anything critical but otherwise might be a bit slow to respond.

Dec 16 2019, 2:03 AM · Restricted Project
peter.smith accepted D71520: [ELF] Rename .plt to .iplt and decrease EM_PPC{,64} alignment of .glink to 4.

Looks good to me too.

Dec 16 2019, 1:45 AM · Restricted Project

Dec 13 2019

peter.smith accepted D71161: [ELF] Update st_size when merging a common symbol with a shared symbol.

Looks good to me. Will be worth seeing if there are any comments in the US time zone.

Dec 13 2019, 12:25 AM · Restricted Project

Dec 11 2019

peter.smith committed rG86d24193a9eb: [LLD][ELF][AArch64][ARM] When errata patching, round thunk size to 4KiB. (authored by peter.smith).
[LLD][ELF][AArch64][ARM] When errata patching, round thunk size to 4KiB.
Dec 11 2019, 6:12 AM
peter.smith closed D71281: [LLD][ELF][AArch64][ARM] When errata patching, round thunk size to page boundary.
Dec 11 2019, 6:12 AM · Restricted Project
peter.smith committed rG247b2ce11cf0: [LLD][ELF][AArch64][ARM] Add missing classof to patch sections. (authored by peter.smith).
[LLD][ELF][AArch64][ARM] Add missing classof to patch sections.
Dec 11 2019, 6:12 AM
peter.smith closed D71242: [LLD][ELF]{ARM][AArch64] Add missing classof to patch sections..
Dec 11 2019, 6:12 AM · Restricted Project
peter.smith added a comment to D71281: [LLD][ELF][AArch64][ARM] When errata patching, round thunk size to page boundary.

LGTM

Thank you for fixing the issue. This is an interesting edge case that I haven't thought about that before.

Dec 11 2019, 6:12 AM · Restricted Project
peter.smith accepted D71327: [ELF][AArch64] Rename --force-bti to -z force-bti and --pac-plt to -z pac-plt.

Looks good to me.

Dec 11 2019, 1:52 AM · Restricted Project

Dec 10 2019

peter.smith updated the diff for D71281: [LLD][ELF][AArch64][ARM] When errata patching, round thunk size to page boundary.

Updated diff with test changes.

Dec 10 2019, 11:04 AM · Restricted Project
peter.smith added a comment to D71281: [LLD][ELF][AArch64][ARM] When errata patching, round thunk size to page boundary.

Thanks, I'll upload a new diff with the test changes.

Dec 10 2019, 11:04 AM · Restricted Project
peter.smith created D71281: [LLD][ELF][AArch64][ARM] When errata patching, round thunk size to page boundary.
Dec 10 2019, 8:55 AM · Restricted Project
peter.smith created D71242: [LLD][ELF]{ARM][AArch64] Add missing classof to patch sections..
Dec 10 2019, 1:47 AM · Restricted Project
peter.smith accepted D71019: [AArch64] Emit PAC/BTI .note.gnu.property flags.

Looks good to me. Will be worth giving Tim or other reviewers a day for a last chance to comment before committing.

Dec 10 2019, 12:25 AM · Restricted Project

Dec 9 2019

peter.smith added a comment to D71161: [ELF] Update st_size when merging a common symbol with a shared symbol.

Reading through the PR I think that this is the right thing to. I think that there is a possibility of narrowing down the assumption that a Shared definition is common, by checking its address against the loadable segment containing BSS. I don't think it is worth doing that.

Dec 9 2019, 11:29 AM · Restricted Project
peter.smith added a comment to D71157: [ELF] Refine section group --gc-sections rules to not discard .debug_types.

Assuming George is happy with them, changes look good to me,

Dec 9 2019, 11:02 AM · Restricted Project
peter.smith added inline comments to D71163: [ELF] --icf: do not fold preemptible symbols.
Dec 9 2019, 1:53 AM · Restricted Project

Dec 6 2019

peter.smith added a comment to D71019: [AArch64] Emit PAC/BTI .note.gnu.property flags.

Apologies for the delay in responding. I've got a query about the alignment of the .note.gnu.property sections. As I understand it they should be 8-byte aligned for a 64-bit machine. I also think it would be helpful to warn that if at least one, but not all functions have the branch-target-enforcement attribute.

Dec 6 2019, 10:23 AM · Restricted Project

Dec 5 2019

peter.smith committed rG784f57584fc3: [LLD][ELF][AArch64] .note.gnu.property sections should have alignment 8 (authored by peter.smith).
[LLD][ELF][AArch64] .note.gnu.property sections should have alignment 8
Dec 5 2019, 2:21 AM
peter.smith closed D70962: [LLD][ELF] .note.gnu.property sections should have wordsize alignment.
Dec 5 2019, 2:21 AM · Restricted Project
peter.smith committed rG4d6c4cb4269e: [LLD][ELF] Add support for PT_GNU_PROPERTY (authored by peter.smith).
[LLD][ELF] Add support for PT_GNU_PROPERTY
Dec 5 2019, 2:12 AM
peter.smith closed D70961: [LLD][ELF] add support for PT_GNU_PROPERTY.
Dec 5 2019, 2:11 AM · Restricted Project

Dec 4 2019

peter.smith updated the diff for D70961: [LLD][ELF] add support for PT_GNU_PROPERTY.

Uploading diff, I could have sworn I did this early today. Maybe I forgot to press something.

Dec 4 2019, 10:00 AM · Restricted Project
peter.smith committed rG2120612e46bc: [ELF] Support for PT_GNU_PROPERTY in header and tools (authored by peter.smith).
[ELF] Support for PT_GNU_PROPERTY in header and tools
Dec 4 2019, 7:58 AM
peter.smith closed D70959: [ELF] Support for PT_GNU_PROPERTY in program headers and tools.
Dec 4 2019, 7:58 AM · Restricted Project
peter.smith added a comment to D70961: [LLD][ELF] add support for PT_GNU_PROPERTY.

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

Dec 4 2019, 2:26 AM · Restricted Project
peter.smith added a comment to D70959: [ELF] Support for PT_GNU_PROPERTY in program headers and tools.

LGTM, with @MaskRay's comment addressed (I don't mind if the comment is deleted, or kept, but it should at least be fixed).

On the LLD side, I'm assuming that LLD needs to emit it because the kernel in question doesn't parse the PT_NOTE segment for this section? It seems a bit superfluous otherwise...

Dec 4 2019, 1:58 AM · Restricted Project

Dec 3 2019

peter.smith updated the diff for D70962: [LLD][ELF] .note.gnu.property sections should have wordsize alignment.

Thanks both for the comments. Updated to use config->wordsize, converted test to x86_64 and added 32-bit x86 test for the alignment 4 case.

Dec 3 2019, 10:24 AM · Restricted Project
peter.smith updated the diff for D70961: [LLD][ELF] add support for PT_GNU_PROPERTY.

Thanks for the comments. Converted test to x86_64, removed indendation and fixed comments.

Dec 3 2019, 10:15 AM · Restricted Project
peter.smith updated the diff for D70959: [ELF] Support for PT_GNU_PROPERTY in program headers and tools.

Thanks for the comments. I've paired the tests back to only print the header type. The GNU readelf string type matches, it looks like GNU objdump is missing the string, I've followed the same convention it uses for PT_GNU_RELRO, which is just RELRO.

Dec 3 2019, 9:18 AM · Restricted Project
peter.smith added inline comments to D70959: [ELF] Support for PT_GNU_PROPERTY in program headers and tools.
Dec 3 2019, 9:18 AM · Restricted Project
peter.smith updated the diff for D70959: [ELF] Support for PT_GNU_PROPERTY in program headers and tools.

Thanks for the comments. I've updated the diff with context, apologies it has been some time since I last posted a review and I forgot. I've also applied the suggested changes to the test.

Dec 3 2019, 6:59 AM · Restricted Project
peter.smith updated the diff for D70961: [LLD][ELF] add support for PT_GNU_PROPERTY.

Updated to add context to diffs.

Dec 3 2019, 6:40 AM · Restricted Project
peter.smith updated the diff for D70962: [LLD][ELF] .note.gnu.property sections should have wordsize alignment.

Updated to add context.

Dec 3 2019, 6:40 AM · Restricted Project
peter.smith added a child revision for D70961: [LLD][ELF] add support for PT_GNU_PROPERTY: D70962: [LLD][ELF] .note.gnu.property sections should have wordsize alignment.
Dec 3 2019, 6:13 AM · Restricted Project
peter.smith created D70962: [LLD][ELF] .note.gnu.property sections should have wordsize alignment.
Dec 3 2019, 6:13 AM · Restricted Project
peter.smith added a child revision for D70959: [ELF] Support for PT_GNU_PROPERTY in program headers and tools: D70961: [LLD][ELF] add support for PT_GNU_PROPERTY.
Dec 3 2019, 6:04 AM · Restricted Project
peter.smith created D70961: [LLD][ELF] add support for PT_GNU_PROPERTY.
Dec 3 2019, 6:04 AM · Restricted Project
peter.smith created D70959: [ELF] Support for PT_GNU_PROPERTY in program headers and tools.
Dec 3 2019, 5:54 AM · Restricted Project
peter.smith added a comment to D70937: [ELF][PPC64] Support long branch thunks with addends.

The change looks reasonable to me but I'll defer to the PPC experts for the PPC specific changes, as I'm not that familiar with it.

Dec 3 2019, 3:18 AM · Restricted Project

Nov 28 2019

peter.smith added a comment to D70815: Enable `-funwind-tables` flag when building libunwind.

I've put some comments inline. I'm no CMake expert though, so there may be other reviewers that can make alternative suggestions.

Nov 28 2019, 5:35 AM · Restricted Project

Nov 27 2019

peter.smith accepted D70637: [ELF][AArch64] Support R_AARCH64_{CALL26,JUMP26} range extension thunks with addends.

I'm happy with this change. It would be worth waiting a bit to see if there are any comments from other reviewers.

Nov 27 2019, 6:45 AM · Restricted Project
peter.smith accepted D70690: [ELF][ARM] Add getPCBias().

LGTM. I think this looks a lot cleaner thanks. I also agree that the getISDThunk call sites don't need modifying as at this point the Thunk is our destination and we don't want an addend from the thunk symbol.

Nov 27 2019, 6:45 AM · Restricted Project
peter.smith added a comment to D70766: Consolidate global variables to a single place.

There are a couple of downsides to putting all the globals in one place. I think the LLD codebase is probably small enough for them to not be too serious:

  • It couples pretty much every file to a single place. Touching this file means recompiling everything.
  • It can make the code harder to reason about as any file can see all the globals, and can potentially modify them. This is a possible argument for not promoting statics to globals as the scope of a static is much easier to reason about.
Nov 27 2019, 6:08 AM · Restricted Project

Nov 26 2019

peter.smith added a comment to D70637: [ELF][AArch64] Support R_AARCH64_{CALL26,JUMP26} range extension thunks with addends.

Can you elaborate? My understanding is that needsThunk() and inBranchRange() assume all Arm branches to have an immediate (which becomes the relocation addend) of -8 and Thumb branches to have -4. This cancels the PC-bias of 8-bytes in Arm and 4-bytes in Thumb.

This is my current thinking. If the addend from the branch immediate, whether read from the instruction, or assumed to be -8 for an Arm branch or -4 for a Thumb branch, is passed in to needsThunk() I think that we could do:

src = branchAddr + (Is Thumb branch then + 4, else +8)
dst = (expr == R_PLT_PC) ? s.getPltVA(addend) : s.getVA(addend);

Created D70690 as a prerequisite change to make this patch affect ARM even less.

The update in test/ELF/arm-thumb-mix-range-thunk-os.s is related to the following diff:

if (auto *d = dyn_cast<Defined>(rel.sym))
  if (!d->isInPlt() && d->section)
    thunkVec = &thunkedSymbolsBySectionAndAddend[{
        {d->section->repl, d->value}, rel.addend}];  // If I use 0 instead of rel.addend,
if (!thunkVec)
  thunkVec = &thunkedSymbols[{rel.sym, rel.addend}]; // If I use 0 instead of rel.addend,  test/ELF/arm-thumb-mix-range-thunk-os.s will not need an update.

The R_ARM_THM_CALL in tfunc33 causes creation of a Thumb thunk. The R_ARM_CALL in tfunc34 have different implicit addends, so making rel.addend part of the key will create an ARM thunk, rather than reuse the existing Thumb thunk. If reusing the thunk is the intended behavior, I'll change it to

// This ternary expression makes the code less general.
int64_t addend = config->emachine == EM_ARM ? 0 : rel.addend;
 
if (auto *d = dyn_cast<Defined>(rel.sym))
  if (!d->isInPlt() && d->section)
    thunkVec = &thunkedSymbolsBySectionAndAddend[{
        {d->section->repl, d->value}, addend}];
if (!thunkVec)
  thunkVec = &thunkedSymbols[{rel.sym, addend}];

can keep the test unchanged.

Nov 26 2019, 2:34 AM · Restricted Project
peter.smith added a comment to D70690: [ELF][ARM] Add getPCBias().

Is there any reason not to have just one function inBranchRange() and assume that it needs to take care of the relocation addend explicitly, that way the PC Bias could be limited to the ARM implementation of inBranchRange() and inBranchRangeBeforePCBias wouldn't be needed?

Nov 26 2019, 2:06 AM · Restricted Project

Nov 25 2019

peter.smith added a comment to D70637: [ELF][AArch64] Support R_AARCH64_{CALL26,JUMP26} range extension thunks with addends.

I'm comfortable with the detailed code-changes. I initially thought that using a nested pair might make the code too difficult to read, but it is clear at the construction of the pair which field is which. I'd definitely want to avoid referring the elements of the pair with first.first or first.second, but as we only use it as a key it doesn't matter.

Nov 25 2019, 9:10 AM · Restricted Project
peter.smith accepted D70658: [LLD][ELF] - Make compression level be dependent on -On..

Looks good to me, looks like it matches the discussion in https://bugs.llvm.org/show_bug.cgi?id=44089 will be worth waiting for US timezone based reviewers to comment.

Nov 25 2019, 4:14 AM · Restricted Project
peter.smith added a comment to D70637: [ELF][AArch64] Support R_AARCH64_{CALL26,JUMP26} range extension thunks with addends.

Will look at the code in detail this afternoon.

Nov 25 2019, 4:05 AM · Restricted Project

Nov 22 2019

peter.smith added a comment to D70505: [ELF] Replace SymbolTable::forEachSymbol with iterator_range symbols().

I'm happy with this too.

Nov 22 2019, 2:05 AM · Restricted Project

Nov 21 2019

peter.smith accepted D70506: [ELF] Add a corrector for case mismatch problems.

I think this is a good idea, I'd say most of my typos are of this form, such as SubTarget vs Subtarget. Will be worth waiting to see if anyone in US timezone has any objections.

Nov 21 2019, 5:02 AM · Restricted Project
peter.smith added a comment to D70505: [ELF] Replace SymbolTable::forEachSymbol with iterator_range symbols().

I'm comfortable with defining a filter iterator, I think the trade off of increased complexity vs simple interface is worth it. I can see some benefit in `for (Symbol *sym : symtab->symbols())` as it is not clear from sym : symtab that PlaceHolders are being skipped.

Nov 21 2019, 1:39 AM · Restricted Project

Nov 19 2019

peter.smith added a comment to D70146: [ELF] Improve --gc-sections compatibility with GNU ld regarding section groups.

This looks good to me too.

Nov 19 2019, 2:19 AM · Restricted Project
peter.smith added a comment to D70421: Initialize global vectors with invalid members to catch uninitialization errors.

One thing that might work better than potentially random problems if the containers aren't cleared, is some kind of assert only, or self-diagnosis mode that could give an assert or error message that a data-structure hadn't been cleared. For example assert(inputSections.empty()); assert(In.bss == nullptr);. With asserts checking I think we could get away with a small set of tests to run twice with some --self-diagnostic-option to check the majority of the cases.

Nov 19 2019, 2:19 AM · Restricted Project

Nov 13 2019

peter.smith added a comment to D70146: [ELF] Improve --gc-sections compatibility with GNU ld regarding section groups.

I'm broadly in favour of making this change. I recognise there are some concerns about the intent behind the ELF specification, which is largely written without garbage collection in mind. One reading of it is that garbage collection is orthogonal to group selection, and the all or nothing comments relate to group selection only. Will be good to wait a few days to see if there are any strong objections. I tend towards the conservative approach unless there are some demonstrable benefits from not doing so.

Nov 13 2019, 3:01 AM · Restricted Project

Nov 12 2019

peter.smith added a comment to D69411: [MC] Parse .if conditions with symbols in consecutive MCDataFragements.

Thanks for the summary.

To summarise where I think we are right now.

  • D76002 fixes a problem affecting within MCDataFragment eager evaluation of .if expressions. Specifically when there is a label that would be inserted into the MCDataFragment, but at the time of encountering the label the fragment hadn't been created. MC will attempt to reuse an MCDataFragment for new instructions, see CanReuseDataFragment() in MCObjectStreamer.cpp. As noted earlier when the Subtarget changes a new MCDataFragment is created. In the majority of cases this is done via a .arch or .arch_extension directive.
  • This patch extends the eager evaluation to cope with two adjacent MCDataFragments. My understanding is that this only occurs in the following circumstances:
    • When bundle locking and --mc-relax-all is used, there is a complicated 1 instruction per fragment + fragment merging that goes on here. This is only used in NaCl which I'm not sure what the status of in Chrome is. I think it is at best deprecated.

I'll follow up on this. There's likely a lot of code that can be dropped if that's the case.

    • When there is a Subtarget change between MCDataFragments.
  • In all other cases such as .align, .space and a relaxable instruction there is a separate non MCDataFragment created so we cannot fix these up.
  • The patch restricts the eager evaluation to .if as some asm backends do not want the expressions between fragments evaluated eagerly in some cases.

    From the perspective of the linux kernel, is D76002 sufficient?

I can still reproduce https://github.com/ClangBuiltLinux/linux/issues/742 with https://reviews.llvm.org/D70062 applied.

Nov 12 2019, 11:32 AM · Restricted Project
peter.smith added a comment to D69411: [MC] Parse .if conditions with symbols in consecutive MCDataFragements.

To summarise where I think we are right now.

  • D76002 fixes a problem affecting within MCDataFragment eager evaluation of .if expressions. Specifically when there is a label that would be inserted into the MCDataFragment, but at the time of encountering the label the fragment hadn't been created. MC will attempt to reuse an MCDataFragment for new instructions, see CanReuseDataFragment() in MCObjectStreamer.cpp. As noted earlier when the Subtarget changes a new MCDataFragment is created. In the majority of cases this is done via a .arch or .arch_extension directive.
  • This patch extends the eager evaluation to cope with two adjacent MCDataFragments. My understanding is that this only occurs in the following circumstances:
    • When bundle locking and --mc-relax-all is used, there is a complicated 1 instruction per fragment + fragment merging that goes on here. This is only used in NaCl which I'm not sure what the status of in Chrome is. I think it is at best deprecated.
    • When there is a Subtarget change between MCDataFragments.
  • In all other cases such as .align, .space and a relaxable instruction there is a separate non MCDataFragment created so we cannot fix these up.
  • The patch restricts the eager evaluation to .if as some asm backends do not want the expressions between fragments evaluated eagerly in some cases.
Nov 12 2019, 4:04 AM · Restricted Project
peter.smith added a comment to D70020: [lld] Better support for group semantic wrt. notes.

Thanks for the updates, I've no more comments and have no objections.

Nov 12 2019, 2:31 AM · Restricted Project, lld
peter.smith added a comment to D70062: MCObjectStreamer: assign MCSymbols in the dummy fragment to offset 0..

I'm happy with this too. I checked the ARM Mapping Symbol code, and it only uses the EmitLabelAtPos in one specific circumstance, a pending data mapping symbol which always has a MCDataFragment associated with it.

Nov 12 2019, 2:08 AM · Restricted Project