Detecting errors in the LLVM release 13.0.0

Commercial static analyzers perform deeper and fuller code analysis compared to compilers. Let’s see what PVS-Studio found in the source code of the LLVM 13.0.0 project.

How this article appeared

Compilers developers constantly improve their products and built-in code analyzers. Some IDEs such as Visual Studio and CLion provide enhanced built-in analyzers. So, developers ask an obvious question – is it reasonable to use additional solutions to control code quality? Or it’s enough to use built-in tools of a modern compiler or IDE?

Developing a project, you should use the minimum required applications. Therefore, if you use a program with up-to-the-mark mechanisms of code analysis, it’s enough – you don’t need to add additional utilities to the pipeline.

That’s how we do it in PVS-Studio. Occasionally, our users ask us whether we provide analysis better than some other compiler or its analyzer. Usually, the number of such questions increases with a new compiler release.

In theory, there are some proper answers to these questions. Here they are:

  • we constantly enhance our analyzer. We develop new diagnostics (example), improve data flow analysis (example), and so on. Compilers learn to find new bugs, and PVS-Studio learns even faster. That’s why commercial static code analyzers exist;
  • you should not compare analyzers by the number of diagnostics. Besides, their quality and easy integration in the development process are important. Enhanced infrastructure and integration to various systems such as SonarQube, PlatformIO, Azure DevOps, Travis CI, CircleCI, GitLab CI/CD, Jenkins, and so on means a lot. And, of course, don’t downplay the support. Therefore, some new diagnostic rules in the compiler make no difference.

But that’s not what you wanted to hear, yeah? :). It seems that we want to evade the question. This article is a way to answer. Our team checks compilers showing the product’s capabilities.

Today we check the latest LLVM 13.0.0 release. Of course, our readers and we are not interested in LLVM. We’re going to evaluate the power of PVS-Studio in comparison with the Clang compiler, Clang Static Analyzer, and Clang-tidy. LLVM developers use these programs to build and check the project. If we find some errors, you’ll see the benefits of introducing PVS-Studio in the development process.

Previously, we checked LLVM 11. Click here if you’re wondering to know more.

Checking LLVM

It’s more convenient to view PVS-Studio warnings in an IDE. I had Visual Studio 2019 on my computer. So, I used it. And little left to do:

  • download the LLVM 13.0.0 source codes;
  • create a project for VS2019: cmake -S llvm -B build -G “Visual Studio 16 2019”;
  • compile – to generate various inc files, necessary to preprocess and, then, analyze many cpp files;
  • wonder that we have more than 100 Gb of different files;
  • choose to check the solution in the Visual Studio menu to tell the PVS-Studio plugin what to do;
  • profit.

In fact, that’s not so easy. If you don’t want to receive a large number of false or banal (within the project) warnings, you need to preconfigure the analyzer. I don’t mind receiving such warnings, since I need to find some exciting bugs worthy of an article. And that’s all.

If you want to use the analyzer regularly – you need to preconfigure it. Also, it’s better to start with declaring all warnings a technical debt and hide them. Then, you can handle new warnings, gradually eliminating technical debt. Here you can find this approach described in detail.

We have lots of articles explaining how to set up and introduce the analyzer. Let’s stick to the main topic. Curious to know what we found? Let’s figure it out.

I spent an evening viewing the log and wrote out engaging warnings. Sure you can find far more errors. However, the fact that skimming through the report, you can fix 20 errors, demonstrates the analyzer in a favorable light.

Typos

PVS-Studio is, and always has been good at detecting typos. You can easily spot them in code snippets described in the article. During code reviews, programmers fail to find typos and then get angry detecting them after debugging :).

It’s easy to come up with rules for detecting typos. But it is much more difficult to implement them. You have to strike a balance between useful warnings and false positives. The Clang compiler and related analyzers have diagnostics to identify various types of errors that I describe below. But since they did not help – our analyzer has better diagnostics.

Bug N1, trying to create a 64-bit value from two 32-bit values

