One more Windows Unicode PR: do not use %S by nojb · Pull Request #1398 · ocaml/ocaml

nojb · 2017-10-04T21:47:12Z

This PR addresses the following small issue left over from #1200. In that PR there are a handful of uses of the printf format specifier %S to print wide strings (wchar_t *), mostly in calls to caml_gc_message to print filename arguments (see e.g. here and here).

The specifier %S is both POSIX and conforms to the Unix specification. Unfortunately it is effectively broken under Windows (in spite of "supporting" it). The details are a little long but see this post for some good pointers.

Hence we would like to get rid of the %S specifiers. The only issue is that if we convert the wide strings into usual strings with our existing functions then we must not forget to free them, etc. Also it would be nicer not to allocate when printing GC debug messages.

This PR defines a function win_utf8_string_of_utf16 in win32.c which converts as needed but uses a fixed-length statically-allocated buffer to store the resulting UTF-8 string. This is not an issue because in each use of the function the result of doing the conversion does not need to be stored anywhere (and possible truncation is OK).

We also use this function for some arguments to caml_fatal_error_arg which were being "leaked" (just before the program ends).

Together with this change, we also add a call to SetConsoleOutputCP in the different caml_{main,startup*} functions so that, under Windows with Unicode mode enabled, the console code page is set to UTF-8, saving the user from having to do it. This helps to print things correctly on the Windows Console (e.g. if you run your programs from the Command Prompt).

Lastly, and for the record, as far as I know, the most "native" solution to these printing issues would be to use the Windows call WriteConsole (like we do in headernt.c) to print wide strings, which would not require us to do any conversion. However, it is not straightforward to integrate this function in our printf-based functions caml_gc_message, caml_fatal_error_arg, and caml_fatal_error_arg2.

@dra27

gasche

If the function may truncate its input (and has a special memory-ownship behavior), it should be clear in the name. Currently the name suggests a normal conversion function, that (1) creates a new caller-owned value and (2) is injective.

I'm not fully convinced that this solution is an improvement over the direct style of having a conversion call before the print and a free call after the print. This is a burden at the callsite, but it is a normal kind of burden for C programmers, whereas you alleviate the issue by introducing a convenient but unusual way to shoot oneself in the foot (if someday the usage is extended to a case where truncation is problematic; and truncation has proved problematic in other places of the runtime before, eg. executable_name).

One aspect of the explicit-free solution that is unsatisfying is that error functions (caml_fatal_error_arg) never return and would thus never freed the copied strings (which is inconvenient if you look for leaks using valgrind, etc.). But since bf70eb9 we explicitly track caml_stat_alloc memory, so using a static allocation should not be an issue there.

gasche · 2017-10-05T07:02:23Z

byterun/win32.c

