One tiny change to add some punctuation to a comment. This can be done prior to commit, no need to post another review.

This looks correct, although I think there are a few places where we can simplify the code to remove the explicit endianness.

I think that's all the comments I have for this round.

A few more comments on some things that don't look right compared to the GNU ld interface.

I think I've spotted a problem in .ARM.exidx table compression. I also think we can get rid of config->be32 for this patch as nothing else reads it.

I've not finished checking yet, but thought it worth sending the comments that I have so far. Only thing that I can spot that looks wrong is that CMSE is M-profile only so the attributes check looks like it needs adjusting. The remainder are nit level comments.

In D145786#4190640, @efriedma wrote:

In some other cases of iterative algorithms like this, we change the algorithm after a few passes to a version that converges faster. For example, you could consider adding extra padding after inserted thunks, in anticipation of the need for more thunks.

A few small comments to move an error message to Driver.cpp and to remove the --be32 option. This will mean updating the tests.

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. I've left a suggestion that can be applied prior to commit if you want to take it up.

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions.

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions.

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions.

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions.

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. I've left some comments that can be applied prior to commit if you want to take them up.

Actually click the button this time to set approval, see previous comment.

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions.
From looking at the source code alone - assuming that most people would not track down the commit message/review for extra context - I found it difficult to work out the convention for when Flags is used and when Tags is used. I've made a suggestion for some comments. This can be applied prior to commit if you want to take it up.

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. I've left a suggestion for a comment, but this can be applied prior to commit if you want to take it up.

Just one small comment about when traversing relocations from .ARM.exidx sections get implemented.

Looks like a good improvement, one small suggestion on the text, but no strong opinion on it if you prefer the original.

A few small comments concentrating on the Arm specifics.

Thanks for the update. Will try and take a look next week. Ran out of time today.

LGTM too.

I've taken a look from the Arm side, for initial support I think a lot of the comments don't need attention, but may be worth considering for future revisions. While what's here looks similar to other JITLink backends, I don't have any experience with it so I'll need to defer to someone else more familiar with its code base. Very happy to look at the Arm specifics though.

Thanks for the updates. I agree with SImon that this is an improvement worth having.

No objections from me. I guess that --no-print-imm-hex is not honoured as we are not printing the immediate anymore?

No objections to MaskRay's suggestion to merge with an earlier patch. I've made some suggestions to make the error messages be a bit more specific about what is wrong.

No comments on the implementation.

Thanks for the updates, changes look good to me. I think that we can get away without trying to break the floating point options into flags, at least for how floating point units are available in hardware today.

Thanks for the update, one nit in the language, otherwise looks good to me. It is a good reflection of the implementation.

A couple of small suggestions but otherwise looks good to me.

Looks good to me. Some small suggestions around execute-only as I expect this to be a useful multilib variant.

Thanks for the updates, I'm happy with the changes and don't have any more comments. Happy to give my implicit approval.

Thanks for the updates, I'm happy with the changes and don't have any more comments. Happy to give my implicit approval.

Thanks very much for writing this. Will be worth updating the RFC with a link as I think that this is an approachable place to comment on the format and selection model without the implementation detail. I'm happy with what has been written so far. My suggestions are mostly additions rather than modifications.

LGTM, a couple of small suggestions.

Looks good to me on the Arm side.

A couple of small stylisitic suggestions.

Looks good so far. Some stylistic suggestions.

Thanks for the update, I'm happy with the changes. Best wait a bit to let anyone in the US timezone comment.

Looks OK to me. I think we can simplify by always using the movs instruction.

LGTM as avoiding text segment relocations when we can is probably better than trying to add dynamic relocations.

The Code Models https://github.com/ARM-software/abi-aa/blob/main/sysvabi64/sysvabi64.rst#7code-models do clarify that the text segment includes ro-data explicitly for the benefit of signed 32-bit offsets, however I don't think that this should change the decision here to use sdata8 for AArch64 targets. So no objections from me.

No objections, if the option isn't needed then we should remove it from the mandatory part.

No objections. The intention is to cross-compile LLVM to run on another target and this change preserves that. I'm not well versed in the subtle differences between the CMake variables so I'm happy for others to take the lead on that part.

In D139097#4068742, @peter.smith wrote:

In D139097#4062420, @peter.smith wrote:

@peter.smith Were you able to discuss the issue with the GNU team? Can you start a discussion on binutils@sourceware.org regarding this?

My apologies, my day job is very busy at the moment and I've not had chance. I should be able to ask our own internal team later this week, most likely Friday.

I've asked internally. I'll report back as soon as I get an answer, or next week if I don't.

