This is an archive of the discontinued LLVM Phabricator instance.

[clang-analyzer] fix warnings emitted on lldb code base
Needs ReviewPublic

Authored by apelete on Apr 13 2016, 5:18 PM.

Details

Summary

The following warnings were reported while running clang analyzer on
LLDB code base:

API: argument with 'nonnull' attribute passed null, on file:

  • source/Plugins/SymbolFile/DWARF/DWARFASTParserJava.cpp.

Dead store: dead assignement, on file:

  • source/Plugins/SymbolFile/DWARF/DWARFASTParserJava.cpp.

Logic error: branch condition evaluates to a garbage value, on file:

  • source/Core/DynamicLoader.cpp

Logic error: called C++ object pointer is null, on files:

  • source/API/SBThread.cpp,
  • source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp,
  • source/DataFormatters/VectorType.cpp (22 warnings in cpp file, suppressed in custom assertion handler in include/lldb/Utility/LLDBAssert.h),
  • source/Core/FormatEntity.cpp.

(please note that first revision was sent only to lldb-commits mailing
list, not reviewed yet).

Signed-off-by: Apelete Seketeli <apelete@seketeli.net>

Diff Detail

Event Timeline

apelete updated this revision to Diff 53646.Apr 13 2016, 5:18 PM
apelete retitled this revision from to [clang-analyzer] fix warnings emitted on lldb code base.
apelete updated this object.
apelete added a subscriber: lldb-commits.

These look like good changes to me, especially the SBThread fix - I'm surprised that hasn't caused a problem. The DynamicLoader change should be unnecessary but I don't have a problem with that. Tamas might want to speak to what he meant to do in DWARFASTParserJava.cpp there. Greg Clayton should probably comment on the FormatEntity change.

source/Core/FormatEntity.cpp
1016

Greg should look at this.

granata.enrico added inline comments.
source/Core/FormatEntity.cpp
1016

Personally, this LGTM (the code was originally written by me - Greg moved it around last though)

Greg, feel free to disagree

jingham requested changes to this revision.Apr 13 2016, 5:51 PM
jingham added a reviewer: jingham.
jingham added a subscriber: jingham.
jingham added inline comments.
source/API/SBThread.cpp
926

Can you switch this around and check for log first? We generally don't like to do any work that the log might do if there's no log, and also it makes it easier to see that this whole block is only for logging.

source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp
309–329

Seems to me this whole section of code doesn't do anything reasonable if reg_ctx is null. It would be better to move the reg_ctx check higher up.

This revision now requires changes to proceed.Apr 13 2016, 5:51 PM
apelete updated this revision to Diff 53774.Apr 14 2016, 1:08 PM
apelete edited edge metadata.

[clang-analyzer] fix warnings emitted on lldb code base

Following changes were done in this revision:

  • source/API/SBThread.cpp: swith if() statement conditions to branch out early,
  • source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp: return early if reg_ctx is null.
apelete marked 2 inline comments as done.Apr 14 2016, 1:10 PM

Ping :).
Requested changes are done, could someone have a look have this one ?

clayborg accepted this revision.Apr 19 2016, 1:43 PM
clayborg edited edge metadata.

These all look fine.

apelete updated this revision to Diff 55753.May 1 2016, 9:50 AM
apelete edited edge metadata.

[clang-analyzer] fix warnings emitted on lldb code base

Changes since last revision:

  • fast forward rebase on git master branch.

Waiting for review, could someone please have a look at this one ?

clayborg accepted this revision.May 2 2016, 10:03 AM
clayborg edited edge metadata.

Looks good

granata.enrico resigned from this revision.Nov 29 2016, 10:05 AM
granata.enrico removed a reviewer: granata.enrico.

I am not an Apple employee working on LLDB anymore