This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ][z/OS] Localize runtime error messages in libc++
Needs ReviewPublic

Authored by muiez on Jan 25 2022, 12:44 PM.

Details

Reviewers
zibi
SeanP
ldionne
fanbo-meng
Group Reviewers
Restricted Project
Restricted Project
Summary

The aim of this patch is to localize the string literals in order to carry out translations based on the char-mode. This is a continuation to add ASCII/EBCDIC support for z/OS. This could also be the ground work for enabling internationalization of the runtime messages.

Diff Detail

Event Timeline

muiez requested review of this revision.Jan 25 2022, 12:44 PM
muiez created this revision.
muiez set the repository for this revision to rG LLVM Github Monorepo.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 25 2022, 12:44 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

@libc++ Can I get a review please? Thanks

muiez updated this revision to Diff 408575.Feb 14 2022, 12:48 PM

resolve lit failure

muiez updated this revision to Diff 408867.Feb 15 2022, 7:27 AM

kickoff CI

muiez updated this revision to Diff 410572.Feb 22 2022, 9:47 AM
muiez set the repository for this revision to rG LLVM Github Monorepo.

Resolve merge conflicts

zibi added a comment.Feb 22 2022, 5:05 PM

LGTM with minor suggestions.

libcxxabi/src/abort_message.cpp
8

I might be wrong but I think the use defined headers should be included after the system ones. Can you keep it the original order?

libcxxabi/src/abort_message.h
12

This will make consistent by including this header as user define one and move it down after `cxxabi.h as being done in other locations.

libcxxabi/src/cxa_guard_impl.h
48

same as above

libcxxabi/src/cxa_virtual.cpp
9

same as above

muiez updated this revision to Diff 410854.Feb 23 2022, 9:57 AM

improve include's

muiez marked 4 inline comments as done.Feb 23 2022, 10:02 AM
muiez added inline comments.
libcxxabi/src/abort_message.cpp
8

The "abort_message.h" header should be included before the others in this case because we refer to bimodal functions (eg __fprintf_a). In particular, abort_message.h includes error_message.h which includes the Bimodal header. This should be included before the other headers (such as stdio.h) to avoid an undeclared identifier error.

SeanP added inline comments.Feb 23 2022, 11:33 AM
libcxxabi/src/abort_message.cpp
45–50

I suggest making a macro to wrap the function fprintf. That would simplify the this code. For example:

#ifdef __MVS__
  #define fprintf(...) \
    if (__isASCII()) __fprintf_a(__VA_ARGS__); \
    else __fprintf_e(__VA_ARGS__)
#endif

Do the same for vfprintf() and we can remove the duplication in this code. Same for the snprintf system_error.cpp. Just confirm fprintf, etc aren't macros already on MVS.

muiez updated this revision to Diff 412219.Mar 1 2022, 1:22 PM
muiez marked an inline comment as done.
muiez set the repository for this revision to rG LLVM Github Monorepo.

Refactor with macros

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 1:22 PM
muiez marked an inline comment as done.Mar 1 2022, 1:22 PM

Ping :) @libcxx @libcxxabi

muiez updated this revision to Diff 439056.Jun 22 2022, 9:26 AM
muiez removed a reviewer: Quuxplusone.

Fix merge conflict.

muiez updated this revision to Diff 439388.Jun 23 2022, 7:22 AM
muiez set the repository for this revision to rG LLVM Github Monorepo.

Correct conflict resolution

muiez updated this revision to Diff 439523.Jun 23 2022, 1:47 PM

Fix warning

muiez added a comment.Jun 27 2022, 1:33 PM

The CI is failing with the following warning (shown as an error):

/home/libcxx-builder/.buildkite-agent/builds/63ef6a49a323-1/llvm-project/libcxx-ci/libcxx/src/system_error.cpp:134:44: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
      snprintf(buffer, strerror_buff_size, getRuntimeErrorString(RM_UNKNOWN_ERROR), ev);

Any suggestions on getting passed this warning? It should be noted that getRuntimeErrorString(RM_UNKNOWN_ERROR) returns "Unknown error %d", which includes a format specifier so we can't make the call with "%s" as a string literal.