uint64_t uval;
....
bool DWARFFormValue::extractValue(const DWARFDataExtractor &Data,
                                  uint64_t *OffsetPtr, dwarf::FormParams FP,
                                  const DWARFContext *Ctx,
                                  const DWARFUnit *CU) {
  ....
  case DW_FORM_LLVM_addrx_offset:
    Value.uval = Data.getULEB128(OffsetPtr, &Err) << 32;
    Value.uval = Data.getU32(OffsetPtr, &Err);
    break;
  ....
}

The PVS-Studio warning: V519 [CWE-563, CERT-MSC13-C] The ‘Value.uval’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 334, 335. DWARFFormValue.cpp 335

It makes no sense to write different values one by one to the same variable. This is exactly what the analyzer warns us about. The code author made a typo, forgetting to add ‘|’. This code should create one 64-bit value from two 32-bit values. The correct code looks as follows:

case DW_FORM_LLVM_addrx_offset:
  Value.uval = Data.getULEB128(OffsetPtr, &Err) << 32;
  Value.uval |= Data.getU32(OffsetPtr, &Err);
  break;

Bug N2, hasty copy-paste

In the ExecutorAddress class, we need to implement operators of the same type. I’m pretty sure the programmer used copy-paste. Don’t you think it’s boring to write the following code without copy-paste?

class ExecutorAddress {
  ....
  ExecutorAddress &operator++() {
    ++Addr;
    return *this;
  }
  ExecutorAddress &operator--() {
    --Addr;
    return *this;
  }
  ExecutorAddress operator++(int) { return ExecutorAddress(Addr++); }
  ExecutorAddress operator--(int) { return ExecutorAddress(Addr++); }

  ExecutorAddress &operator+=(const ExecutorAddrDiff Delta) {
    Addr += Delta.getValue();
    return *this;
  }

  ExecutorAddress &operator-=(const ExecutorAddrDiff Delta) {
    Addr -= Delta.getValue();
    return *this;
  }
  ....
private:
  uint64_t Addr = 0;
}

Unfortunately, the faster you write the code – the higher is the probability of forgetting to replace something in the copied code. It’s tedious to write and check such code. That’s why it “attracts” errors.

Fortunately, static analyzers work hard and can’t get tired :). PVS-Studio completes code reviews. It offers to pay attention to these two functions:

ExecutorAddress operator++(int) { return ExecutorAddress(Addr++); }
ExecutorAddress operator--(int) { return ExecutorAddress(Addr++); }

The PVS-Studio warning: V524 It is odd that the body of ‘–‘ function is fully equivalent to the body of ‘++’ function. ExecutorAddress.h 104

A striking error: the programmer forgot to replace the ++ operator with — in the right part of the copied line.

Bug N3, no one knows how to write comparison functions

bool operator==(const BDVState &Other) const {
  return OriginalValue == OriginalValue && BaseValue == Other.BaseValue &&
    Status == Other.Status;
}

V501 [CWE-571] There are identical sub-expressions to the left and to the right of the ‘==’ operator: OriginalValue == OriginalValue RewriteStatepointsForGC.cpp 758

A classic error! I covered this topic in another long article – “The evil within the comparison functions“.

To reduce the number of such errors, I recommend using table-style formatting when handling operations of the same type. Here’s how I would write this function:

bool operator==(const BDVState &Other) const {
  return
       OriginalValue == OriginalValue
    && BaseValue == Other.BaseValue
    && Status == Other.Status;
}

The code is longer, but it helps the programmer to notice the typo during code review. However, you still may fail to notice an error. To be on the safe side, it’s better to use an enhanced analyzer.

Bug N4, no one knows how to write comparison functions (I mean it)

Considering the previous example, you may think that I’m exaggerating because it is a random blooper. Unfortunately, comparison functions tend to typos. Let’s take a look at another example.

