src: fix compiler warning in udp_wrap.cc #15402

danbev · 2017-09-14T06:39:14Z

Currently the following compiler warning is generated:

1 warning generated.
../src/udp_wrap.cc:238:12: warning: comparison of integers of different
signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare]
  if (size != args[0].As<Uint32>()->Value()) {
      ~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

This commit changes the check to see that the Uint32 value does not
exceed the max int size instead of first casting and then comparing.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

mscdex · 2017-09-14T06:48:36Z

src/udp_wrap.cc

@@ -233,16 +233,17 @@ void UDPWrap::BufferSize(const FunctionCallbackInfo<Value>& args) {

  CHECK(args[0]->IsUint32());
  CHECK(args[1]->IsUint32());
-  int size = static_cast<int>(args[0].As<Uint32>()->Value());
+  unsigned int u_size = static_cast<int>(args[0].As<Uint32>()->Value());


Better to use uint32_t?

Much better, will update now. Thanks!

danbev · 2017-09-14T06:52:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10072/

danbev · 2017-09-14T11:01:51Z

Updated CI run: https://ci.nodejs.org/job/node-test-pull-request/10073/

targos · 2017-09-14T11:46:12Z

src/udp_wrap.cc

@@ -233,16 +233,17 @@ void UDPWrap::BufferSize(const FunctionCallbackInfo<Value>& args) {

  CHECK(args[0]->IsUint32());
  CHECK(args[1]->IsUint32());
-  int size = static_cast<int>(args[0].As<Uint32>()->Value());
+  uint32_t u_size = static_cast<int>(args[0].As<Uint32>()->Value());


shouldn't this be static_cast<uint32_t> ?

actually, Uint32::Value() returns a uint32_t already so you can remove the cast

@targos Thanks!

I was just focusing on the compiler warning and not giving proper attention to the code I'm afraid. I think it was correct to have the cast to int as this is then checked to make sure that the value can fit into an int before passing it along to uv_recv_buffer_size or uv_send_buffer_size. Otherwise it will never detect such a situation.

fhinkel · 2017-09-16T08:00:44Z

src/udp_wrap.cc


-  if (size != args[0].As<Uint32>()->Value()) {
+  if (args[0].As<Uint32>()->Value() > std::numeric_limits<int>::max()) {


Thanks!

There is a IsInt32() function, https://cs.chromium.org/chromium/src/v8/src/api.cc?q=IsUint32&sq=package:chromium&l=3694.

If you want, we could replace the > check with IsInt32(). But I'm also fine with landing as is, as it's correct and gets rid of the compiler warning.

There is a bit too much typecasting to my taste. I'd probably assign it to a uint32_t, do the range check and only then assign it to an int. IsInt32() would work too since the CHECK(args[0]->IsUint32()) a few lines up establishes that it's >= 0.

Aside: the second argument is really just a bool in disguise. The logic in lib/dgram.js could be simplified to self._handle.bufferSize(size, buffer === 'recv') if the code here was updated to expect a bool.

Aside aside: since this is new code, it should have been written to use the Value() overloads that take a Local<Context>. Oh well.

@fhinkel Nice, I was not aware of that. I'll update the PR. Thanks!

@bnoordhuis I'd be happy to take a closer look at this, but perhaps as a separate PR?

Currently the following compiler warning is generated: 1 warning generated. ../src/udp_wrap.cc:238:12: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare] if (size != args[0].As<Uint32>()->Value()) { ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. This commit changes the check to see that the Uint32 value does not exceed the max int size instead of first casting and then comparing.

danbev · 2017-09-20T11:11:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10161/

danbev · 2017-09-20T13:41:14Z

test/aix failure looks unrelated

console output:

not ok 1686 inspector/test-stop-profile-after-done # TODO : Fix flaky test
  ---
  duration_ms: 0.791
  severity: flaky
  stack: |-
    [test] Connecting to a child Node process
    [test] Testing /json/list
    [err] Debugger listening on ws://127.0.0.1:55055/85359bfe-2a33-4295-936c-616bbf936f31
    [err] For help see https://nodejs.org/en/docs/inspector
    [err] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    { Error: connect ECONNREFUSED 127.0.0.1:55055
        at Object._errnoException (util.js:1018:13)
        at _exceptionWithHostPort (util.js:1039:20)
        at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1179:14)
      errno: 'ECONNREFUSED',
      code: 'ECONNREFUSED',
      syscall: 'connect',
      address: '127.0.0.1',
      port: 55055 }
    1
  ...

test/arm-fanned failure looks unrelated

console output:

+ git clean -fdx
warning: failed to remove out/
Removing out/
Build step 'Execute shell' marked build as failure
TAP Reports Processing: START
Looking for TAP results report in workspace using pattern: *.tap
Did not find any matching files. Setting build result to FAILURE.
Checking ^not ok
Jenkins Text Finder: File set '*.tap' is empty
Notifying upstream projects of job completion
Finished: FAILURE

Currently the following compiler warning is generated: 1 warning generated. ../src/udp_wrap.cc:238:12: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare] if (size != args[0].As<Uint32>()->Value()) { ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. This commit changes the check to see that the Uint32 value does not exceed the max int size instead of first casting and then comparing. PR-URL: nodejs#15402 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

danbev · 2017-09-20T13:59:45Z

Landed in 87bddda.

@targos @fhinkel Thanks!

jasnell · 2017-09-20T17:16:56Z

This does not land cleanly in v8.x as it depends on #13623 and should be backported along with that one.

Currently the following compiler warning is generated: 1 warning generated. ../src/udp_wrap.cc:238:12: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare] if (size != args[0].As<Uint32>()->Value()) { ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. This commit changes the check to see that the Uint32 value does not exceed the max int size instead of first casting and then comparing. PR-URL: nodejs/node#15402 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Currently the following compiler warning is generated: 1 warning generated. ../src/udp_wrap.cc:238:12: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare] if (size != args[0].As<Uint32>()->Value()) { ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. This commit changes the check to see that the Uint32 value does not exceed the max int size instead of first casting and then comparing. PR-URL: #15402 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

MylesBorins · 2017-10-17T19:11:18Z

setting as dont-land-v6.x as the commit this is based on is Semver-Minor and has not landed in v6.x. Please lmk if you think we should reconsider

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dgram Issues and PRs related to the dgram subsystem / UDP. labels Sep 14, 2017

mscdex reviewed Sep 14, 2017

View reviewed changes

danbev force-pushed the udp_wrap_compiler_warning branch from 282baf3 to c91f5ec Compare September 14, 2017 06:52

danbev force-pushed the udp_wrap_compiler_warning branch from c91f5ec to a1c2c81 Compare September 14, 2017 07:34

targos reviewed Sep 14, 2017

View reviewed changes

danbev force-pushed the udp_wrap_compiler_warning branch from a1c2c81 to 115989a Compare September 15, 2017 05:21

fhinkel reviewed Sep 16, 2017

View reviewed changes

fhinkel approved these changes Sep 16, 2017

View reviewed changes

danbev force-pushed the udp_wrap_compiler_warning branch from 115989a to ff211e9 Compare September 17, 2017 10:06

danbev closed this Sep 20, 2017

danbev deleted the udp_wrap_compiler_warning branch September 20, 2017 14:00

jasnell mentioned this pull request Sep 20, 2017

Dgram buffers #13623

Closed

4 tasks

jasnell added the backport-requested-v8.x label Sep 20, 2017

MylesBorins removed the backport-requested-v8.x label Sep 28, 2017

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fix compiler warning in udp_wrap.cc #15402

src: fix compiler warning in udp_wrap.cc #15402

danbev commented Sep 14, 2017 •

edited

Loading

mscdex Sep 14, 2017

danbev Sep 14, 2017

danbev commented Sep 14, 2017

danbev commented Sep 14, 2017

targos Sep 14, 2017

targos Sep 14, 2017

danbev Sep 14, 2017

fhinkel Sep 16, 2017

bnoordhuis Sep 16, 2017

danbev Sep 17, 2017

danbev commented Sep 20, 2017

danbev commented Sep 20, 2017

danbev commented Sep 20, 2017

jasnell commented Sep 20, 2017

MylesBorins commented Oct 17, 2017


		if (size != args[0].As<Uint32>()->Value()) {
		if (args[0].As<Uint32>()->Value() > std::numeric_limits<int>::max()) {

src: fix compiler warning in udp_wrap.cc #15402

src: fix compiler warning in udp_wrap.cc #15402

Conversation

danbev commented Sep 14, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Sep 14, 2017

danbev commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Sep 20, 2017

danbev commented Sep 20, 2017

danbev commented Sep 20, 2017

jasnell commented Sep 20, 2017

MylesBorins commented Oct 17, 2017

danbev commented Sep 14, 2017 •

edited

Loading