It’s not upstreamed yet, but there is a sub extensions with change-of-flow instructions, so we definitely need to enable Xqci in the lldb disassembler, for source stepping (and, oh yeah, the users want to see the correct disassembly).
llvm-objdump looks at the build attributes to turn this on, so I figure we can do the same. We do that for certain ARM things in ObjectFileELF::ParseARMAttributes, but that sets a flag in the ArchSpec. I don’t know if we want to set one of the 32 flags in the ArchSpec for some rando RISC-V extension. The other possibility I thought of is parsing the attributes note every time we do a disassemble, but that seems non-optimal.
Any thoughts on how we can save off “the Xqci extension is enabled”?
Can you go more into what these instructions are and show an example of what might be in the disassembly if this were to be implemented?
I know llvm-objdump and lldb are able to resolve addresses where you load base+offset, and show where the result points to. Is this the same sort of thing or more complex?
Because I don’t think for Arm we have to have 1 setting per extension that needs this, I could be wrong though.
RISC-V has the concept of extensions - some published by RVI, some published by vendors, some kept downstream by vendors. The Xqci extension is an integer extension that adds a bunch of instructions that do things like immediate loads/stores with wider immediate addresses, unconditional and conditional branches to wider address ranges, etc. These extensions are disabled by default in the disassembler, so we need to enable them.
So if Qualcomm has 4 extensions and Apple has 6 extensions and NVidia has 3 extensions, and so on, we’d fill up flags pretty quickly.
I want to have a way to turn on the extension upstream if the build attributes have it, without having to parse the build attributes every time the disassembler is instantiated.
llvm-objdump looks like this:
: file format elf32-littleriscv
lldb with the flag turned on manually looks like this:
(lldb) target create “xqcia.o”
Current executable set to ‘xqcia.o’ (riscv32).
(lldb) dis -a 0x4 -Y +experimental-xqcia
xqcia.o`$x:
xqcia.o[0x0] <+0>: qc.slasat a0, gp, a7
xqcia.o[0x4] <+4>: qc.sllsat s7, s9, s11
xqcia.o[0x8] <+8>: qc.addsat a7, a4, t2
xqcia.o[0xc] <+12>: qc.addusat s0, s2, t3
xqcia.o[0x10] <+16>: qc.subsat s6, sp, a2
xqcia.o[0x14] <+20>: qc.subusat s1, a4, a7
xqcia.o[0x18] <+24>: qc.wrap gp, t5, s7
xqcia.o[0x1c] <+28>: qc.wrapi t1, a2, 0x7ff
xqcia.o[0x20] <+32>: qc.norm gp, t2
xqcia.o[0x24] <+36>: qc.normu a1, a7
xqcia.o[0x28] <+40>: qc.normeu s10, t6
And lldb without the flag turned on looks like this:
(lldb) target create “xqcia.o”
Current executable set to ‘xqcia.o’ (riscv32).
(lldb) dis -a 0x4
xqcia.o`$x:
xqcia.o[0x0] <+0>:
xqcia.o[0x4] <+4>:
xqcia.o[0x8] <+8>:
xqcia.o[0xc] <+12>:
xqcia.o[0x10] <+16>:
xqcia.o[0x14] <+20>:
xqcia.o[0x18] <+24>:
xqcia.o[0x1c] <+28>:
xqcia.o[0x20] <+32>:
xqcia.o[0x24] <+36>:
xqcia.o[0x28] <+40>:
Sorry I thought you wanted to enable something beyond just disassembling the instructions, like resolving the targets of the jumps.
For AArch64 we have all possible extensions enabled for llvm-objdump and lldb, but perhaps we can do this because of the way the architecture is controlled.
I’ll ask the question though, what would happen if you enabled everything just for lldb? Overlapping encodings, as vendor extensions can use any of the unused values?
Surprised you didn’t get the raw hex codes here at least.
RISC-V vendor extension space is basically “do what you want in this encoding space”, so if we enable disassembly for a Qualcomm and an NVidia extension that use the same encodings for different things, we’ll get bad results.
Me too. The Hexagon disassembler will print something like “invalid opcode”. Seems like something that should be fixed in the RISC-V llvm disassembler.
Given the ISA string is encoded in the RISC-V ELF attributes section of the binary it should be possible for LLDB to infer them automatically if it doesn’t already, just as llvm-objdump can and does.
I’ve used -Y to manually set the correct features flag above, and it works, but I want something that gets done automatically. Are you suggesting I set target.disassembly-features in ObjectFileELF when the target is created? The API tests clear all settings, but I think that will happen before the target is created, so I guess that would work.
I can get the data from the attributes section, but I’m trying to figure out a good way to save this info and not have to parse it every time we instantiate the disassembler. llvm-objdump instantiates it once; lldb usually does it many times.
I think we need to add a third place to get features/cpu from – the ObjectFile. Jonas’ patch is a handy pointer to all the places that need to be considered.
I think when we have an address/address range that we know we are disassembling, we need to ask the Module’s ObjectFile if it has features/cpu for disassembly. And if the user specifies features/cpu via a settings, that should override (my initial impression) – the user should always be able to say “no, this is the correct thing”.
I only skimmed Jonas’ patch quickly, but my concern would be cases where we might be creating a Disassembler without consideration to a specific address range that we are disassembling, I don’t know what the right answer is there. When we actually ask to disassemble concrete instructions, if it’s in a Module+ObjectFile see if there’s a feature/cpu and if none was specified in the creation of that Disassembler, set it?
I would say “the Target should own this knowledge” but you might have multiple Modules, some using a basic set of ISA extensions and then one Module is loaded conditionally with a special extension. So Target would have to go over its ImageList looking for a “best” cpu/feature list? I don’t see how that works well.
I was thinking last night about the current implementation of -Y. It replaces all features with what the user specifies. And sometimes that’s what you want. But in my case with RISC-V, adding in +experimental-Xqcia removed the auto calculated +a,+m,+c, which made certain common instructions not be disassembled.
Perhaps we have another setting, that can be changed by ObjectFileELF when the Module is loaded. It could be called target.additional-disassembly-features, and the automatic code would append to it. When the disassembler is instantiated, it goes through what it does today to build the features string, then adds the additional features. @JDevlieghere ?
I vaguely remember thinking about whether the setting should override or append. For RISC-V, appending seems more appropriate. For AArch64, where we pass +all by default, overriding made more sense. I definitely don’t want to bifurcate the behavior between architectures so I think having an additional is probably the best solution with the most control.
PS: If you look at my patch, it touches a lot of code to thread the features argument through. In hindsight, I should have anticipated that we might need to pass more information and created a struct to wrap the flavor, the cpu, and the features. If we add the option, I would recommend doing that first in an NFC commit and then adding the field there instead of adding another argument to all the functions. Given I’m partially to blame for that I’m happy to volunteer to do the former.
I was thinking we could handle the features string in DisassemblerLLVMC, appending the new setting to the features before instantiating the disassembler. The new setting would be empty by default, so no change in current behavior.
Whatever you design you come up with, please also be mindful of mapping symbols, which locally change the ISA string for disassembly. It would be good to ensure those are supported too, rather than putting effort into making it work at the object file level only to discover it’s not suitable for supporting mapping symbols.
It’s my intention that current functionality remain. This would initially be done for RISC-V extensions (specifically Qualcomm’s Xqci, but could be extended to other extensions). I don’t expect that it would affect mapping symbols.
I’m planning on doing this for Hexagon as well, and foresee others doing it for their cores when needed.