bool TypeInfer::EnforceSmallerThan(TypeSetByHwMode &Small,
                                   TypeSetByHwMode &Big) {
  ....
  if (Small.empty())
    Changed |= EnforceAny(Small);
  if (Big.empty())
    Changed |= EnforceAny(Big);

  assert(Small.hasDefault() && Big.hasDefault());

  SmallVector<unsigned, 4> Modes;
  union_modes(Small, Big, Modes);

  for (unsigned M : Modes) {
    TypeSetByHwMode::SetType &S = Small.get(M);
    TypeSetByHwMode::SetType &B = Big.get(M);

    if (any_of(S, isIntegerOrPtr) && any_of(S, isIntegerOrPtr)) {
      auto NotInt = [](MVT VT) { return !isIntegerOrPtr(VT); };
      Changed |= berase_if(S, NotInt);
      Changed |= berase_if(B, NotInt);
    } else if (any_of(S, isFloatingPoint) && any_of(B, isFloatingPoint)) {
      auto NotFP = [](MVT VT) { return !isFloatingPoint(VT); };
      Changed |= berase_if(S, NotFP);
      Changed |= berase_if(B, NotFP);
    } else if (S.empty() || B.empty()) {
      Changed = !S.empty() || !B.empty();
      S.clear();
      B.clear();
    } else {
      TP.error("Incompatible types");
      return Changed;
    }
  ....
}

Why don’t you try to find the typo before I show you the error? Here’s a picture to hide the answer.

Here’s the issue:

if (any_of(S, isIntegerOrPtr) && any_of(S, isIntegerOrPtr))

Bug N5, table-style formatting is not always helpful

LegalizerHelper::LegalizeResult LegalizerHelper::lowerRotate(MachineInstr &MI) {
  Register Dst = MI.getOperand(0).getReg();
  Register Src = MI.getOperand(1).getReg();
  Register Amt = MI.getOperand(2).getReg();
  LLT DstTy = MRI.getType(Dst);
  LLT SrcTy = MRI.getType(Dst);
  LLT AmtTy = MRI.getType(Amt);
  ....
}

The PVS-Studio warning: V656 [CWE-665] Variables ‘DstTy’, ‘SrcTy’ are initialized through the call to the same function. It’s probably an error or un-optimized code. Consider inspecting the ‘MRI.getType(Dst)’ expression. Check lines: 5953, 5954. LegalizerHelper.cpp 5954

As I mentioned earlier, formatting the code with a table helps to protect code from typos. Yep, it helps, but you can’t be sure for 100%. This is beautiful code, resembling a table. But it still contains an error.

It seems that the programmer used copy-paste for the following line:

LLT DstTy = MRI.getType(Dst);

But they replaced Dst by Src only in one place:

LLT SrcTy = MRI.getType(Dst);

The correct code looks as follows:

LLT DstTy = MRI.getType(Dst);
LLT SrcTy = MRI.getType(Src);
LLT AmtTy = MRI.getType(Amt);

Null pointers

One does not simply write code in C or C++ without accidentally dereferencing a null pointer somewhere :). LLVM has such cases too. It’s boring and tedious to study warnings about null pointers. I’ve looked through these warnings. I guess I could find much more of them.

Bug N6, potential null pointer dereferencing

void DwarfCompileUnit::addLabelAddress(DIE &Die, dwarf::Attribute Attribute,
                                       const MCSymbol *Label) {
  ....
  if (Label)
    DD->addArangeLabel(SymbolCU(this, Label));

  bool UseAddrOffsetFormOrExpressions =
      DD->useAddrOffsetForm() || DD->useAddrOffsetExpressions();

  const MCSymbol *Base = nullptr;
  if (Label->isInSection() && UseAddrOffsetFormOrExpressions)
    Base = DD->getSectionLabel(&Label->getSection());
  ....
}

The PVS-Studio warning: V1004 [CWE-476, CERT-EXP34-C] The ‘Label’ pointer was used unsafely after it was verified against nullptr. Check lines: 74, 81. DwarfCompileUnit.cpp 81

The “if (Label)” check tells us and the analyzer that the Label pointer can be null. But then this pointer is dereferenced without any verification:

if (Label->isInSection() && UseAddrOffsetFormOrExpressions)

Better not to do so.

