bnoordhuis / node-iconv

node.js iconv bindings - text recoding for fun and profit!
Other
799 stars 123 forks source link

Pointer Truncation Warning on Windows 64-bit builds #174

Closed superman20 closed 3 years ago

superman20 commented 7 years ago

There are quite a few warning when installing this on 64-bit Windows using either Visual Studio 2015 or 2017 tools. It looks like it stems from the (int)(long) constructs like those in aliases.h (starting at line 813): {(int)(long)&((struct stringpool_t *)0)->stringpool_str7, ei_sjis}, Visual Studio 2015 and 2017 are more strict about reporting pointer truncation (even when explicit casting is used). On Windows, type long, is always 32-bits no matter the build type (32 or 64-bit). So when building for Windows 64-bit, the cast to long is truncating a 64 pointer to 32-bits and thus the warning. Clearly, this doesn't really truncate anything based on the construct used. However, it would be nice if the warnings weren't there. To fix, one would need to cast to a 64-bit integral type (on Windows) before casting to int.

bnoordhuis commented 7 years ago

That's code that comes from upstream libiconv. I don't usually touch that unless I have to because floating patches makes syncing with upstream more difficult. If you report it to libiconv and it gets fixed there, I'll pick it up the next time I sync or I can cherry-pick the patch ahead of time.

edit: I could also simply suppress the warning since it's harmless. What's the Cxxxx code it prints?

superman20 commented 7 years ago

The code is C4311, but let me chase it down upstream before you just suppress it. I think it'd be best to fix upstream. That warning is actually useful in most cases (just not this one).

Edit: Upstream bug posted. I'll let you know how it goes.

superman20 commented 7 years ago

Here is the response I received from Bruno Haible of libiconv:

The warnings come from the use of gperf. gperf 3.1 is fixed: it no longer produces code that elicits these warnings; see https://savannah.gnu.org/bugs/?45330 .

The libiconv 1.15 release, unfortunately, was made with gperf-3.0.4. But the next libiconv release will use gperf 3.1 or newer.

As a workaround, you can either ignore the warnings, or regenerate said files, through Makefile.devel, assuming you have gperf version 3.1 in your PATH.

As for me, I'm fine to ignore these warnings until the gperf fix works its way into the next libiconv release.

superman20 commented 7 years ago

FYI...Just in case you want to know. The fix is to replace all the constructs that cast a pointer with (int)(long) to (int)(size_t)

bnoordhuis commented 7 years ago

Regenerating the file isn't too onerous, I'll do that in the next release. Thanks!

superman20 commented 7 years ago

Excellent. Thanks!

pl4yradam commented 4 years ago

Still doesnt work

superman20 commented 3 years ago

Is there still any interest in fixing this in the near future? I'm seeing these warnings in several projects now and they really pollute the log files with over 1700 lines of warning. I tried chasing it upstream and it is still not fixed in libiconv 1.16 as suggested here. I'm fine with suppressing warning 4311 since it seems like an upstream fix will never happen. If you prefer, I can submit a pull request to add the warning suppression. Thanks.

bnoordhuis commented 3 years ago

I published iconv@3.0.1 just now with a suppression for that warning.

superman20 commented 3 years ago

Thank you!