This patch inserts a percent symbol before the register names on PowerPC
for Linux. This makes easier to recognize the register types in the
assembly code. That improves the syntax of the code disassembled by
LLDB.
Tests were changed to ignore the percent prefixes.
Details
Diff Detail
- Build Status
Buildable 11378 Build 11378: arc lint + arc unit
Event Timeline
Just to show how much the disassembly is changed. Consider the following instruction:
li %r3,0
currently, it's disassembled by llvm like:
0x20000780 <+0>: li 3, 0
However this is what it looks like in gdb:
0x0000000020000780 <+0>: li r3,0
and in objdump -d:
780: 00 00 60 38 li r3,0
The code currently disassembled by llvm is not wrong! But it is surely less readable when not placing register type hints.
Now how this patch makes it looks like:
li[0x780] <+0>: li %r3, 0
PS: Both %r3 and 3 are valid 1st parameter for li instruction on PowerPC.
A few general comments:
- You should add reviewers to this review. At the minimum @hfinkel, @kbarton and @echristo. Maybe someone from FreeBSD if this affects them.
- Why do we need to change the default and get the existing behaviour with an option? Why not the other way around?
- Is this syntax accepted by all the assemblers (I think LLVM for PPC runs on FreeBSD as well, but I have no idea if FreeBSD returns true for isOSLinux()? We don't want to produce assembly that we can't assemble.
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp | ||
---|---|---|
45 | I'm not a fan of this name. We're not "ignoring" the percent sign here, we're just not printing it, right? | |
460 | Naming convention: variables start with capitals. | |
489 | With this, LLVM will show CR registers as %crN. I am not aware of any other tool that will produce this spelling. |
The patch is not acceptable in the current form. This includes fixing the memory leaks.
On PowerPC, three different output forms have to be supported:
(1) register name without percent on Darwin
(2) plain register number on AIX
(3) register name with percent OR plain register number on anything else
I don't think there is a big reason for supporting the second option of (3) though. The least amount of change is to introduce a predicate that checks for !Darwin && !AIX and then prints the % in the assembler printer. It is better to just pass down the target directly than deciding in the higher layers which forms should be choosen.
1, 3: FreeBSD should return true for isOSFreeBSD. This patch only affects the Linux behavior.
2: The default should be changed to allow LLDB prints the register prefixes. The code is still accepted by the assemblers in Linux.
It is not possible to set this option by LLDB.
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp | ||
---|---|---|
45 | That's right. Fixed variable name to StripRegisterPrefix. | |
460 | Fixed variable names. |
Fixed memory leak problem.
Changed code to handle target in the assembler printer, passing the target directly in the class constructor.
This is even worse. You can't new[] and then free(). Please follow the suggestion on just embedding the prefix directly, if desirable.
Is the suggestion to keep the default without the prefixes?
Because it is not possible to change the cl option by LLDB.
There should be no need to. This is not a public interface anyway. Long term, all sources should be changed to consistently use the human-friendly names, but that's a separate issue.
I am confused. Does the LLVM will be changed to support human-friendly names in the future by default?
Is there another way to change it now?
The LLVM interface could be modified to accept an option for printing the prefixes.
But It would require a lot of changes.
If what we want is to name the registers with the percent sign prefix on all non-AIX, non-Darwin systems, why not just change the definitions in the .td files to include the prefix? That way you don't have to worry about this kind of hacky/leaky code to prepend the symbol to the register names. And on AIX/Darwin, we just strip the full prefix (percent symbol and letter). Considering that it is questionable whether LLVM will ever have full support on AIX and that Darwin support is likely to be pulled soon, this seems like the best choice.
Of course, we'd then need to decide how we want to print the CR registers (i.e. if we want to have the ability to print them the way binutils tools print them - 4*cr4+gt, etc.)
Almost.
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp | ||
---|---|---|
489 | Should be a static bool showRegistersWithPercentPrefix(RegName(const char *RegName) member of PPCInstPrinter and just return true or false. | |
539 | ...and here if (showRegistersWithPercentPrefix(RegName) O << "%";. Rest of the RegName logic can remain. |
The prefixes are already being printed if they are not stripped in PPCInstPrinter.cpp.
And the default can be changed to include the percent without a option.
However, all the test files should be changed to consider different syntaxes for each OS.
What did you mean with "Long term"? I'm interested in having such a human-friendlier disassembler on LLDB and I noticed that this patch is not going to change that.
(I just want to know when this mode would reach LLDB.)
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp | ||
---|---|---|
531 | I said make it a member function because the knowledge that Darwin doesn't get the percent should be part of it, not external... |
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp | ||
---|---|---|
531 | If the OS is Darwin, the register name can't be stripped too. Or I can not strip the register prefix in stripRegisterPrefix if the OS is Darwin. What do you think it is better? |
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp | ||
---|---|---|
531 | That's why I suggested to keep things separate and simple. First just decide if the '%' prefix is desired; afterwards, decide whether the register name should be stripped. Keeping the decisions separate makes the flow easier. They will become less entangled in the future too. I haven't really made up my mind on whether supporting the AIX flavor long term is desirable, I let others shim in on that. But that's a separate follow-up. |
Hi,
Could someone review/approve this patch, please?
I don't have the permissions to commit it.
Thanks. ;)
I really appreciate you doing this work. This has been something I've wanted for some time :)
I've added a few things to this. Should also have a testcase as well.
Thanks!
-eric
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp | ||
---|---|---|
456 | Seems to be no reason to make it a member function? | |
472 | Ditto. | |
488 | Having this not be the direct inverse of the above is weird. Perhaps redo some of that logic? | |
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.h | ||
23 | "TT" is a standard Triple name. |
Split into verbose conditional register names into a separate function. We likely want to remove them going forward as they are a specific feature of the Darwin assembler and not wildly supported.
Create precise predicates for using % register names and stripped register names, decoupling the (incomplete) existing checks.
So the next steps if you have the time would IMO be:
(1) Convert the Darwinish verbose conditional codes into annotations:
bt 2, foo # condition: eq
or something like that. This removes some platform specific logic and can be applied unconditionally.
(2) Convert the existing test cases and drop the -ppc-reg-with-precent-prefix option. This is sadly mostly busy work unless you can script it.
"TT" is a standard Triple name.