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

src: init emit_env_nonstring_warning_ #19283

Closed

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Mar 11, 2018

Currently there is no member initialiser for emit_env_nonstring_warning_
in the Environment constructor leading to undefined behaviour.
For a debug build, this memory would be initialized:

(lldb) memory read -f x -s 4 -c 1 &emit_env_nonstring_warning_
0x7fff5fbfe254: 0x00000001

But for a release build there will be no such initialization:

(lldb) memory read -f x -s 4 -c 1 `$r12 + 0x46c`
0x7fff5fbfe374: 0x00000000

This can be seen by running test-process-env-deprecation.js using
the Debug build and the Release build.

This commit adds a member initialiser for emit_env_nonstring_warning_
setting it to true.

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

Currently there is no member initialiser for emit_env_nonstring_warning_
in the Environment constructor leading to undefined behaviour.
For a debug build, this memory would be initialized:
(lldb) memory read -f x -s 4 -c 1 &emit_env_nonstring_warning_
0x7fff5fbfe254: 0x00000001

But for a release build there will be no such initialization:
(lldb) memory read -f x -s 4 -c 1 `$r12 + 0x46c`
0x7fff5fbfe374: 0x00000000

This can be seen by running test-process-env-deprecation.js using
the Debug build and the Release build.

This commit adds a member initialiser for emit_env_nonstring_warning_
setting it to true.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 11, 2018
@danbev
Copy link
Contributor Author

danbev commented Mar 11, 2018

@danbev
Copy link
Contributor Author

danbev commented Mar 11, 2018

node-test-commit-freebsd failure looks unrelated

console output:

not ok 708 parallel/test-http-client-timeout-agent
  ---
  duration_ms: 1.695
  severity: fail
  stack: |-
    res#0 data:0
    res#0 end
    res#2 data:2
    res#2 end
    res#4 data:4
    res#4 end
    res#6 data:6
    res#6 end
    res#8 data:8
    res#8 end
    res#10 data:10
    res#10 end
    res#12 data:12
    res#12 end
    res#14 data:14
    res#14 end
    res#16 data:16
    res#16 end
    res#18 data:18
    res#18 end
    res#20 data:20
    res#20 end
    res#22 data:22
    res#22 end
    res#24 data:24
    res#24 end
    res#26 data:26
    res#26 end
    res#28 data:28
    res#28 end
    req#1 timeout
    req#3 timeout
    req#5 timeout
    req#7 timeout
    req#9 timeout
    req#11 timeout
    req#13 timeout
    req#15 timeout
    req#17 timeout
    req#19 timeout
    req#21 timeout
    req#23 timeout
    req#25 timeout
    req#27 timeout
    req#29 timeout
    req#0 timeout
    req#2 timeout
    req#4 timeout
    req#6 timeout
    req#8 timeout
    req#10 timeout
    req#12 timeout
    req#14 timeout
    req#16 timeout
    req#18 timeout
    req#20 timeout
    req#22 timeout
    req#24 timeout
    req#26 timeout
    req#28 timeout
    req#28 close
    req#26 close
    req#24 close
    req#22 close
    req#20 close
    req#18 close
    req#16 close
    req#14 close
    req#12 close
    req#10 close
    req#8 close
    req#6 close
    req#4 close
    req#2 close
    req#0 close
    req#29 error
    req#29 close
    req#27 error
    req#27 close
    req#25 error
    req#25 close
    req#23 error
    req#23 close
    req#21 error
    req#21 close
    req#19 error
    req#19 close
    req#17 error
    req#17 close
    req#15 error
    req#15 close
    req#13 error
    req#13 close
    req#11 error
    req#11 close
    req#9 error
    req#9 close
    req#7 error
    req#7 close
    req#5 error
    req#5 close
    req#3 error
    req#3 close
    req#1 error
    req#1 close
    done=45 sent=30
    assert.js:80
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: timeout on http request called too much
        at process.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-http-client-timeout-agent.js:94:10)
        at process.emit (events.js:187:15)
  ...

@danbev
Copy link
Contributor Author

danbev commented Mar 11, 2018

@addaleax Do you think this should be fast-tracked perhaps? It is causing all new PR to fail at the moment.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 11, 2018

I think we should fast track this.

@danbev danbev added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 11, 2018
@danbev
Copy link
Contributor Author

danbev commented Mar 11, 2018

Landed in a7e298a

@danbev danbev closed this Mar 11, 2018
@danbev danbev deleted the init_emit_env_nonstring_warning_ branch March 11, 2018 15:06
danbev added a commit that referenced this pull request Mar 11, 2018
Currently there is no member initialiser for emit_env_nonstring_warning_
in the Environment constructor leading to undefined behaviour.
For a debug build, this memory would be initialized:
(lldb) memory read -f x -s 4 -c 1 &emit_env_nonstring_warning_
0x7fff5fbfe254: 0x00000001

But for a release build there will be no such initialization:
(lldb) memory read -f x -s 4 -c 1 `$r12 + 0x46c`
0x7fff5fbfe374: 0x00000000

This can be seen by running test-process-env-deprecation.js using
the Debug build and the Release build.

This commit adds a member initialiser for emit_env_nonstring_warning_
setting it to true.

PR-URL: #19283
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Currently there is no member initialiser for emit_env_nonstring_warning_
in the Environment constructor leading to undefined behaviour.
For a debug build, this memory would be initialized:
(lldb) memory read -f x -s 4 -c 1 &emit_env_nonstring_warning_
0x7fff5fbfe254: 0x00000001

But for a release build there will be no such initialization:
(lldb) memory read -f x -s 4 -c 1 `$r12 + 0x46c`
0x7fff5fbfe374: 0x00000000

This can be seen by running test-process-env-deprecation.js using
the Debug build and the Release build.

This commit adds a member initialiser for emit_env_nonstring_warning_
setting it to true.

PR-URL: nodejs#19283
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x? It fails to compile with the following warning

../src/env.cc:89:7: error: member initializer 'emit_env_nonstring_warning_' does not name a non-static data member or base class
      emit_env_nonstring_warning_(true),
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[1]: *** [/Users/mborins/code/node/v8.x/out/Release/obj.target/node_lib/src/env.o] Error 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants