This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ/z/OS] Implement GOFF writer for empty files
ClosedPublic

Authored by Kai on Oct 8 2021, 9:25 AM.

Details

Summary

Adds the infrastructure to write GOFF object files.
Also adds a GOFF writer which writes only HDR/END records.

Diff Detail

Event Timeline

Kai created this revision.Oct 8 2021, 9:25 AM
Kai requested review of this revision.Oct 8 2021, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2021, 9:25 AM

The SystemZ back-end changes LGTM.

kpn added a subscriber: kpn.Oct 11 2021, 12:55 PM
kpn added inline comments.Oct 11 2021, 1:04 PM
llvm/lib/MC/GOFFObjectWriter.cpp
274

I have found it useful to use the Unix 'dd' command to slide up GOFF files when they cause the Binder to abend. This becomes much harder when this record count field is used. I'm not sure what the upside of this field is.

Kai added inline comments.Oct 12 2021, 1:17 PM
llvm/lib/MC/GOFFObjectWriter.cpp
274

I need to check if the binder is happy without this field.
Both xlc and xlcang set the record count, so setting the field is consistent with these compilers.

kpn added inline comments.Oct 13 2021, 6:52 AM
llvm/lib/MC/GOFFObjectWriter.cpp
274

We've been putting zero for the record count for nearly 20 years. It's been fine all that time. If it stopped working I'd get a phone call pretty quickly. Last night's builds on V2R4 ran fine, for example.

Kai updated this revision to Diff 379519.Oct 13 2021, 1:43 PM
  • Removed writing logical record count in END record
  • Fixed formatting
Kai added inline comments.Oct 13 2021, 1:44 PM
llvm/lib/MC/GOFFObjectWriter.cpp
274

Updated code to just write out the number of logical records for debug purposes.

kpn added inline comments.Oct 25 2021, 10:30 AM
llvm/lib/MC/GOFFObjectWriter.cpp
274

Thank you!

Kai added a comment.Nov 1 2021, 7:17 AM

More comments about the implementation?
E.g. is the name flags_t acceptable?

Kai updated this revision to Diff 386006.Nov 9 2021, 4:14 PM

Fix path of moved include.

Kai updated this revision to Diff 386171.Nov 10 2021, 7:59 AM

Update formatting.

RKSimon resigned from this revision.Dec 1 2021, 3:33 AM

Sorry this isn't an area I know much about.

Kai updated this revision to Diff 407148.Feb 9 2022, 6:54 AM

Rebase on latest main branch.

kpn added a comment.Feb 10 2022, 1:08 PM

More comments about the implementation?
E.g. is the name flags_t acceptable?

I wouldn't use `flags_t'. Isn't that namespace reserved by the standard?

It would be helpful if you used the 'git clang-format' command to reformat _only_ the parts that you are changing. There seem to be a number of places that aren't GOFF but have changed formatting. See 'clang/tools/clang-format'.

I have no experience in the MC layer so there's not much I can say about those connecting bits.

Kai added a comment.Feb 11 2022, 11:55 AM

I wouldn't use `flags_t'. Isn't that namespace reserved by the standard?

My rational is that I mimic a standard type like uint64_t. Well, may be better to change, to avoid fruitless discussions.

It would be helpful if you used the 'git clang-format' command to reformat _only_ the parts that you are changing. There seem to be a number of places that aren't GOFF but have changed formatting. See 'clang/tools/clang-format'.

Thanks! I do not note that, even if it so obvious. I change it.

Kai updated this revision to Diff 409029.Feb 15 2022, 1:08 PM
  • Renamed flags_t to Flags
  • Minimized formatting changes
kpn accepted this revision.Feb 16 2022, 10:48 AM

LGTM.

Please wait a couple of days in case there are any comments from anyone else.

This revision is now accepted and ready to land.Feb 16 2022, 10:48 AM
Kai added a comment.Feb 16 2022, 2:07 PM

Please wait a couple of days in case there are any comments from anyone else.

Sure, no problem. Thanks!

Basically stylistic-style comments and questions, as I don't know GOFF at all.

llvm/include/llvm/MC/MCGOFFObjectWriter.h
35
llvm/lib/MC/GOFFObjectWriter.cpp
69
75

My gut says we should be following standard LLVM naming scheme with this class name. The raw_ostream class uses the different style because it's intended to mirror the standard std::ostream class, but I don't think that's a justification to extend it to classes that make use of the stream.

93

"begin" is a verb. You want "start" or "beginning".

114

Similar to my above comment, unless the function is an implementation of a virtual function in the basis class, I'd use standard LLVM lowerCameCase for the methods throughout this class.

139
142

I think the general preference within LLVM is to use C++ static_cast and reinterpret_cast as appropriate, rather than the C-style cast.

177

I don't think you need the NDEBUG check here? Isn't that within the assert macro?

217–219

No braces for single line if.

252

clang-format understands this style for labelling arguments.

272
274

It might be worth a comment explaining why this field is always 0. Otherwirse a future contributor may not be aware of the reasoning and change it to be "correct".

llvm/lib/MC/MCGOFFStreamer.cpp
29–30

This seems like a memory leak?

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZELFObjectWriter.cpp
56

Whilst you're moving this file, I'd consider doing a complete clang-format of it too.

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCAsmBackend.cpp
45–46

clang-format the modified code.

llvm/test/MC/GOFF/empty-goff.s
2

I'd find this syntax easier to follow, as it's a more common style among tests I've worked with.

3

Why do you need the --ignore-case option?

Kai added inline comments.Feb 22 2022, 2:03 PM
llvm/lib/MC/MCGOFFStreamer.cpp
29–30

Spontaneously I cannot say when the object will be released again. The code corresponds to all other Streamer implementations, e.g. MCELFStreamer.

IMHO it would be better to return a std::unique_ptr<MCStreamer>, but this also required global changes in the way how the object retrieved from the TargetRegistry is handled.

llvm/test/MC/GOFF/empty-goff.s
3

The case of hexadecimal digits is not defined by the POSIX standard.
Consequently, Linux uses upper case digits, and the z/OS USS implementation uses lower case letters.

Kai updated this revision to Diff 410643.Feb 22 2022, 2:06 PM

Addressed review comments.

Kai marked 6 inline comments as done.Feb 22 2022, 2:15 PM

Thanks for the review!

llvm/lib/MC/GOFFObjectWriter.cpp
93

Sorry, my fault. In German, both words can be used as verb or noun.

177

Yes, agree, the #ifndef is not necessary.

Kai updated this revision to Diff 410645.Feb 22 2022, 2:16 PM
Kai marked an inline comment as done.

Removed unnecessary #ifndef.

Kai marked 5 inline comments as done.Feb 22 2022, 2:18 PM
jhenderson added inline comments.Feb 23 2022, 1:01 AM
llvm/lib/MC/GOFFObjectWriter.cpp
83

It might be worth explaining why this is in an ifdef, in this comment.

140

"BE" being an acronym for "big endian" should be capitalized.

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZELFObjectWriter.cpp
56

Not addressed?

llvm/test/MC/GOFF/empty-goff.s
2

Not addressed?

5–9

For true comments, as opposed to lit or FileCheck directives, I like to use double comment markers, i.e. "** Header record:" etc. Also why "*"? "#" is the more common marker I've seen used (although I admit it doesn't apply everywhere).

Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2022, 7:58 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
Kai updated this revision to Diff 557482.Sep 29 2023, 7:45 AM

Rebase on latest master.

This revision was landed with ongoing or failed builds.Sep 29 2023, 12:27 PM
This revision was automatically updated to reflect the committed changes.