Thanks for the update, LGTM.

I think this is pretty close. I think we shouldn't claim full support for Armv4 as we don't support the R_ARMV4BX relocation yet.

In D142317#4073423, @andrewng wrote:

Perhaps I'm missing something here, but I thought that thread_local variables are zero initialized.

That is my understanding too. An unitialized TLS variable is assigned to .tbss which is the TLS equivalent of .bss.

In D139097#4062420, @peter.smith wrote:

@peter.smith Were you able to discuss the issue with the GNU team? Can you start a discussion on binutils@sourceware.org regarding this?

My apologies, my day job is very busy at the moment and I've not had chance. I should be able to ask our own internal team later this week, most likely Friday.

@peter.smith Were you able to discuss the issue with the GNU team? Can you start a discussion on binutils@sourceware.org regarding this?

I'm happy with these from the Arm v4 support perspective. I think some TODO comments may no longer apply, but these can easily be fixed up pre-commit.

Looks good to me too.

In D139097#4007457, @quic-subhkedi wrote:

@peter.smith Can you address @MaskRay comments to start a discussion on binutils@sourceware.org to discuss the expected nm behavior?

The title implies that this is just adding tests. Can you update the title to include something like Complete Arm BE32 big-endian support and add tests; or move the SyntheticSections change back to Patch [2/3]

It will be worth adding to the description of the difference between BE32 and BE8. BE32 is the legacy way of handling big-endian on Arm systems prior to ArmV6. In this model all relocatable object files are BigEndian, and all the instructions and data are BigEndian. In ArmV6 the byte-invariant BE8 mode was added, in the output ELF file (executable/shared library) this is the same model as AArch64 where the ELF file and Data are big-endian but instructions are little-endian. In contrast to AArch64 relocatable object files are identical in BE8 and BE32, the linker is expected to endian reverse big-endian instructions from big-endian to little-endian. In modern Arm systems BE32 support has been removed, leaving only BE8. However BE32 is a necessary pre-requisite to support BE8.

I've noticed a couple of places where a .word is written in two 16-bit halves. This won't change the result for little-endian to little-endian writes, but could affect big-endian results later. It may also be worth adding [NFCI] to the title as this is a refactoring change (No Functional Changes Intended). Otherwise looks good to me.

Thanks for the updates, I'm happy from the Arm side. Will be worth waiting some time to give MaskRay some time to take a look.

Thanks for the updates. One more suggestion on the naming, but I've not got a strong opinion.

What are the "absolute relocations"? They usually refer to R_ARM_ABS32 and other relocation types that are represented as R_ABS in lld.

When I added the Thunk support I borrowed terminology from Arm's linker. It uses ABS as short for ABSOLUTE which was its term for Non-PI or position dependent.

I recommend a test with --numeric-sort when --print-raw-values is used. As I understand it the numeric sort will use the values with thumb/mips bit cleared, which will mean that when printed some symbols will look out of order. This will look a bit weird but is probably better than having different sorting orders.

I don't think adding state to the shared llvm/Object/ELFObjectFile.h is the right thing to do there. I think that bit of code is used in too many places that are relying on the behaviour being what it is without having it mutated by a stateful parameter. I think you could add an additional member function that retrieved the symbol without any additional processing, I think this would show the intent more clearly, as well as not adding state. llvm-nm could call that function when wanting raw symbol addresses.

Thanks for posting this I've got about half way through. I'll need to re-read the specification as well. Will comment more as I have them.

In D139097#3966468, @shankare wrote:

Shouldn't nm just print the raw value stored in the symbol table like what readelf does ?

$ llvm-readelf -s a.o

Symbol table '.symtab' contains 2 entries:
   Num:    Value  Size Type    Bind   Vis       Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT   UND
     1: 00001003     0 FUNC    GLOBAL DEFAULT     1 foo

$ llvm-nm a.o
00001002 T foo

Thanks for the updates LGTM too.

Which version of nm are you using? The arm-none-eabi-nm from the Arm GNU toolchain (at least in my GCC toolchain) strips off the bottom bit of Thumb STT_FUNC like llvm-nm. The x86_64 version does not and prints the value with the bottom bit set. As such I think that llvm-nm for Arm targets should match arm-non-eabi-nm. I guess if there is a way to pass a triple to llvm-nm that is non-Arm then it shouldn't strip the bottom bit.

Code changes LGTM. I've made some suggestions for the comments and names.

Will take a look next week. I think you should be able to get v5t support from the same changes as the overall Arm/Thumb instruction. That could be done in a follow up patch though.

