LGTM.

Thanks for all of the improvements so far.

Adding a test showing that using .option arch in inline assembly doesn't affect ELF attributes nor set the EF_RISCV_RVC ELF flag would be nice. The latter can also be tested for assembly files.

Overall LGTM. Only some minor points left to address.

You now accept .option arch, rv32ic (without the quotes) but that's not covered in the valid tests.
Also, you're normalizing that to the quoted version .option arch, "rv32i2p0_c2p0" and accepting that but, at first glance, I'm not seeing that as an accepted case in the binutils tests.

Update with the re-landed patch.

In D142282#4164562, @stefanp wrote:

On AIX we end up without a definition for findModulesAndOffsets so very likely the combination of #if, #elif, #else, #endif is excluding AIX from all of the definitions and it did not do that before..

In D144174#4164725, @luismarques wrote:

AFAICT it would not improve the GNU backtrace situation.

From Kito's comment in D144174: https://github.com/gcc-mirror/gcc/commit/3cd08f7168c196d7a481b9ed9f4289fd1f14eea8

In D144174#4164652, @kito-cheng wrote:

RISC-V GCC has added -fasynchronous-unwind-tables to the default option for linux, maybe we should do that on clang side? this should able improve the situation.

In D144174#4141913, @reames wrote:

Can you give a bit more context on *why* backtrace is broken? And on which systems you've observed this? This is the first time I'm hearing this.

Yes, this can land independently of the other work. Since D144839 was abandoned I'll update and land this.
I'll also work on the unit test and llvm-symbolizer improvement, I've just been busy with childcare and other work.

@StephenFan Can you please rebase this?

I suppose we could also have added S-type instructions tests, for thoroughness.

The reasoning and approach makes sense to me. I haven't (yet) checked the implementation and test coverage in detail, so this is a weak approval only.

In D143345#4130585, @craig.topper wrote:

It'd be really cool if we could use the relocation to print %pcrel_lo(sym) instead of 0. But I don't know how much work that would be.

In D143345#4130539, @luismarques wrote:

I'm not sure I understand your second question, though (it's late here...). I assume it's more than just a curiosity and it has some relation to this patch but it's not clear to me what the implication is. Yes, without -r the disassemblies can be a bit puzzling at times, and you need to anticipate that some 0 immediates are misleading, and they won't actually be zero in the final binary. You have the same problem with mvs, just exacerbated because then you have to think that they aren't even true mv instructions at all. Is your point that when you do use -r it should be clear that the mvs aren't actually moves and the addis aren't actually zero? Apologies if I'm being dense.

In D143345#4129968, @craig.topper wrote:

I think the reasoning makes sense to me. Do you also use -r when you disassemble object files? Or do you anticipate just knowing that an addi with 0 is some unrelocated address?

Does this mean that Android, OpenBSD, and Cygwin have emulated TLS support for all supported architectures other than RISC-V?
Or are you essentially asserting, by making this contribution, that we should add additional opt-outs for other architectures?

In D142283#4092092, @dblaikie wrote:

Ah, so how does a debugger find debug info without these sort of hints? Could that logic be reused in llvm-symbolizer? (are there other symbolizers (addr2line, etc) that work on MacOS? How do they find the debug info?)

In D142282#4088945, @luismarques wrote:

I'll submit a separate patch to add a unit test for symbolized stack traces.

LGTM. It might have been needed in an older version of D62592.

In D142283#4091407, @dblaikie wrote:

Huh, this seems like an unfortunate workaround/thing to have to implement.

In D139996#4000438, @zixuan-wu wrote:

Nope, it's not enough. Before this patch, for example

Overall seems a reasonable implementation, despite the limitations pointed out in the patch description.

LGTM.

In D142282#4081511, @mehdi_amini wrote:

Is this something we can test for?

Improve LLVM directory search.

LGTM, with some in-line caveats.

Possibly we want to add this change to the release notes?

In D139823#4081239, @luismarques wrote:

The lsan tests fail when SANITIZER_CAN_USE_ALLOCATOR64 is defined as 1, both on rv64 sv39 and sv48 hosts.

