I wonder whether it would be nicer to add CC_RISCV_FastCC as a function, much like CC_RISCV, instead, to match the current code style.

In D58531#1662466, @jdoerfert wrote:

I like the patch and I think it is fine.

Small nits:
Could we have a test for " we can detect dodgy pthread_create declarations" and maybe pthread[_attr]_t?
There is an unresolved comment by @shrines.
@probinson: "it seems straightforward enough although clearly needs clang-format-diff run over it."

I'll accept this assuming the above points are easy to fix and given that no one expressed concerns but only positive comments were made.

Rebased, added explicit no-warning test, clang-format'ed, updated assertion message.

Rebased

In D68542#1697636, @draganm wrote:

I don't think you can have frame-pointer based stack unwinding under current Mips ABIs, albeit this might be useful for some stack scan based unwind. Not sure tho.

Note that the AArch64, ARM, PowerPC (32/64/64le), RISCV (32/64) and SystemZ backends all do this (Sparc gets it by virtue of register windowing so it half counts, and similarly X86 due to the hardware stack push), at least with my C test case (which was reduced to give the IR test case addd here); Mips is currently the odd one out.

Fixed slightly differently by D66658 which landed over a month ago.

In D68526#1696587, @lebedev.ri wrote:

There's not a single word in the patch's description.

Having said that, the one warning this raised in FreeBSD (at least for our configuration) was a completely unnecessary use of a character buffer and could be cleaned up, so maybe this case is a useful one to catch. Does anyone else have an opinion on this?

Removed unwanted changes

Stray whitespace

Updated existing tests

A couple of minor remaining points.

I'm still personally not fond of the Fx_F/Fx_D convention. The fact that F0_F < F1_F < F0_D < F1_D is highly surprising, especially when F0_32 < F0_64 < F1_32 < F1_64, and not what one would naively assume. I also don't know whether this peculiarity of TableGen is meant to be something that's relied upon. I know it doesn't match the ISA convention of always calling them Fx, but Fx and Dx have precedence in other backends and do not risk this same confusion. Moreover, the spec itself is highly inconsistent in how to refer to single and double precision floating point:

Hm, I don't particularly like this (or at least the F parts of it); the tables are clear, whereas this seems very brittle.. e.g. when you add Q you have to also modify the places doing arithmetic on F and D registers.

In D66973#1664698, @lenary wrote:

In D66973#1653084, @jrtc27 wrote:

%hi and %lo don't need to be adjacent to relax them; the ABI has been designed with this in mind (and bfd keeps to that) So long as the result of the %hi is always "consumed" by a relaxable %lo, things will work.

Does this apply even if there is another relocation between %hi and %lo?

I'm trying to merge this into our fork. The previous ELF patch was fine, but for this commit I'm having issues because the tool in D64123 doesn't produce this exact result, and so running it on our tree produces a ton of conflicts. Did you use a modified clang-llvm-rename for this (and if so could you please upload it), or are all the discrepancies changes you made by hand afterwards (or with a separate script you would be able to provide)?

In D58531#1611356, @jdoerfert wrote:

In D58531#1599209, @probinson wrote:

We've started running into this too in building the PS4 system. +jdoerfert who added pthread_create to the builtin list.

Looking at the patch, it seems straightforward enough although clearly needs clang-format-diff run over it.
I don't touch Clang that much so I'm reluctant to okay it myself.

@probinson Thanks for making me aware of this patch.

A separate point is whether it makes sense to be emitting this warning in the first place for GE_Missing_type. I'd argue that, if we don't know the type of the builtin, we should never emit the warning

@jrtc27 I hope the immediate need for this is gone after D58091 was finally committed? Given that I caused this mess I can take a look at the patch if you are still interested in it, are you?

Looks sensible in general, just various minor points.

@asb Would be great if you could review this; this is the revision I was referring to back in Boston.

D65205 landed.

%hi and %lo don't need to be adjacent to relax them; the ABI has been designed with this in mind (and bfd keeps to that) So long as the result of the %hi is always "consumed" by a relaxable %lo, things will work.

In D65205#1600772, @lenary wrote:

Is there a reason not to include them in the instruction operand string? It would mean you don't need the custom printer, no?