Bug N7-N9, potential null pointer dereferencing

static bool HandleUse(....)
{
  ....
  if (Pat->isLeaf()) {
    DefInit *DI = dyn_cast<DefInit>(Pat->getLeafValue());
    if (!DI)
      I.error("Input $" + Pat->getName() + " must be an identifier!");
    Rec = DI->getDef();
  }
  ....
}

The PVS-Studio warning: V1004 [CWE-476, CERT-EXP34-C] The ‘DI’ pointer was used unsafely after it was verified against nullptr. Check lines: 3349, 3351. CodeGenDAGPatterns.cpp 3351

The DI pointer is checked, but then it is immediately dereferenced without checking. The question arises: is this an error? If the DI pointer is null, the error function that can generate an exception is called. Let’s take a look at this function:

void TreePattern::error(const Twine &Msg) {
  if (HasError)
    return;
  dump();
  PrintError(TheRecord->getLoc(), "In " + TheRecord->getName() + ": " + Msg);
  HasError = true;
}

Nope, this function does not throw an exception and does not terminate the program.

Right after logging an error state, null pointer dereference happens.

The project has a few more similar errors. There’s no point to consider them separately:

  • V1004 [CWE-476, CERT-EXP34-C] The ‘OpDef’ pointer was used unsafely after it was verified against nullptr. Check lines: 2843, 2844. CodeGenDAGPatterns.cpp 2844
  • V1004 [CWE-476, CERT-EXP34-C] The ‘Val’ pointer was used unsafely after it was verified against nullptr. Check lines: 3418, 3420. CodeGenDAGPatterns.cpp 3420

Bug N10, insufficient protection from null pointer

Error DWARFDebugLine::LineTable::parse(...., raw_ostream *OS, bool Verbose) {
  assert((OS || !Verbose) && "cannot have verbose output without stream");
  ....
  auto EmitRow = [&] {
    if (!TombstonedAddress) {
      if (Verbose) {
        *OS << "\n";
        OS->indent(12);
      }
      if (OS)
        State.Row.dump(*OS);
      State.appendRowToMatrix();
    }
  };
  ....
}

The PVS-Studio warning: V595 [CWE-476, CERT-EXP12-C] The ‘OS’ pointer was utilized before it was verified against nullptr. Check lines: 791, 793. DWARFDebugLine.cpp 791

The “if (OS)” check indicates that the OS pointer can be null. However, this pointer can be already dereferenced without prior verification.

The code block starts with assert that protects against null pointers. However, this is not enough, since, in the release build, the assert macro is expanded in an empty string.

It’s better to make the code safer:

auto EmitRow = [&] {
  if (!TombstonedAddress) {
    if (OS)
    {
      if (Verbose) {
        *OS << "\n";
        OS->indent(12);
      }
      State.Row.dump(*OS);
    }
    State.appendRowToMatrix();
  }
};

Issues with enumerations (enum)

LLVM developers sometimes think that small enums are represented by a single byte. That is, sizeof(enum) == sizeof(char). It’s dangerous to think so. For example, by default, the Visual C++ compiler equals the size of the enumeration with the size of int.

Bug N11, a dangerous index

enum class FunctionKinds { ENTRY, EXIT, TAIL, LOG_ARGS_ENTER, CUSTOM_EVENT };
....
static Error loadObj(....) {
  ....
  auto Kind = Extractor.getU8(&OffsetPtr);
  static constexpr SledEntry::FunctionKinds Kinds[] = {
      SledEntry::FunctionKinds::ENTRY, SledEntry::FunctionKinds::EXIT,
      SledEntry::FunctionKinds::TAIL,
      SledEntry::FunctionKinds::LOG_ARGS_ENTER,
      SledEntry::FunctionKinds::CUSTOM_EVENT};
  if (Kind >= sizeof(Kinds))
    return errorCodeToError(
        std::make_error_code(std::errc::executable_format_error));
  Entry.Kind = Kinds[Kind];
  ....
}