I've got about as far as the clang changes. As I mentioned in Discourse I'm not familiar enough in this area to give good feedback on the implementation, most if not all my comments fall under the bike shedding category. Will need to leave the high-level feedback to people that are more experienced in this area.

I think it makes sense as the first level of warning control and matching the GNU ld featureset is useful.

Is this symptomatic of a problem in Clang or GCC? It looks like they are not generating the same groups for the same source file which I would expect considering that both claim to implement the itanium ABI. I couldn't understand at first why the STB_GNU_UNIQUE symbols were not getting discarded as part of a non-prevailing group, however the clang and gcc (I used aarch64-none-linux-gnu-g++ (fsf-10.39) 10.1.1 20200629) generated groups have different signatures so we end up with two groups with one STB_GNU_UNIQUE and one STB_WEAK.

LGTM. I think all CPUs can benefit from the linker relaxation of the fused sequence. Worth leaving some time for any other reviewer to object.

Thanks for the update. Code changes look good to me and are localised to ZSTD. Will be worth leaving a little bit of time for objections from other reviewers.

I've left some comments based on a first read of the code. The code looks reasonable to me. I'm not in a position to mention if this gives bad peformance on Windows (if there is a ZSTD consumer on that platform anyway) though.

A reference I found helpful when looking at this in revision ia of the Arm ARM (https://developer.arm.com/documentation/ddi0487/ia/?lang=en) Instructions for accessing the PSTATE fields

CFINV ; Inverts the value of PSTATE.C
MSR DAIFSet, #Imm4 ; Used to set any or all of DAIF to 1
MSR DAIFClr, #Imm4 ; Used to clear any or all of DAIF to 0
MSR SPSel, #Imm4 ; Used to select the Stack Pointer, between SP_EL0 and SP_ELx
MSR UAO, #Imm4 ; Used to set the value of PSTATE.UAO
MSR PAN, #Imm4 ; Used to set the value of PSTATE.PAN
MSR DIT, #Imm4 ; Used to set the value of PSTATE.DIT
MSR SSBS, #Imm4 ; Used to set the value of PSTATE.SSBS
MSR TCO, #Imm4 ; Used to set the value of PSTATE.TCO
MSR ALLINT, #Imm1 ; Used to set the value of PSTATE.ALLINT

No objections from me. Some small suggestions for comments and a way that might catch someone using the relocs array before mergeRels has been called.

LGTM. I'm taking https://groups.google.com/g/generic-abi/c/satyPkuMisk as an acceptance that ELFCOMPRESS_ZSTD will make into ELF.

LGTM. GCC no longer supports Arm architecture prior to v4 so it is likely the alternative of adding support for v3 is not worth it. The only Arm machines running v2 are likely to be the Acorn Archimedes family, these have their own object file format so are unlikely to benefit from LLVM support.

One really small comment, that is only a suggestion. This looks good to me. I've set approved, but please give a bit of time to see if there are any objections.

I can take a look from the perspective of the ABI, not too familiar with the implementation but will try my best. I'm running a bit behind schedule so it may be a few days before I can comment. Very happy if others want to approve, particularly on the implementation.

Thanks for the updates. Looks good to me.

Do you need to update the docs/SymbolizerMarkupFormat.rst to say that backtrace is now implemented?

On a M1 MacBook Air (8 GiB) using the lld speed test https://s3-us-west-2.amazonaws.com/linker-tests/lld-speed-test.tar.xz I didn't see any significant differences with the patch. Both Chromium and Clang GDB index were within a small fraction of a percentage faster. Mozilla showed the largest difference with the patch being about 3% faster. I think most of the results are that this doesn't make a lot of difference on an M1. Which isn't an ideal target to do benchmarking on, nor does it have many threads + some of the CPUs are efficiency cores. However it does at least show that this patch isn't going to harm results either.

Just to say, no objections so far. I don't have a lot of easy access to targets that haven't been benchmarked on already. One that I do have is an M1 Macbook Air. I should be able to cross-compile Clang for AArch64 ELF on that. Although not an ideal machine for benchmarking it may give some info on how this performs on Apple Silicon.

Thanks very much for writing the ABI doc. Will take a look and leave comments on the pull request.

Not a lot to add over what has already been said. IIUC we do have code to detect when an Output Section overlaps and the user has to explicitly choose --noinhibit-exec to force the write? I think that non-deterministic output is reasonable in that case. Perhaps --noinhibit-exec could imply --threads=1 if we were concerned about people relying on order of output.

Thanks for the explanation. LGTM. May be worth waiting a day or so to see if anyone else on the review has any further comments.

Implementation looks good to me. One quick question to clarify my understanding.

Thanks for the update. Looks good to me.