+
+CAMLexport char * win_utf8_string_of_utf16(const wchar_t * s)
+{
+  static char buf[1024];


Is this use of global state correct in a multicore runtime? (Would the static buffer be copied by each runtime instance, which is fine, or is there a risk of race to a single buffer?)

This was an alarm bell for me - I think @stedolan will confirm that we should be avoiding adding static buffers unless absolutely necessary.

Well spotted, and yes, this would be bad in multicore. (I have some hacky scripts to detect such buffers, but they only work on ELF binaries. I'm not sure how to prevent them sneaking into windows-only codepaths, other than by vigilance)

gasche · 2017-10-05T07:02:54Z

byterun/caml/osdeps.h

+   The returned string points to a fixed-length, statically-allocated buffer so
+   make sure to copy it if it needs to be preserved.  Also, due to the fixed
+   length of the buffer, the result may be truncated. */
+extern char * win_utf8_string_of_utf16(const wchar_t *s);


The function name in the documentation comment does not match the exposed name.

The function no longer matches the semantics of the comment, either!

nojb · 2017-10-05T08:03:41Z

Thanks for the review @gasche! I agree that my approach may be more complex/error-prone than needed. I will amend by explicitly allocating and freeing the converted string at the caml_gc_message callsites (and do not worry about freeing in the caml_fatal_error_arg calls).

dra27 · 2017-10-05T08:04:50Z

Just about to review this too - @gasche, please could we hold off beta2 until a decision is taken on this one. The code page setting part of this GPR I would very much like to be in (I'm preparing a larger post on the Unicode alterations for Windows)

gasche · 2017-10-05T08:20:58Z

please could we hold off beta2 until a decision is taken on this one

sure

(I have to discuss it with Damien first but I'm thinking of trying a "beta early, beta often" approach this time, which means that I could not wait and just do another beta when you ask for one.)

alainfrisch · 2017-10-05T08:29:49Z

What's the current behavior and how bad it is? If it's only about displaying garbage for non-ASCII characters in filenames in case of runtime debug messages or fatal errors, this does not sound a critical bug, and it might be better to avoid any risk of regression or rushed solution for 4.06.

dra27

The previous code you showed me had some changes in the debugger - have you intentionally reverted those?

dra27 · 2017-10-05T08:27:03Z

asmrun/startup.c

    return Val_unit;

+#if defined(_WIN32) && WINDOWS_UNICODE
+  SetConsoleOutputCP(CP_UTF8);


It would be worth a comment noting that this affects all output functions (i.e. both stdout and stderr), I think.

Two possible ideas:

Selecting CP_UTF8 only works if a TrueType font is selected (legacy Raster Fonts won't work, which is still the default case pre Windows 8, IIRC). You can detect this using this function

int console_supports_unicode (void) { CONSOLE_FONT_INFOEX fontInfo; fontInfo.cbSize = sizeof(fontInfo); if (GetCurrentConsoleFontEx(GetStdHandle(STD_OUTPUT_HANDLE), FALSE, &fontInfo) && !(fontInfo.FontFamily & TMPF_TRUETYPE)) { return (GetCurrentConsoleFontEx(GetStdHandle(STD_ERROR_HANDLE), FALSE, &fontInfo) && (fontInfo.FontFamily & TMPF_TRUETYPE)); } else { return 1; } }

Having done that, it might be worth the addition of a function in Sys which exposes this (a bit like Unix.has_symlink)

dra27 · 2017-10-05T08:29:31Z

byterun/caml/misc.h

 #define caml_stat_strdup_to_os caml_stat_strdup
 #define caml_stat_strdup_of_os caml_stat_strdup
 #define caml_copy_string_of_os caml_copy_string
+#define caml_utf8_string_of_os(s) s


I think convention says the s should be (s) to ensure that you don't change the meaning of an expression, although I think you'd have to be doing something seriously contrived to manage that with a string pointer.

dra27 · 2017-10-05T08:30:28Z

byterun/caml/osdeps.h

+   The returned string points to a fixed-length, statically-allocated buffer so
+   make sure to copy it if it needs to be preserved.  Also, due to the fixed
+   length of the buffer, the result may be truncated. */
+extern char * win_utf8_string_of_utf16(const wchar_t *s);


The function no longer matches the semantics of the comment, either!

dra27 · 2017-10-05T08:32:16Z

byterun/dynlink.c

  realname = caml_search_dll_in_path(&caml_shared_libs_path, name);
-  caml_gc_message(0x100, "Loading shared library %"
-                  ARCH_CHARNATSTR_PRINTF_FORMAT "\n", realname);
+  caml_gc_message(0x100, "Loading shared library %s\n", caml_utf8_string_of_os(realname));


I agree with @gasche's concern about using a static buffer. You cover nearly all cases of the strdup with either caml_fatal_arg (which will automatically clean up caml_stat_strdup anyway) or caml_gc_message. You could introduce caml_gc_message_free which strictly takes a string parameter and frees it after displaying the message.

dra27 · 2017-10-05T08:32:40Z

byterun/startup.c

    return;

+#if defined(_WIN32) && WINDOWS_UNICODE
+  SetConsoleOutputCP(CP_UTF8);


See similar comment from asmrun/startup.c

dra27 · 2017-10-05T08:32:51Z

byterun/startup.c

    return Val_unit;

+#if defined(_WIN32) && WINDOWS_UNICODE
+  SetConsoleOutputCP(CP_UTF8);


dra27 · 2017-10-05T08:33:47Z

byterun/sys.c

  } else {
   fprintf(stderr, "Cannot load C plugin %s\nReason: %s\n",
-           caml_stat_strdup_of_os(plugin), caml_dlerror());
+           caml_utf8_string_of_os(plugin), caml_dlerror());


This would require a manual free if caml_utf8_string_of_os starts allocating.

nojb · 2017-10-05T08:45:30Z

The current behavior is that it will either display garbage or even nothing at all (depending on Console code page, etc) for non-ASCII characters in those places where %S is used (runtime debug messages). ASCII characters should come out fine.

dra27 · 2017-10-05T08:57:08Z

The SetConsoleOutputCP part is about displaying anything at all - without it, you get raw UTF-8 which from a PR (by which I mean "Public Relations") perspective makes it look like we're not properly supporting Unicode. The code to enable it is correctly guarded by the WINDOWS_UNICODE, so legacy mode can continue "unaffected". I can imagine we may end up getting various reports along the lines of "OCaml isn't displaying foreign characters correctly" which get the response "Please run chcp 65001" otherwise!

dra27 · 2017-10-05T08:57:44Z

Note that running SetConsoleOutputCP affects your process only - the console returns to whichever codepage the user had previously selected when your process terminates.

alainfrisch · 2017-10-05T09:03:48Z

Note that running SetConsoleOutputCP affects your process only

What about the scenario where the OCaml runtime is embedded in a host application? Could this negatively affect outputs of this application?

dra27 · 2017-10-05T09:05:33Z

Compiled in legacy mode, no difference at all; compiled with WINDOWS_UNICODE=1, the Console would start correctly displaying UTF-8 sequences as the characters they represent - such an application would definitely have been displaying garbage before. It definitely only affects the console, not any file translations.

dra27 · 2017-10-05T09:05:49Z

(it also doesn't affect input processing, which is SetConsoleCP)

nojb · 2017-10-05T09:06:38Z

I pushed one commit changing to explicitly allocation/deallocation around the few caml_gc_message calls. Altogether it is much simpler and we do not even have to introduce a new function.

@dra27:

I will add some comments to the SetConsoleOutputCP calls.
What about input? Should we also call SetConsoleCP?
What is the point of checking whether a Unicode font is available? To print a message to the user? Or simply to expose this to the OCaml programmer?

gasche · 2017-10-05T09:16:56Z

byterun/dynlink.c


  realname = caml_search_dll_in_path(&caml_shared_libs_path, name);
-  caml_gc_message(0x100, "Loading shared library %"
-                  ARCH_CHARNATSTR_PRINTF_FORMAT "\n", realname);


Unrelated to the PR, but am I correct that ARCH_CHARNATSTR_PRINTF_FORMAT is a new 4.06 thing that escaped the big os rename? (Will this PR end up removing it?)

Yes, and yes.

gasche · 2017-10-05T09:18:48Z

I have the feeling that we are converging on the matter of how printf stuff should handle the string conversion, but that the ConsoleCP stuff requires another level of expertise and is somewhat orthogonal (although in the end we need to have both for proper Unicode printing, if I understand correctly). @nojb, you may consider splitting the PR in two parts -- but feel free to keep only one if that's easier for you.

dra27 · 2017-10-05T09:21:15Z

byterun/win32.c

+{
+  int retcode = win_wide_char_to_multi_byte_noexc(s, slen, out, outlen);
+
  if (retcode == 0)


This code is almost certainly now eliminated, but this should be if (retcode == 0 && slen != 0)

Indeed, this was the cause for a previous AppVeyor failure, but is now gone.

(that was causing your previous AppVeyor failure, since the building of dynlink includes -ccopt "" so you were hitting the case of an empty string being passed to win_wide_char_to_multi_byte)

dra27 · 2017-10-05T09:36:55Z

@nojb - cosmetically, it would be that we only tell the console to switch to UTF-8 output if it can actually have a chance of displaying the symbols. It would certainly be useful to display that to the user. Cosmetically, setting CP_UTF8 when a truetype font is not selected for the console should cause no change in the output (you just get garbage UTF-8). It certainly doesn't have to be done in this PR at all, therefore.

@gasche - SetConsoleCP only affects input, we get proper Unicode output on the console without doing that.

dra27 · 2017-10-05T10:21:06Z

I don't ~~fully~~ understand exactly how these things work with non-Latin keyboards but, for example, if on my Windows box I add Russian keyboard layout, switch the code page to 1250 and have a simple C program which calls gets and displays the output and press the letters ASD on my QWERTY keyboard, then the C program gets three ??? characters. If I alter the program to run SetConsoleCP(CP_UTF8) and repeat the experiment, then gets returns an empty string.

That experiment is enough to suggest that we shouldn't change the way input is being handled - SetConsoleOutputCP is only ever going to affect what is displayed to the user, it's not going (potentially) to change the semantic behaviour of programs.

nojb · 2017-10-05T10:35:29Z

@alainfrisch Re embedded runtime situation, my understanding is as follows:

The call to SetConsoleOutputCP fixes the way the bytes output by your process (be it OCaml code or some other code) get interpreted by the Console, which is natively Unicode. So if you are emitting bytes (say, by using C's printf), you have to make sure these bytes have the right encoding. But if you are using C# or other .NET things which work with Unicode strings to write to the Console then no translation is actually necessary and things should just work.
The call to SetConsoleOutputCP will not have any effect whatsoever if standard output/error is redirected (in which they are not really going to the Console but to a file, another process, etc...)

@dra27 do you agree?

dra27 · 2017-10-05T11:24:11Z

@nojb - yes, that's my understanding of it.

nojb · 2017-10-05T12:16:50Z

@dra27 I added a comment to the SetConsoleOutputCP calls. I would prefer to leave the other bits (figuring out if the font supports Unicode and possibly exposing this in Sys) for later.

I think we addressed the other points as well, so in principle I consider this PR ready to be merged.

gasche · 2017-10-05T12:22:06Z

Would you rebase the PR to remove some back-and-forth, but preferably keep the console-setting and %S-business in separate commits?

@dra27: do you approve the current state? Do you think we should cherry-pick in 4.06?

nojb · 2017-10-05T12:28:19Z

Rebased.

gasche · 2017-10-05T12:36:51Z

I approve the first and third commits :-)

dra27 · 2017-10-05T12:44:58Z

I approve the second commit :-) Yes, I think we should cherry-pick this to 4.06 - to a certain extent, it's all part of the "big picture" of Windows Unicode support.

dra27 · 2017-10-05T14:51:38Z

Still investigating some issues (especially around MPR#6925) with regard to the SetConsoleOutputCP call.

gasche · 2017-10-05T14:53:36Z

@nojb I would be able to merge the %S change in 4.06 quickly if you send a separate PR (I'll let @dra27 deal with the console business). We need it at some point anyway: that the ARCHCHARNAT must not be released.

nojb · 2017-10-05T14:58:00Z

OK, for simplicity I will simply remove commit 2 from this PR, and we can open a new PR for it as needed.

nojb · 2017-10-05T15:00:03Z

OK, done (and renamed PR for accuracy).

One more Windows Unicode PR: do not use %S (cherry picked from commit 6aae29d)

gasche · 2017-10-08T15:38:46Z

Merged in trunk and cherry-picked in 4.06 ( d8e0921 ). (There was really no other choice than cherry-picking as this commit removes an API that we don't want to release.)

nojb force-pushed the do_not_use_%S branch from 9006c4e to dab0a91 Compare October 4, 2017 22:20

gasche reviewed Oct 5, 2017

View reviewed changes

dra27 reviewed Oct 5, 2017

View reviewed changes

gasche reviewed Oct 5, 2017

View reviewed changes

dra27 reviewed Oct 5, 2017

View reviewed changes

Do not use %S

d0f68c9

nojb force-pushed the do_not_use_%S branch from 30a978f to 0e8afd5 Compare October 5, 2017 12:26

gasche approved these changes Oct 5, 2017

View reviewed changes

dra27 added the suspended label Oct 5, 2017

gasche added this to the 4.06.0 milestone Oct 5, 2017

Add Changes entry

f9290fc

nojb force-pushed the do_not_use_%S branch from 0e8afd5 to f9290fc Compare October 5, 2017 14:59

nojb changed the title ~~One more Windows Unicode PR: do not use %S, set Console code page to UTF-8~~ One more Windows Unicode PR: do not use %S Oct 5, 2017

dra27 removed the suspended label Oct 5, 2017

dra27 mentioned this pull request Oct 6, 2017

Windows Console Unicode Support #1408

Open

gasche merged commit 6aae29d into ocaml:trunk Oct 8, 2017

gasche added a commit that referenced this pull request Oct 8, 2017

Merge pull request #1398 from nojb/do_not_use_%S

d8e0921

One more Windows Unicode PR: do not use %S (cherry picked from commit 6aae29d)

Conversation

nojb commented Oct 4, 2017

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojb commented Oct 5, 2017

Uh oh!

dra27 commented Oct 5, 2017

Uh oh!

gasche commented Oct 5, 2017

Uh oh!

alainfrisch commented Oct 5, 2017

Uh oh!

dra27 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojb commented Oct 5, 2017

Uh oh!

dra27 commented Oct 5, 2017

Uh oh!

dra27 commented Oct 5, 2017

Uh oh!

alainfrisch commented Oct 5, 2017

Uh oh!

dra27 commented Oct 5, 2017

Uh oh!

dra27 commented Oct 5, 2017

Uh oh!

nojb commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasche commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dra27 commented Oct 5, 2017

Uh oh!

dra27 commented Oct 5, 2017

Uh oh!

nojb commented Oct 5, 2017

Uh oh!

dra27 commented Oct 5, 2017

Uh oh!

nojb commented Oct 5, 2017

Uh oh!

gasche commented Oct 5, 2017

Uh oh!

nojb commented Oct 5, 2017

gasche commented Oct 5, 2017 •

edited

Loading