The PVS-Studio warning: V557 [CWE-125, CERT-ARR30-C] Array overrun is possible. The value of ‘Kind’ index could reach 19. InstrumentationMap.cpp 196

The warning requires explanation. Data flow analysis processes this code:

if (Kind >= sizeof(Kinds))
  return errorCodeToError(...);

As a result, if the condition is not met, the Kind variable further has the [0..19] value.

Why 19 and not 4? I checked the project with a plugin for Visual Studio 2019. So, the analyzer knows that the Visual C++ compiler was used and that the enumeration is represented by four bytes. You can verify this by writing the following test program:

int main()
{
  enum class FunctionKinds { ENTRY, EXIT, TAIL, LOG_ARGS_ENTER, CUSTOM_EVENT };
  static constexpr FunctionKinds Kinds[] = {
    FunctionKinds::ENTRY, FunctionKinds::EXIT, FunctionKinds::TAIL,
    FunctionKinds::LOG_ARGS_ENTER, FunctionKinds::CUSTOM_EVENT
  };
  std::cout << sizeof(Kinds) << std::endl;
  return 0;
}

We build the program with the Visual C++ compiler, run it and see the number “20”.

It turns out that our code is not protected from the protection against array index out of bounds. To fix the code, you need to compare Kind not with the size of the array in bytes, but with the number of array elements.

The correct check:

if (Kind >= sizeof(Kinds) / sizeof(Kinds[0]))
  return errorCodeToError(....);

Bug N12, array initialization error

enum CondCode {
  // Opcode       N U L G E       Intuitive operation
  SETFALSE, //      0 0 0 0       Always false (always folded)
  SETOEQ,   //      0 0 0 1       True if ordered and equal
  ....
  SETCC_INVALID // Marker value.
};

static void InitCmpLibcallCCs(ISD::CondCode *CCs) {
  memset(CCs, ISD::SETCC_INVALID, sizeof(ISD::CondCode)*RTLIB::UNKNOWN_LIBCALL);
  ....
}

The PVS-Studio warning: V575 [CWE-628, CERT-EXP37-C] The ‘memset’ function processes the pointer to enum type. Inspect the first argument. TargetLoweringBase.cpp 662

The code runs only if you are lucky and the elements of the CondCode enumeration are represented by one byte.

The memset function fills an array of bytes. The SETCC_INVALID value is written to each byte. If enum is represented by 4 bytes, as happens with Visual C++ assembly, the array is filled with meaningless values. These values are equal to the result of repeating the constant in each of the 4 bytes:

SETCC_INVALID << 24 | SETCC_INVALID << 16 | SETCC_INVALID << 8 | SETCC_INVALID

The correct way to fill the array:

std::fill(CCs, CCs + RTLIB::UNKNOWN_LIBCALL, ISD::SETCC_INVALID);

Control flow errors

Bug N13-N14, uninitialized variable

Expected<std::pair<JITTargetAddress, Edge::Kind>>
EHFrameEdgeFixer::readEncodedPointer(uint8_t PointerEncoding,
                                     JITTargetAddress PointerFieldAddress,
                                     BinaryStreamReader &RecordReader) {
  .....
  Edge::Kind PointerEdgeKind;

  switch (EffectiveType) {
  case DW_EH_PE_udata4: {
    ....
    PointerEdgeKind = Delta32;
    break;
  }
  case DW_EH_PE_udata8: {
    ....
    PointerEdgeKind = Delta64;
    break;
  }
  case DW_EH_PE_sdata4: {
    ....
    PointerEdgeKind = Delta32;
    break;
  }
  case DW_EH_PE_sdata8: {
    ....
    PointerEdgeKind = Delta64;
    break;
  }
  }

  if (PointerEdgeKind == Edge::Invalid)
    return make_error<JITLinkError>(
        "Unspported edge kind for encoded pointer at " +
        formatv("{0:x}", PointerFieldAddress));

  return std::make_pair(Addr, Delta64);
}

The PVS-Studio warning: V614 [CWE-457, CERT-EXP53-CPP] Potentially uninitialized variable ‘PointerEdgeKind’ used. EHFrameSupport.cpp 704

