Page MenuHomePhabricator

Add Percent Symbol In PPC Registers for Linux
ClosedPublic

Authored by joerg on Oct 17 2017, 11:56 AM.

Details

Summary

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.

Diff Detail

Event Timeline

alexandreyy created this revision.Oct 17 2017, 11:56 AM
gut added a comment.Oct 18 2017, 4:44 AM

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:

  1. You should add reviewers to this review. At the minimum @hfinkel, @kbarton and @echristo. Maybe someone from FreeBSD if this affects them.
  2. Why do we need to change the default and get the existing behaviour with an option? Why not the other way around?
  3. 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?

463

Naming convention: variables start with capitals.

520

With this, LLVM will show CR registers as %crN. I am not aware of any other tool that will produce this spelling.

joerg requested changes to this revision.Oct 20 2017, 5:51 AM
joerg added a subscriber: joerg.

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.

This revision now requires changes to proceed.Oct 20 2017, 5:51 AM
alexandreyy edited edge metadata.

Update according to review comments

alexandreyy marked 2 inline comments as done.Oct 23 2017, 8:13 AM

A few general comments:

  1. You should add reviewers to this review. At the minimum @hfinkel, @kbarton and @echristo. Maybe someone from FreeBSD if this affects them.
  2. Why do we need to change the default and get the existing behaviour with an option? Why not the other way around?
  3. 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.

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.

463

Fixed variable names.

alexandreyy marked 2 inline comments as done.Oct 23 2017, 8:17 AM

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.

Fixed memory leak problem.
Changed code to handle target in the assembler printer, passing the target directly in the class constructor.

joerg added a comment.Oct 23 2017, 8:51 AM

This is even worse. You can't new[] and then free(). Please follow the suggestion on just embedding the prefix directly, if desirable.

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.

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.

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.)

Update according to review comments

joerg added a comment.Oct 24 2017, 6:58 AM

Almost.

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp
520

Should be a static bool showRegistersWithPercentPrefix(RegName(const char *RegName) member of PPCInstPrinter and just return true or false.

552

...and here if (showRegistersWithPercentPrefix(RegName) O << "%";. Rest of the RegName logic can remain.

Changed printRegisterWithPercentPrefix to showRegistersWithPercentPrefix

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.)

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.

gut added a comment.Oct 24 2017, 10:18 AM

Long term, all sources should be changed to consistently use the human-friendly names, but that's a separate issue.

Macro tell_me_more:

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.)

Hi.
Are these changes ok?

joerg added inline comments.Oct 27 2017, 9:15 AM
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp
544

I said make it a member function because the knowledge that Darwin doesn't get the percent should be part of it, not external...

alexandreyy added inline comments.Oct 27 2017, 12:18 PM
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp
544

If the OS is Darwin, the register name can't be stripped too.
So, We need to check the OS in the external function printOperand.

Or I can not strip the register prefix in stripRegisterPrefix if the OS is Darwin.

What do you think it is better?

joerg added inline comments.Oct 27 2017, 2:36 PM
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp
544

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.

Update patch according to suggestions

Are these changes ok?

Hi, @joerg
Could you review/approve these changes?

Hi,
Could someone review/approve this patch, please?
I don't have the permissions to commit it.
Thanks. ;)

echristo requested changes to this revision.Nov 9 2017, 10:50 PM

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
459–460

Seems to be no reason to make it a member function?

475

Ditto.

519

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–29

"TT" is a standard Triple name.

This revision now requires changes to proceed.Nov 9 2017, 10:50 PM
alexandreyy edited edge metadata.

Updated patch according to review.

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

Thanks @echristo.
I modified the patch according to your comments.

Fixed identation.

alexandreyy marked 3 inline comments as done.Nov 10 2017, 11:15 AM
alexandreyy added inline comments.
lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp
459–460

Removed from class.

519

Better with that logic.
But it does not change the output printed.
The percent is optional, so it is inserted if the flag FullRegNamesWithPercent is enabled.

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.h
23–29

Changed to TT

alexandreyy marked an inline comment as done.Nov 14 2017, 10:49 AM

Hi, guys!
Are these changes Ok to commit?

@joerg, @echristo, Could you check if these changes are Ok?
Thanks!

@joerg, @echristo, Could you check if these changes are Ok?
Thanks!

ping

joerg commandeered this revision.Nov 21 2017, 2:10 PM
joerg edited reviewers, added: alexandreyy; removed: joerg.
joerg updated this revision to Diff 123850.Nov 21 2017, 2:13 PM

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.

alexandreyy accepted this revision.Nov 22 2017, 3:30 AM

Thanks, @joerg !
It looks better than before.

Hi, @echristo @joerg

Could we commit this patch?

This revision was automatically updated to reflect the committed changes.
joerg added a comment.Nov 29 2017, 3:10 PM

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.