@kito-cheng when you uploaded this patch did you run check-lsan and get different results? If so, do you have information about your test system and base LLVM commit?

The lsan tests fail when SANITIZER_CAN_USE_ALLOCATOR64 is defined as 1, both on rv64 sv39 and sv48 hosts.

In D142551#4080298, @luke wrote:

Good idea, I don't have access to a remote on it though. Would someone be able to post a branch with this patch on it?

In D142551#4080298, @luke wrote:

Good idea, I don't have access to a remote on it though. Would someone be able to post a branch with this patch on it?

It might be a good idea to see the compile-time impact of this with https://llvm-compile-time-tracker.com?

With the current (unpatched) LLVM tree on an RV64 SV39 host I get this ASAN test failure (for check-asan):

Makes sense to me and the results I'm seeing are reasonable. (this is also a weak approval)

I'm probably not the best person to review this but, for what it's worth, it LGTM.

Do we actually want to include the dso_local in the full signature?

LGTM but others should also chime in.

LGMT.

LGTM.

In D132994#3927328, @smd wrote:

I think one of possible solutions for the problem mentioned above might be the following:

• mark __hwasan_tag_mismatch_v2 as .variant_cc directly in asm file
• add support for reading/dumping .variant_cc as emitDirectiveVariantPCS does
• remove setting ELF::STO_RISCV_VARIANT_CC for __hwasan_tag_mismatch_v2 in RISCVAsmPrinter.cpp

Supposedly (2) should be implemented/committed as a separate patch before this one. It looks like it's being done as part of https://reviews.llvm.org/D103435
I might have my own patch for .variant_cc support. Does it make sense to publish it for the review, since the D103435 has been sitting there for over a year?

@jrtc27 @luismarques what do you think of that?

I was assuming this would never increase code size but that didn't seem to be the case. Since it wasn't clear to me why that would be, I reduced a problematic case and the conclusion is... it's because it's incomplete. The new register allocation sometimes ends up being unfavorable for the compressible instructions that are not yet handled (obvious in hindsight). I presume that once this code is complete that will either go away or be much more unlikely. Still, even as it is, the typical result is to improve code size so it's a nice improvement!

LGTM.

I see no reason not to accept this. I've checked that the tests pass.
LGTM.

As far as I can tell from looking at the relevant psABI discussions and GNU patches, this overall approach is OK.

I looked at this more carefully. I think the CHECKs for the .section were already not being updated automatically (not sure if that was the case when they were originally added), so you didn't change that (when I originally skimmed the patch I thought that was new). I'm not sure the two UTC_ARGS are actually needed.

Sorry, could you please explain why this required a manually written check? It's not obvious to me.

Is the tagging actually incompatible with the code model or only with the address materialization instructions that we use as part of the implementation of the code model? Does the tagging actually change the location of globals? I was assuming the location was the same, and the tag was just changing how we encoded that location in a numerical value. My understanding of the code model specifications is that they only care about the actual address ranges. Although the RISC-V psABI gives examples of instruction sequences my interpretation of that is that it's non-normative. So if the location doesn't actually change in principle you could materialize those with a different instruction sequence and still claim you comply with the code model, is that not the case? That only impacts the comments in the patch, I'm not opposed to the approach of using the GOT.

Regarding the overkill of "RISCVInstrInfoZawrs.td", how about having a "RISCVInstrInfoExtra.td" (or "RISCVInstrInfoExt.td") as a grab bag for everything that doesn't merit its own .td file?

LGTM.

LGTM. Thanks!

In D130560#3746379, @luismarques wrote:

The tests don't seem to have been properly updated with update_llc_test_checks.py. llvm/test/CodeGen/RISCV/branch-relaxation.ll contains RV64 RUN lines but the corresponding CHECK lines are missing in some functions.

The tests don't seem to have been properly updated with update_llc_test_checks.py. llvm/test/CodeGen/RISCV/branch-relaxation.ll contains RV64 RUN lines but the corresponding CHECK lines are missing in some functions.

Looks correct to me now. Let's wait a little bit to see if there's more feedback from others.