The PointerEdgeKind variable may remain uninitialized after executing the switch block. However, if the variable has not been initialized, then it is expected to be equal to the named Edge::invalid constant.

You should initialize it with this constant immediately when declaring a variable:

Edge::Kind PointerEdgeKind = Edge::Invalid;

Another such error: V614 [CWE-457, CERT-EXP53-CPP] Potentially uninitialized variable ‘RESULT’ used. llvm-rtdyld.cpp 998

Bug N15, unreachable code

At the beginning, consider the auxiliary report_fatal_error function:

void llvm::report_fatal_error(const Twine &Reason, bool GenCrashDiag) {
  ....
  abort();
}

The important thing here is that it terminates the program by calling the abort function. That is, report_fatal_error is the noreturn function.

There is also an intermediate function, the call of which we discuss further:

void llvm::report_fatal_error(const char *Reason, bool GenCrashDiag) {
  report_fatal_error(Twine(Reason), GenCrashDiag);
}

Note. The GenCrashDiag argument is optional:

__declspec(noreturn) void report_fatal_error(const char *reason, 
                                                bool gen_crash_diag = true);

By the way, it just struck me – we could not consider the body of the function. The annotation of the __declspec(noreturn) function states that it does not return control. But I decided to leave it as it is to explain the situation as detailed as possible.

Let’s get to the point. Take a look at this code snippet:

int AMDGPUCFGStructurizer::improveSimpleJumpintoIf(....)
{
  ....
  if (LandBlkHasOtherPred) {
    report_fatal_error("Extra register needed to handle CFG");
    Register CmpResReg =
        HeadMBB->getParent()->getRegInfo().createVirtualRegister(I32RC);
    report_fatal_error("Extra compare instruction needed to handle CFG");
    insertCondBranchBefore(LandBlk, I, R600::IF_PREDICATE_SET,
        CmpResReg, DebugLoc());
  }
  ....
}

The PVS-Studio warning: V779 [CWE-561, CERT-MSC12-C] Unreachable code detected. It is possible that an error is present. AMDILCFGStructurizer.cpp 1286

Note that after calling the report_fatal_error function, the program is still trying to do something. All these actions don’t make sense anymore.

I guess the code author did not plan to terminate the program but wanted to report an error. Perhaps a programmer must use some other function to log information about the issue.

Bug N16-N17, a memory leak

uint64_t WinCOFFObjectWriter::writeObject(MCAssembler &Asm,
                                          const MCAsmLayout &Layout) {
  ....
  if (EmitAddrsigSection) {
    auto Frag = new MCDataFragment(AddrsigSection);
    Frag->setLayoutOrder(0);
    raw_svector_ostream OS(Frag->getContents());
    for (const MCSymbol *S : AddrsigSyms) {
      if (!S->isTemporary()) {
        encodeULEB128(S->getIndex(), OS);
        continue;
      }

      MCSection *TargetSection = &S->getSection();
      assert(SectionMap.find(TargetSection) != SectionMap.end() &&
             "Section must already have been defined in "
             "executePostLayoutBinding!");
      encodeULEB128(SectionMap[TargetSection]->Symbol->getIndex(), OS);
    }
  }
  ....
}

The PVS-Studio warning: V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] Visibility scope of the ‘Frag’ pointer was exited without releasing the memory. A memory leak is possible. WinCOFFObjectWriter.cpp 1116

Perhaps, I’m wrong, and it’s not an error. But I don’t understand where and how the object referenced by the Frag pointer can be deleted. I agree with the analyzer: it looks like a memory leak.

A similar case: V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] Visibility scope of the ‘Frag’ pointer was exited without releasing the memory. A memory leak is possible. WinCOFFObjectWriter.cpp 1130

Code smell

In this section, you can view code fragments that attracted my attention. However, I can’t call them bugs. It resembles redundant and unsuccessful code. Now I’m going to explain it to you.

Code smell N1, duplicate lines

