Page MenuHomePhabricator

[SystemZ][z/OS] ASCII/EBCDIC support with no coexistence
ClosedPublic

Authored by muiez on Nov 30 2021, 12:34 PM.

Details

Reviewers
ldionne
Quuxplusone
SeanP
zibi
Group Reviewers
Restricted Project
Restricted Project
Commits
rGa1da73961d29: [SystemZ][z/OS] ASCII/EBCDIC support with no coexistence
Summary

The aim of this patch is to break up the larger patch (https://reviews.llvm.org/D111323) to be more upstream friendly. In particular, this patch adds the char encoding sensitive changes but does not use inline namespaces as before. The use of namespaces to build both versions of the library, and localization of error messages will follow in a subsequent patch.

Diff Detail

Event Timeline

muiez created this revision.Nov 30 2021, 12:34 PM
muiez requested review of this revision.Nov 30 2021, 12:34 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 30 2021, 12:34 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
muiez updated this revision to Diff 391379.Dec 2 2021, 10:18 AM
muiez edited the summary of this revision. (Show Details)

Remove LIBCXXABI_BUILD_ZOS_ENCODING

muiez updated this revision to Diff 396162.Dec 24 2021, 7:41 AM
muiez edited the summary of this revision. (Show Details)

Remove LIBCXX_BUILD_ZOS_ENCODING

zibi added a comment.Dec 24 2021, 9:18 AM

We need to pass "-fzos-le-char-mode=ebcdic for libcxx same as we do for libcxxabi

libcxx/CMakeLists.txt
470

add_target_flags_if_supported("-fzos-le-char-mode=ebcdic")

muiez updated this revision to Diff 396528.Wed, Dec 29, 7:03 AM

Add default encoding

muiez added a comment.Tue, Jan 11, 1:45 PM

ping @libc++ @libc++ab

zibi accepted this revision.Wed, Jan 12, 12:34 PM

LGTM

ldionne requested changes to this revision.Wed, Jan 12, 1:19 PM

A couple of minor comments -- I don't see anything wrong specifically with this patch, but I'm eager to see the follow-up patches.

libcxx/include/regex
1314–1322

Those should be inline functions instead of macros.

libcxxabi/CMakeLists.txt
270

What does this flag do? Isn't it necessary only when building using EBCDIC?

This revision now requires changes to proceed.Wed, Jan 12, 1:19 PM
muiez updated this revision to Diff 399779.Thu, Jan 13, 1:48 PM

Using inline functions instead of macros

muiez marked 3 inline comments as done.Thu, Jan 13, 1:49 PM
muiez added inline comments.
libcxxabi/CMakeLists.txt
270

This flag ensures that our default encoding is ebcdic. In a subsequent patch, we build the ascii version as a separate library and override this flag.

ldionne requested changes to this revision.Fri, Jan 14, 7:48 AM
ldionne added inline comments.
libcxx/include/regex
1314

If you look at the surrounding code, we uglify everything with double underscores. The reason is that names starting with double underscores are reserved for the implementation, so users wouldn't be allowed to define a macro like is_07 that would (in the status quo) break our implementation. You'll need to do that here.

libcxxabi/CMakeLists.txt
270

I think this should be enclosed in if (MVS) or whatever the CMake check is, then. Otherwise, if another compiler supports that flag (I assume Clang will support that flag at some point?), we'd start using it everywhere.

This revision now requires changes to proceed.Fri, Jan 14, 7:48 AM
muiez updated this revision to Diff 400009.Fri, Jan 14, 7:59 AM
muiez marked an inline comment as done.

safeguard to z/OS, use double underscore function names

muiez marked 2 inline comments as done.Fri, Jan 14, 8:01 AM

Great points! Updated to account for this.

ldionne accepted this revision.Fri, Jan 14, 8:03 AM

LGTM with my __to_lower suggestion.

libcxx/include/regex
1357

Can we change this into:

__ch = __to_lower(__ch_);

Sorry I didn't spot it before. As it stands, it really looks like a bug until you realize __ch is passed by reference.

This revision is now accepted and ready to land.Fri, Jan 14, 8:03 AM
muiez updated this revision to Diff 400020.Fri, Jan 14, 8:19 AM

addressed comment to be more readable

muiez marked an inline comment as done.Fri, Jan 14, 8:19 AM
This revision was landed with ongoing or failed builds.Fri, Jan 14, 8:37 AM
This revision was automatically updated to reflect the committed changes.