nodejs / llnode

An lldb plugin for Node.js and V8, which enables inspection of JavaScript states for insights into Node.js processes and their core dumps.
Other
1.15k stars 100 forks source link

src: fix gcc/clang warnings #297

Closed fanatid closed 5 years ago

fanatid commented 5 years ago

2 warnings still here. maybe-uninitialized looks like GCC bug. unused-but-set-variable introduced in https://github.com/nodejs/llnode/commit/b53010b97c6fec725439b0b0da3560b0f23fb9fc#diff-86a9160147028a03800f8ba4fa555285R1502-R1525 -- not sure how should be handled.

PR GCC (9.2.1):

../src/llv8.cc: In member function ‘llnode::v8::Value llnode::v8::JSObject::GetDescriptorProperty(std::string, llnode::v8::Map, llnode::Error&)’:
../src/llv8.cc:1149:14: warning: variable ‘value’ set but not used [-Wunused-but-set-variable]
 1149 |       double value;
      |              ^~~~~
  CXX(target) Release/obj.target/plugin/src/llv8-constants.o
  CXX(target) Release/obj.target/plugin/src/llscan.o
../src/llscan.cc: In member function ‘virtual bool llnode::FindReferencesCmd::DoExecute(lldb::SBDebugger, char**, lldb::SBCommandReturnObject&)’:
../src/llscan.cc:568:22: warning: ‘scanner’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  568 |     ScanForReferences(scanner);
      |     ~~~~~~~~~~~~~~~~~^~~~~~~~~

master GCC (9.2.1):

../src/llv8.cc: In member function ‘double llnode::v8::LLV8::LoadDouble(int64_t, llnode::Error&)’:
../src/llv8.cc:114:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  114 |   return *reinterpret_cast<double*>(&value);
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/llv8.cc: In member function ‘llnode::v8::Value llnode::v8::JSObject::GetDescriptorProperty(std::string, llnode::v8::Map, llnode::Error&)’:
../src/llv8.cc:1149:14: warning: variable ‘value’ set but not used [-Wunused-but-set-variable]
 1149 |       double value;
      |              ^~~~~
../src/llv8.cc: In constructor ‘llnode::v8::StackTrace::StackTrace(llnode::v8::JSArray, llnode::Error&)’:
../src/llv8.cc:1291:74: warning: format ‘%lld’ expects argument of type ‘long long int’, but argument 2 has type ‘int’ [-Wformat=]
 1291 |           "JSArray doesn't look like a Stack Frames array. stack_len: %lld "
      |                                                                       ~~~^
      |                                                                          |
      |                                                                          long long int
      |                                                                       %d
 1292 |           "array_len: %lld",
 1293 |           len_, frame_array_.GetArrayLength(err));
      |           ~~~~                                                            
      |           |
      |           int
../src/llv8.cc:1292:26: warning: format ‘%lld’ expects argument of type ‘long long int’, but argument 3 has type ‘int64_t’ {aka ‘long int’} [-Wformat=]
 1292 |           "array_len: %lld",
      |                       ~~~^
      |                          |
      |                          long long int
      |                       %ld
 1293 |           len_, frame_array_.GetArrayLength(err));
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                            |
      |                                            int64_t {aka long int}
../src/llscan.cc: In member function ‘virtual bool llnode::FindReferencesCmd::DoExecute(lldb::SBDebugger, char**, lldb::SBCommandReturnObject&)’:
../src/llscan.cc:568:22: warning: ‘scanner’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  568 |     ScanForReferences(scanner);
      |     ~~~~~~~~~~~~~~~~~^~~~~~~~~

master clang (8.0.0):

In file included from ../src/llnode.cc:13:
../src/llnode.h:42:13: warning: private field 'llv8_' is not used [-Wunused-private-field]
  v8::LLV8* llv8_;
            ^
1 warning generated.
../src/llv8.cc:1293:11: warning: format specifies type 'long long' but the argument has type 'int' [-Wformat]
          len_, frame_array_.GetArrayLength(err));
          ^~~~
../src/llv8.cc:1293:17: warning: format specifies type 'long long' but the argument has type 'int64_t' (aka 'long') [-Wformat]
          len_, frame_array_.GetArrayLength(err));
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
../src/llscan.cc:650:19: warning: format string is not a string literal (potentially insecure) [-Wformat-security]
    result.Printf(seen_str.str().c_str());
                  ^~~~~~~~~~~~~~~~~~~~~~
../src/llscan.cc:650:19: note: treat the string as an argument to avoid this
    result.Printf(seen_str.str().c_str());
                  ^
                  "%s",
1 warning generated.
codecov-io commented 5 years ago

Codecov Report

Merging #297 into master will increase coverage by 0.61%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
+ Coverage   79.23%   79.85%   +0.61%     
==========================================
  Files          33       33              
  Lines        4229     4229              
==========================================
+ Hits         3351     3377      +26     
+ Misses        878      852      -26
Impacted Files Coverage Δ
src/llnode.cc 74.2% <ø> (ø) :arrow_up:
src/llnode.h 50% <ø> (ø) :arrow_up:
src/llscan.cc 61.31% <100%> (ø) :arrow_up:
src/llv8.cc 75.46% <100%> (+2.46%) :arrow_up:
src/llv8-constants.cc 83.49% <0%> (+0.97%) :arrow_up:
src/llv8-inl.h 92.67% <0%> (+1.04%) :arrow_up:
src/llv8-constants.h 100% <0%> (+1.51%) :arrow_up:
src/llv8.h 100% <0%> (+2.38%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e1a74b0...8fe0f04. Read the comment docs.

mmarchini commented 5 years ago

Thanks for looking into it! Overall LGTM.

maybe-uninitialized looks like GCC bug

It does indeed. Maybe there's a better way to write that piece of code which will prevent the warning though? I'll open an issue.

unused-but-set-variable introduced in b53010b#diff-86a9160147028a03800f8ba4fa555285R1502-R1525 -- not sure how should be handled.

That looks wrong... I'm pretty sure I stumbled on this piece of code a few times and didn't change for a reason. I'll open an issue to investigate it further.

mmarchini commented 5 years ago

Landed in 84eefb4, thanks!