Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix char vs wchar_t bug when expanding command line arguments under Windows #1622

Merged
merged 2 commits into from Feb 28, 2018

Conversation

Projects
None yet
4 participants
@nojb
Copy link
Contributor

nojb commented Feb 20, 2018

I don't think it would cause a segmentation fault, but it would surely garble argv.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 20, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Feb 20, 2018

Yes, I think we should experiment with enabling more warnings for cl.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 20, 2018

@nojb it may be good to either add a no-change-entry-needed label or
to add a Changes entry. I would tend to add the label but was not
sure this is what you would like to do that's why I did not do
it myself.

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Feb 20, 2018

Yes, I would also think this one does not need a Changes entry.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 20, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Feb 27, 2018

This is a real bug fix, and people observing the weird behavior with old versions of OCaml would appreciate to see an explicit reference to the fix in the Changelog.

And it would indeed be interesting to understand why there was no compiler warning!

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Feb 28, 2018

This is a real bug fix, and people observing the weird behavior with old versions of OCaml would appreciate to see an explicit reference to the fix in the Changelog.

OK, I'll add a Changes entry.

And it would indeed be interesting to understand why there was no compiler warning!

cl is not careful enough by default. I compiled the following program

/* foo.c */
#include <wchar.h>
#include <stdio.h>

int main(void)
{
  wchar_t foo[2];
  char c = foo[0];
  printf("%c\n", c); fflush(stdout);
  return 0;
}

with cl /Wn foo.c with n = 0, 1, 2 (no warning). Only with n >= 3 (n = 4 is the maximum) did I get

foo.c(7): warning C4244: 'initializing': conversion from 'wchar_t' to 'char', possible loss of data

I will try to see if it is possible to compile the runtime system with /W3 later.

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Feb 28, 2018

For the record, see https://gist.github.com/nojb/b6318b0cfc0982a9604bcd917c987adc for the list of warnings produced by using /W3 with make world.opt.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 28, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Feb 28, 2018

set_configuration msvc64 "$OCAMLROOT" -WX

and

set_configuration mingw "$OCAMLROOT-mingw32" -Werror

These lines are to treat compiler warnings as errors, but do not change the set of active warnings.

But I seemed to remember @ëra27 explained that with a higher warning
level in msvc, there will be false positives which will be hard to get
rid of.

Yes, that's what I am afraid of as well.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 28, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Feb 28, 2018

MPR#5079 tracks the task to get the runtime to compile with /W3. As @nojb sees, we're not there yet! We may also want to opt in to specific warnings - that particular one looks worth fully enabling.

@nojb - have you seen this garbling anything? I don't think it would garble anything, because the casts would be correct?

@nojb nojb force-pushed the nojb:fix_wchar_vs_char branch from 97c9b4b to 046f114 Feb 28, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Feb 28, 2018

@nojb - have you seen this garbling anything? I don't think it would garble anything, because the casts would be correct?

Well, I think it can truncate the string, since the assignment char c = prefix[i-1] truncates a wchar_t to a char and so can trigger the if condition when it shouldn't. I amended the Changes note to say "truncate" rather than "garble".

@alainfrisch alainfrisch added the bug label Feb 28, 2018

@alainfrisch alainfrisch merged commit 8f3d66c into ocaml:trunk Feb 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.