Page MenuHomePhabricator

[AIX] Print printable byte list as quoted string
ClosedPublic

Authored by jsji on May 19 2021, 2:25 PM.

Details

Summary

.byte supports string, so if the whole byte list are printable,
we can actually print the string for readability and LIT tests maintainence.

.byte 'H,'e,'l,'l,'o,',,' ,'w,'o,'r,'l,'d

->

.byte "Hello, world"

Diff Detail

Event Timeline

jsji created this revision.May 19 2021, 2:25 PM
jsji requested review of this revision.May 19 2021, 2:25 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 19 2021, 2:25 PM
llvm/lib/MC/MCAsmStreamer.cpp
1007–1008

If going through the whole string, going through make_range is not needed.

1009

isPrint returns true for ", which does require escaping.

1048–1051

This breaks the abstraction. That a "byte-list" directive accepting a list of elements where there are single-quote-prefixed character literals is available does not mean that the same directive accepts a string argument.

Note: PrintQuotedString does print more general strings on AIX (that do not contain a newline); however, I do not believe it to be desirable to emit control characters and the such "raw".

1123–1126

Here may be a nice point to add a condition that hasPairedDoubleQuoteStringConstants means we check isPrintableString before deciding to use AIX .string for .asciz and AIX .byte for .ascii.

jsji updated this revision to Diff 346794.May 20 2021, 10:37 AM

Address comments.

jsji marked 4 inline comments as done.May 20 2021, 10:38 AM
jsji added inline comments.May 20 2021, 11:31 AM
llvm/lib/MC/MCAsmStreamer.cpp
1008

This still need some slight change to deal with the ending 0 so that we can handle .string.

jsji updated this revision to Diff 346830.May 20 2021, 12:51 PM

Update .string handling.

llvm/lib/MC/MCAsmStreamer.cpp
1008

Done.

LGTM with minor comments; thanks!

llvm/include/llvm/MC/MCAsmInfo.h
268–270 ↗(On Diff #346830)

See suggested edit. Emphasizing the difference from .ascii by starting with its associated comment is nice. Need to indicate that this is the zero-terminated case though.

llvm/lib/MC/MCAsmStreamer.cpp
1012–1014

Minor comment: Can just return the expression instead of using it for if/else.

1130

Minor nit: typo.

1141–1142

The style guide was clarified some time ago to indicate that mixing use/non-use of braces in an if/else chain is discouraged.

This revision is now accepted and ready to land.May 20 2021, 2:11 PM
jsji marked 4 inline comments as done.May 20 2021, 7:01 PM
jsji updated this revision to Diff 346907.May 20 2021, 7:12 PM

Address comments.

This revision was landed with ongoing or failed builds.May 20 2021, 7:38 PM
This revision was automatically updated to reflect the committed changes.