static uint16_t toSecMapFlags(uint32_t Flags) {
  uint16_t Ret = 0;
  if (Flags & COFF::IMAGE_SCN_MEM_READ)
    Ret |= static_cast<uint16_t>(OMFSegDescFlags::Read);
  if (Flags & COFF::IMAGE_SCN_MEM_WRITE)
    Ret |= static_cast<uint16_t>(OMFSegDescFlags::Write);
  if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
    Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);
  if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
    Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);
  if (!(Flags & COFF::IMAGE_SCN_MEM_16BIT))
    Ret |= static_cast<uint16_t>(OMFSegDescFlags::AddressIs32Bit);
  ....
}

The PVS-Studio warning: V581 [CWE-670] The conditional expressions of the ‘if’ statements situated alongside each other are identical. Check lines: 335, 337. DbiStreamBuilder.cpp 337

This fragment is repeated twice:

if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
  Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);

I think that this is a random redundant code, and it’s better to delete it. However, this may be a real error if a programmer intended to run other checks and perform some other actions in the second block.

Code smell N2, atavism

std::string pathname_;
....
void FilePath::Normalize() {
  if (pathname_.c_str() == nullptr) {
    pathname_ = "";
    return;
  }
....
}

The PVS-Studio warning: V547 [CWE-570] Expression ‘pathname_.c_str() == nullptr’ is always false. gtest-filepath.cc 349

If we delete the function implementation, nothing will change. It does nothing. It looks like an artifact of several consecutive refactorings.

Code smell N3, the misplaced bracket

raw_ostream &raw_ostream::write_escaped(StringRef Str,
                                        bool UseHexEscapes) {
  ....
  *this << hexdigit((c >> 4 & 0xF));
  *this << hexdigit((c >> 0) & 0xF);
  ....
}

The PVS-Studio warning: V592 The expression was enclosed by parentheses twice: ‘((c >> 4 & 0xF))’.V592 The expression was enclosed by parentheses twice: ‘((c >> 4 & 0xF))’. One pair of parentheses is unnecessary or misprint is present. raw_ostream.cpp 188

The first line has double parentheses. This redundancy indicates that a programmer wanted to write the expression in a different way. Indeed, the next line demonstrates the way they wanted to write it. Parentheses were used to make it easier to read the expression.

Programmers wanted to write the following code:

*this << hexdigit((c >> 4) & 0xF);
*this << hexdigit((c >> 0) & 0xF);

Although the parenthesis is in the wrong place, it is not an error. Anyway, the shift precedence (>>) is higher than the binary AND (&). Everything is calculated correctly.

Code smell N4-N6, an unsuccessful code merge?

template <class ELFT>
void ELFState<ELFT>::writeSectionContent(
    Elf_Shdr &SHeader, const ELFYAML::StackSizesSection &Section,
    ContiguousBlobAccumulator &CBA) {
  if (!Section.Entries)
    return;

  if (!Section.Entries)
    return;
  ....
}

The PVS-Studio warning: V581 [CWE-670] The conditional expressions of the ‘if’ statements situated alongside each other are identical. Check lines: 1380, 1383. ELFEmitter.cpp 1383

It looks like unsuccessful merge of two code branches, which caused duplicate lines. It’s not an error, but it’s worth removing the duplicate.

Here are more similar fragments with code duplicates:

  • V581 [CWE-670] The conditional expressions of the ‘if’ statements situated alongside each other are identical. Check lines: 1488, 1491. ELFEmitter.cpp 1491
  • V581 [CWE-670] The conditional expressions of the ‘if’ statements situated alongside each other are identical. Check lines: 1663, 1666. ELFEmitter.cpp 1666

Conclusion

PVS-Studio is still a worthy solution for developers. It has produced and continues to produce deeper and more diverse code analysis compared to compilers and free tools.

Since PVS-Studio is able to find errors even in such well-tested applications as compilers, it makes sense to see what it can find in your projects :). I suggest to try the trial version of the analyzer right away. Thank you for your attention.

Additional links

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s

This site uses Akismet to reduce spam. Learn how your comment data is processed.