This is actually really complex. If I include the parens in the tablegen instruction definition, then we definitely cannot support 0(a0) because the parser requires a ( where the 0 is. If i don't include them in the instruction definition, then when parseMemBaseOp runs, it adds ( tokens into the Operands, which means later the register operand is not in the expected place in Operands when operand matching happens.

In D65205#1599193, @lenary wrote:

In D65205#1599133, @jrtc27 wrote:

Yeah this looks like a nice way to do it, better than making an InstAlias for each one. GNU as will accept any expression that evaluates to the constant 0 in place of the 0 (i.e. it's just an imm0, same logic as all the other [su]immX fields, other than discarding it). Could we not therefore reuse part of the generic parser, just with a tweak to drop the immediate if parsed and 0? Something like:

... snip ...

I am worried about this approach, mostly because of the lines like 1268 in parseMemOpBaseReg, where they're pushing on (/) tokens, which aren't in the instruction operand strings (any more). It does seem like these might complicate the pushing/popping more than necessary. I am also not sure we want to support bare register operands.

Yeah this looks like a nice way to do it, better than making an InstAlias for each one. GNU as will accept any expression that evaluates to the constant 0 in place of the 0 (i.e. it's just an imm0, same logic as all the other [su]immX fields, other than discarding it). Could we not therefore reuse part of the generic parser, just with a tweak to drop the immediate if parsed and 0? Something like:

@asb I prefer D60657 over this as it seems to cover everything, and this is a rather big hammer to be hitting the problem with (that GNU as manages to avoid needing). Have you had a look at that one?

Rebased

Seems like a good idea

Looks sensible to me

I don't like the mismatch between LLD and BFD, but this is a definite improvement over the current state. I'm happy for this to be merged, with the intent that discussions about CALL/CALL_PLT continue in the RISC-V ABI world to get a consensus, specify it and then fix any implementations that deviate from it, whether that be BFD, LLD or both.

Added nounwind to tests. Added full stops to comments.

One minor pedantic stylistic comment but otherwise this seems fine to me.

In D57792#1549678, @asb wrote:

In D57792#1549676, @jrtc27 wrote:

In D57792#1549669, @asb wrote:

Is there documentation on the operand modifiers supported by RISC-V binutils anywhere? I'm struggling to find anything...

This is GCC not binutils, and I can't find them documented anywhere other than the source itself: https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/config/riscv/riscv.c?view=markup&revision=271293#l3181

Sorry, I meant GCC.

Thanks, this patch looks good to me. Tiny nit: LLVM coding guidelines prefer full sentences for comments, so the comments added to RISCVAsmPrinter.cpp should end in a full stop.

In D57332#1549661, @asb wrote:

Is this being used in the wild and does it work sensibly with binutils? I'm seeing that binutils seems to be associating the addend with both the emitted R_RISCV_PCREL_HI20 relocation and R_RISCV_RELAX relocation. Maybe this is just a binutils bug and the addend is ignored by the linker but this does seem surprising...

$ cat t.s 
lla a5, a_symbol + (0xFF << 3)
$ ./riscv32-unknown-elf-as t.s
$ ./riscv32-unknown-elf-readelf -r a.out 

Relocation section '.rela.text' at offset 0xec contains 4 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  00000617 R_RISCV_PCREL_HI2 00000000   a_symbol + 7f8
00000000  00000033 R_RISCV_RELAX                7f8
00000004  00000418 R_RISCV_PCREL_LO1 00000000   .L0  + 0
00000004  00000033 R_RISCV_RELAX                0

In D57792#1549669, @asb wrote:

Is there documentation on the operand modifiers supported by RISC-V binutils anywhere? I'm struggling to find anything...

In D63076#1545858, @ruiu wrote:

Does that make a visible difference?

Still disagree about the behaviour of R_RISCV_CALL. You can't just choose the ABI that you want. Please at least put it back to R_PC. @ruiu do you have an opinion on this?

In D63132#1541242, @MaskRay wrote:

@jrtc27 Do this patch and D63076 look good now?

In D63132#1538147, @MaskRay wrote:

GNU ld treats it as undefined:

Yes. That was why I created the patch to fix the musl -fpie -pie link error: ld.lld: error: relocation R_RISCV_PCREL_HI20 cannot refer to absolute symbol: __global_pointer$. This error is lld specific: in -pie/-shared mode, a PC relative relocation to an absolute symbol is not allowed.

In https://groups.google.com/forum/#!searchin/generic-abi/SHN_ABS|sort:date/generic-abi/MC4b4sqnKC0/oQgZxxwMmJAJ, Cary's interpretation of gABI is:

On the contrary, the only interpretation that makes sense to me is that it will not change because of relocation at link time or at load time.

If lld resolves the PC relative relocation statically, the value will be incorrect when the pie or shared object is loaded at runtime because the SHN_ABS symbol's value does not change. The best lld can do is to issue an error.

An alternative is to create a dummy .sdata in musl's Scrt1.o. I don't know how reasonable it is to have lla gp,__global_pointer$ in other code that might not define .sdata.

GNU ld treats it as undefined:

Sorted OS case statements

Hm, actually, looking more closely, there seems to be inconsistency. Some backends use FooELFStreamer.cpp for an MCELFStreamer, and others for a ELF-specific target streamer. Some, like AArch64, even put both in the same file, so if you don't like this patch, I suppose the recommendation would be to do that?

In D61866#1532288, @asb wrote:

I think this is less confusing, but there's more to be gained by having a similar naming scheme to the other backends which right now all use FooELFStreamer.{cpp,h}.

I you feel strongly about this change, I'd consider making it for all in-tree backends.

Ping?

In D60657#1475609, @lewis-revill wrote:

Thanks for this. I realize D58843 is a very heavy-handed approach so I'd be happy if we could avoid it cleanly with target-specific changes, however I'm not certain we would be able to cover all the combinations we would need to with patches like this?

For example would we be able to cover pairs that occur out of order?

Why do we need this flag; can we not already determine this (either in generic code or the target backends)? Anything where all symbols referenced are not in SHF_EXECINSTR sections can be evaluated even with relaxations, which covers all these cases:

Ouch; LGTM.

Also handle .bss where the zeroes are written to the file.

The problem is that Length is the length of the CIE, i.e. the data structure itself; this means it isn't subject to relaxations and so the difference expression can be folded. I don't see much merit in teaching all the tools out there to check relocations for Length when we can instead just emit the "right" thing. I think the fix is to make sure we only return true from requiresDiffExpressionRelocations if a target is in a code section? MCSection::UseCodeAlign will tell you this (or you can use MCSectionELF::getFlags() & ELF::SHF_EXECINSTR to be less general), though perhaps it should be renamed to isCodeSection to match isVirtualSection.

Updated affected tests.