GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix some clang-cl warnings regarding unused parameters by lemire · Pull Request #8 · targos/node

Conversation

lemire commented Dec 5, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

They occur because we try to compile C code as C++17. The -std=c++17 flag should not be used when compiling C code. A quickfix is to set cflags_cc=[] in C dependencies. It clears the C++ flags.

Interestingly, it illustrates that the following configuration...

            ['target_arch in "ia32 x64" and OS!="ios"', {
              'defines': [ 'ADLER32_SIMD_SSSE3' ],
              'cflags': [ '-mssse3' ],
              'cflags_cc': [],
              'conditions': [

is not triggered. So you get both a build error because you are missing SSSE3, but also a visible warning.

I am sure that it is quite trivial.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          This fixes some clang-cl warnings regarding unused parameters. They

62f6d33

occur because we try to compile C code as C++17.

lemire mentioned this pull request

Dec 5, 2023

build: enable Clang-cl Windows builds nodejs/node#35433

Closed

lemire commented

Dec 5, 2023

View reviewed changes

Comment thread

deps/zlib/zlib.gyp

		'conditions': [
		['target_arch in "ia32 x64" and OS!="ios"', {
		'defines': [ 'ADLER32_SIMD_SSSE3' ],
		'cflags': [ '-mssse3' ],

lemire Dec 5, 2023

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anonrig Would you happen to know why these variables (cflags, cflags_cpp) don't take hold here even though we are building under Windows with x64.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

targos Dec 6, 2023

Copy link

Copy Markdown

Owner

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a GYP issue. It was never meant to support Clang on Windows AFAIK and cflags are passed differently for it. See c1d1878

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

targos reviewed

Dec 7, 2023

View reviewed changes

Comment thread

deps/base64/base64.gyp

		'include_dirs': [ 'base64/include', 'base64/lib' ],
		'sources': [ 'base64/lib/arch/ssse3/codec.c' ],
		'defines': [ 'BASE64_STATIC_DEFINE', 'HAVE_SSSE3=1' ],
		'cflags_cpp': [],

targos Dec 7, 2023

Copy link

Copy Markdown

Owner

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these work? What are the fixed warnings?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

lemire Dec 7, 2023

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without these, you are passing-std=c++17 to clang-cl when compiling C code. The better solution would be for the build engine to recognize that it is compiling C code, and not pass the C++ flags.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

targos Dec 7, 2023

Copy link

Copy Markdown

Owner

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that it works because I don't see any occurence of cflags_cpp in the code base (including the source code of GYP)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

lemire Dec 8, 2023

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. It should be cflags_cc.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

targos commented Dec 7, 2023

Copy link

Copy Markdown

Owner

They occur because we try to compile C code as C++17.

I think this comes from common.gypi:

https://github.com/nodejs/node/blob/3f4ea7ad7f4a1cb058bdc5363311607d1793928e/common.gypi#L284-L289

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lemire commented Dec 7, 2023

Copy link

Copy Markdown

Author

I think this comes from common.gypi:

Yes. And that flag looks fine to me. The issue is that it gets applied when clang-cl builds C code. Node has several C dependencies and they are the ones that appear most broken right now.

Short of making the build engine smarter, I propose to clear the C++ flags when within a C dependency.

It does not quite work perfectly because, as we have observed, some flags do not get applied under clang-cl when it seems they should. This issue leads to breakage having to do with SIMD instructions.

The better solution would be to fix the build engine so that it actually supports clang-cl as a first class compiler.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lemire commented Dec 7, 2023

Copy link

Copy Markdown

Author

Basically this PR gets you closer to building under clang-cl without warnings and errors because it manually removes the unnecessary C++ flags within C projects. I think that you pretty much need to do this if fixing GYP is not an option.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos commented Dec 7, 2023

Copy link

Copy Markdown

Owner

I understand what you're trying to do but what I understand of GYP is that it ignores cflags and related options for the Visual studio generator.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lemire commented Dec 7, 2023

Copy link

Copy Markdown

Author

I understand what you're trying to do but what I understand of GYP is that it ignores cflags and related options for the Visual studio generator.

Checkout my PR and run it. It will take you a minute.

It works! It drastically reduces the volume of warnings.

It is a verifiable statement, you can check.

anonrig reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lemire commented Dec 7, 2023

Copy link

Copy Markdown

Author

To put it another way, GYP does not ignore all of these changes, just a couple.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos commented Dec 7, 2023

Copy link

Copy Markdown

Owner

First warning I see (clean clone with this patch applied):

clang-cl : warning : argument unused during compilation: '-std:c++17' [-Wunused-command-line-argument] [D:\Git\nodejs\node\deps\histogram\histogram.vcxproj]

Then a lot more in uvwasi and zlib

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lemire commented Dec 7, 2023

Copy link

Copy Markdown

Author

First warning I see (clean clone with this patch applied):

With a fresh build? That's intriguing.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lemire commented Dec 8, 2023

Copy link

Copy Markdown

Author

Please see nodejs#35433 (comment)

I think that there is a fundamental problem that should be solved and that what I am proposing here is wrong.

Closing.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lemire closed this

Dec 8, 2023

lemire commented Dec 8, 2023

Copy link

Copy Markdown

Author

@targos Note that the approach illustrated by this PR does work. You do get rid of the warnings when setting cflags_cc to the empty array. (An earlier commit had cflags_cpp which is the wrong variable.)

It is a hack, however. I don't recommend it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung pushed a commit that referenced this pull request

Jun 3, 2026


          src: fix edge case when deflateInit2() fails with Z_VERSION_ERROR

458c37b

This function call can fail with `Z_VERSION_ERROR` if the compiled
library vs loaded library mismatched in version number or in
stream structure size.
In those cases, zlib doesn't initialize the `strm_.msg` field to
null. Therefore, when a `CompressionError` object is created via
`ErrorForMessage()`, it can read a stale or uninitialized `strm_.msg`
pointer that will cause a crash.

Example ASAN report:
```
AddressSanitizer: SEGV on unknown address
    #0 __strlen_avx2
        string/../sysdeps/x86_64/multiarch/strlen-avx2.S:76
    #1 strlen (/work/node/out/Debug/node+0x1a42ab7)
    #2 v8::(anonymous namespace)::StringLength(char const*)
        /work/node/out/../deps/v8/src/api/api.cc:7581:16
    #3 v8::(anonymous namespace)::StringLength(unsigned char const*)
        /work/node/out/../deps/v8/src/api/api.cc:7587:10
    #4 v8::String::NewFromOneByte(v8::Isolate*,
        unsigned char const*, v8::NewStringType, int)
        /work/node/out/../deps/v8/src/api/api.cc:7677:3
    #5 node::OneByteString(v8::Isolate*,
        char const*, int, v8::NewStringType)
        /work/node/out/../src/util-inl.h:166:10
    #6 node::(anonymous namespace)::CompressionStream<
        node::(anonymous namespace)::ZlibContext>
        ::EmitError(node::(anonymous namespace)
        ::CompressionError const&)
        /work/node/out/../src/node_zlib.cc:565:7
    #7 node::(anonymous namespace)::CompressionStream<
        node::(anonymous namespace)::ZlibContext>
        ::CheckError()
        /work/node/out/../src/node_zlib.cc:519:5
    #8 node::(anonymous namespace)::CompressionStream<
        node::(anonymous namespace)::ZlibContext>
        ::AfterThreadPoolWork(int)
        /work/node/out/../src/node_zlib.cc:543:10
    #9 node::ThreadPoolWork::ScheduleWork()
        ::'lambda'(uv_work_s*, int)
        ::operator()(uv_work_s*, int) const
        /work/node/out/../src/threadpoolwork-inl.h:57:15
    #10 node::ThreadPoolWork::ScheduleWork()
        ::'lambda'(uv_work_s*, int)
        ::__invoke(uv_work_s*, int)
        /work/node/out/../src/threadpoolwork-inl.h:48:7
    #11 uv__work_done /work/libuv-1.51.0/src/threadpool.c:330:5
    #12 uv__async_io.part.0
        /work/libuv-1.51.0/src/unix/async.c:208:5
```

Signed-off-by: ndossche <[email protected]>
PR-URL: nodejs#63476
Reviewed-By: Anna Henningsen <[email protected]>

Reviewers

targos

targos left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

lemire commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lemire Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

targos Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

targos Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

lemire Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

targos Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

lemire Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

targos commented Dec 7, 2023

Uh oh!

lemire commented Dec 7, 2023

Uh oh!

lemire commented Dec 7, 2023

Uh oh!

targos commented Dec 7, 2023

Uh oh!

lemire commented Dec 7, 2023

Uh oh!

lemire commented Dec 7, 2023

Uh oh!

targos commented Dec 7, 2023

Uh oh!

lemire commented Dec 7, 2023

Uh oh!

lemire commented Dec 8, 2023

Uh oh!

lemire commented Dec 8, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lemire commented Dec 5, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.