Rebased and added documentation comments

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?

Rebased prior to commit.

Thanks, I didn't realise this was the case.

@asb thanks, I've just requested commit access!

Remove dependency.

On balance this patch seems a more appropriate solution given it doesn't touch generic code. I hope there isn't a case that isn't covered by this because I honestly can't think of one.

Could you re-title the revision to start with [RISCV] and preferably [MC]? As it is, it may not be seen by all the relevant people.

In D58759#1415688, @efriedma wrote:

I don't understand why it necessary to ensure the label is attached to the "correct" fragment; it's still pointing to the same address either way. Could you try a little harder in RISCVMCExpr::getPCRelHiFixup() to find the symbol? (Given an MCFragment, you can call getPrevNode() to get the previous MCFragment.)

I have yet to add a RISC-V test for the behaviour we're trying to fix, but for now see the discussion on D57240.

In D58759#1413207, @apazos wrote:

Lewis, is this what you had in mind, I only changed RISC-V parser path.

In D57240#1385541, @jrtc27 wrote:

My suspicion is that it would be easier to teach MCAssembler::evaluateFixup how to deal with an extra level of indirection, since the problem stems from it believing the relocation can be evaluated and not knowing about the *real* symbol/relocation we care about.

In D54029#1407041, @lewis-revill wrote:

In D54029#1404753, @apazos wrote:

Hi James, I encountered another case not handled yet in this patch. It produces the error: could not find corresponding %pcrel_hi

.option push
.option norelax
la gp, __global_pointer$
.option pop
la sp, _sp

This also occurs when you manually expand la.

Regardless I believe the problem is caused because EmitLabel doesn't have any checks for whether a new data fragment should be introduced, whereas EmitInstToData does. So when the .Lpcrel_hiX label is expanded/parsed it's parent fragment is the one before the directive changed the STI flags, but the auipc and its %pcrel_hi fixup is located in the fragment after.

When getPCRelHiFixup() searches for the %pcrel_hi it fails to find it since it uses the fragment & offset of the label to search, whereas the fixup is in another fragment.

This is just from some initial debugging, so I'm not sure whether this is the only issue and/or what the best solution would be.

In D54029#1404753, @apazos wrote:

Hi James, I encountered another case not handled yet in this patch. It produces the error: could not find corresponding %pcrel_hi

.option push
.option norelax
la gp, __global_pointer$
.option pop
la sp, _sp

Correct test.

Don't use zero register for 'J' constraint.

I think I ended up being thrown off at the time by the output of something with the '%z' modifier, which I see you have now implemented @jrtc27. I'll correct this patch.

Rebased.

Don't incorrectly set RelaxCandidate for GOT fixups.

Correct local rebasing of older patch; add fixup_riscv_relax as well as tprel_add where appropriate; seperate reporting of errors for incorrectly expanding a tprel_add fixup.

Rebased

Rebased

In D57240#1385261, @jrtc27 wrote:

Could we not have a situation where the pcrel_hi20 is seen *after* the lo12, in which case the lo12 would be forced but the hi20 could still be evaluated? E.g. I think the following would be problematic:

foo:
    j 1f
0:  addi a0, a0, %pcrel_lo(1f)
    ret
1: auipc a0, %pcrel_hi(bar)
    j 0b

.align 2
bar:
    # ...

Obviously extremely pathological code, but nonetheless valid.

Don't output an additional '0' when printing indirect memory operands.

Added RISC-V to thread specifier test as a means of checking TLS is supported.

Rebased with updated dependency

Rebased with updated dependency

Rebased with updated dependency

Abandoned in favour of D57450

Should this only support constant offsets to a symbol? It seems sensible to only support offsets to the symbol, IE add/sub rather than more complex expressions, but the RHS could be any expression at the moment.

Also parse exlaim (!) as a valid token to start an immediate. Add some CSR tests.

Save the token for the parsed LParen rather than reconstructing one. Add some more tests.

In D57320#1373431, @jrtc27 wrote:

There's also AsmToken::Exclaim for logical not that we could allow (although that's not one I've ever seen in real-world assembly). Please add tests for the CSR change?

In D57319#1373436, @jrtc27 wrote:

You should save the original AsmToken rather than reconstructing it, so as to retain the SMLoc, and Buf no longer needs to be at function scope. Also, I had some more exhaustive tests in mine; I think the important one is addi a1, a0, (1) to ensure that we can parse parenthesised immediate expressions when *not* part of a memory reference.

Remove irrelevant dependencies.

Some basic tests should probably be included for completeness.

This patch doesnt interact well with instructions like sw a3, CONST(a4) introduced by D52298. Looks like MatchOperandParserImpl is happy to match the 'CONST' as a bare symbol for the pseudo before parseImmediate can be called. Perhaps one way to fix it is to have parseBareSymbol check for a trailing parenthesis?

Rely on default value rather than explicitly marking TLSSupported as true.

Rebased and fixed FileCheck lines.

Rebased.

Rebased (assuming updates to D55279)

Rebased (assuming updates to D55279)

Rebased.

Rebased.

Rebased.

Rebased and updated comments.

Rebased and updated comment.

Can this be rebased to apply cleanly with the upstreamed D54029?

Rebase with updated dependencies. Use and expand the la.tls.ie and la.tls.gd pseudo instructions.

Rebased with updated dependencies. Renamed ie_hi to got_hi to match relocation, and rearrange to be adjacent to gd_hi in most files. Added tests to relocations.s, and ensured that fixups evaluate correctly.

Fix bad merge and failing tests