[RFC] Contributing platform support for Linux on SystemZ

Hello,

I’ve been working on adding support for Linux on SystemZ as a target (and host) platform for LLDB, and now have a patch set ready that I’d like to submit for upstream inclusion.

For reference, I’ve attached the patch set as a quilt stack.
(See attached file: patches-lldb-s390x.tar.bz2)
I’d be happy to split it out into one email per patch or use Phabricator, whatever you prefer. I’d also appreciate suggestions for potential reviewers I could list on Phabricator revisions for each of the affected areas. This is my first contribution to LLDB, and I’m not really familiar with the community yet.

There’s a number of separate changes implemented by the patch set. First of all, I ran into three problems that seem to be platform-independent, but are related to version and implementation details of the linker and libc library in use on the system, which is why they may not show up everywhere (but they certainly caused many test case failures on my system). These are:

  • LLDB was unable to find the .plt section and therefore did not identify PLT call stubs (see diff-pltsection)
  • LLDB would sometimes not find DWARF CFI entries if the PC points one beyond the end of a function (see diff-tailcall)
  • LLDB would fail expression evaluation if clang asks for an identifier that exists both as type name and variable name (see diff-typelookup)

Next, I had to add one new member routine to the ABI class. This was necessary since on SystemZ, the CFA does not equal the incoming stack pointer value (like LLDB unwind code assumes), but instead is offset by a constant, i.e. CFA = incoming SP + 160. The new ABI member CFAOffset allows a platform to provide this offset of the CFA relative to the SP to common unwind code (see diff-cfaoffset).

The patch diff-s390x then adds all the new back-end code to support Linux on SystemZ. Its only strict pre-requisite is diff-cfaoffset. Changes includes in this patch are:

  • A new ArchSpec value of eCore_s390x_generic
  • A new directory Plugins/ABI/SysV-s390x providing an ABI implementation
  • Register context support
  • Native Linux support including watchpoint support
  • ELF core file support
  • Misc. support throughout the code base (e.g. breakpoint opcodes)
  • Test case updates to support the platform

This should provide complete support for debugging the base SystemZ platform. Not yet supported are optional features like transaction support (zEC12) or SIMD vector support (z13). Note that there is no instruction emulation, since our ABI requires that all code provides correct DWARF CFI in .eh_frame to support unwinding.

However, after that there are still many failing test cases due to various bugs in common code. The patch diff-signedchar fixes a number of problems caused by LLDB assuming a plain “char” type is signed; this is actually platform-specific, and on SystemZ, char is in fact unsigned.

The remaining patches fix many problems throughout the code base affecting big-endian platforms (like SystemZ). The biggest problem was that pretty much every access to APInt.getRawData (mostly via the Scalar class, but in a couple of other places as well) was incorrect on big-endian systems. This is fixed in diff-endian-scalar, which requires diff-getbytes-const as pre-requisite, a patch that changes RegisterValue so as to allow Scalar.getBytes to be made const. Other endian related issues were:

  • Bit field numbering didn’t work correctly (see diff-endian-bitfield)
  • Some smaller miscellaneous issues (see diff-endian-misc)
  • ARM instruction emulation tests wouldn’t work on big-endian host systems (see diff-endian-armemu)
  • Misc. test cases made byte order assumptions (see diff-endian-tests)

With the full patch stack applied on a current LLDB code base (rev. 265656), I’m getting a clean test suite run on the platform:

patches-lldb-s390x.tar.bz2 (37.8 KB)

Hello Ulrich,

It's exciting to see support for new platforms being added to LLDB.
I've had a brief glance at the patch set and I was surprised at how
few changes you had actually needed to make to lldb core to support
this. The patches look good at a first glance, but they will need to
be reviewed by respective code owners.

Going forward, the best way to handle this would be to create a
phabricator account and submit the patches there. This will make the
review much easier. For reviewers, you can use the CODE_OWNERS.txt
file (or just git history). If you don't know the right person, just
pick Greg Clayton for general stuff and me or Tamas Berghammer for
Linux-specific code, and we will route it further if needed.

Also, please run clang-format on your patches before submission. Let
me know if you need any help setting that up.

This should provide complete support for debugging the base SystemZ
platform. Not yet supported are optional features like transaction support
(zEC12) or SIMD vector support (z13). Note that there is no instruction
emulation, since our ABI requires that all code provides correct DWARF CFI
in .eh_frame to support unwinding.

Is the CFI info correct at all PC locations, or only at call sites
(synchronous vs. asynchronous exceptions). If it's the latter, you may
still need to do some instruction emulation to "augment" the CFI info
for it to be usable at non-call-sites. However, that's definitely not
a blocker now...

Looking forward to working with you on getting SystemZ support upstream! I'd
also be happy to take on ongoing maintenance (setting up a build bot, fixing
issues as they come up, etc ...).

Setting up a buildbot is definitely recommended and it will make
further significantly easier.

cheers,
pl

Hello Pavel,

> It's exciting to see support for new platforms being added to LLDB.
> I've had a brief glance at the patch set and I was surprised at how
> few changes you had actually needed to make to lldb core to support
> this. The patches look good at a first glance, but they will need to
> be reviewed by respective code owners.
>
> Going forward, the best way to handle this would be to create a
> phabricator account and submit the patches there. This will make the
> review much easier. For reviewers, you can use the CODE_OWNERS.txt
> file (or just git history). If you don't know the right person, just
> pick Greg Clayton for general stuff and me or Tamas Berghammer for
> Linux-specific code, and we will route it further if needed.

OK, sure, I will do that. Thanks for the pointers!

> Also, please run clang-format on your patches before submission. Let
> me know if you need any help setting that up.

Well, in some cases existing / surrounding code already differs from
the format enforced by clang-format (e.g. space vs. no space before
the '(' of a function call). I've now used clang-format on all the
new files, and most other changes, except where the style would
directly contradict surrounding code.

> > This should provide complete support for debugging the base SystemZ
> > platform. Not yet supported are optional features like transaction support
> > (zEC12) or SIMD vector support (z13). Note that there is no instruction
> > emulation, since our ABI requires that all code provides correct DWARF CFI
> > in .eh_frame to support unwinding.
>
> Is the CFI info correct at all PC locations, or only at call sites
> (synchronous vs. asynchronous exceptions). If it's the latter, you may
> still need to do some instruction emulation to "augment" the CFI info
> for it to be usable at non-call-sites. However, that's definitely not
> a blocker now...

It should be correct at all PC locations (-fasynchronous-unwind-tables).

> > Looking forward to working with you on getting SystemZ support upstream! I'd
> > also be happy to take on ongoing maintenance (setting up a build bot, fixing
> > issues as they come up, etc ...).
>
> Setting up a buildbot is definitely recommended and it will make
> further significantly easier.

I'll look into setting this up once the code is in.

Thanks for your comments!

Bye,
Ulrich