Скачай PVS-Studio и найди ошибки в С, С++,C# коде
237,04
рейтинг
22 января в 14:21

Разработка → ChakraCore: проверка JavaScript-движка для Microsoft Edge

В декабре 2015 года на конференции JSConf US разработчики объявили, что планируют открыть исходный код ключевых компонентов JavaScript-движка Chakra, работающего в Microsoft Edge. Недавно исходный код ChackraCore под MIT лицензией опубликовали в соответствующем репозитории на GitHub. В статье я расскажу, что удалось найти интересного в проекте с помощью статического анализатора PVS-Studio.

Введение


ChakraCore это базовая составляющая Chakra, высокопроизводительный движок JavaScript, который запускает приложения Microsoft Edge и Windows, написанные на HTML/CSS/JS. ChakraCore поддерживает JIT-компиляцию на JavaScript для x86/x64/ARM, сборку мусора и широкий спектр самых последних возможностей JavaScript.

PVS-Studio — это статический анализатор для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Инструмент PVS-Studio предназначен для разработчиков современных приложений и интегрируется в среды Visual Studio 2010-2015.

Подготавливая статью о проверке очередного открытого проекта, мы приводим в ней информацию далеко не о всех предупреждениях, которые выдаёт статический анализатор. Поэтому мы рекомендуем авторам проекта самостоятельно выполнить анализ и изучить все выдаваемые анализатором сообщения. Мы предоставляем на время ключ разработчикам открытых проектов.

Разные ошибки




V501 There are identical sub-expressions 'this->propId == Js::PropertyIds::_superReferenceSymbol' to the left and to the right of the '||' operator. diagobjectmodel.cpp 123

IDiagObjectModelDisplay * ResolvedObject::CreateDisplay()
{
  ....
  if (this->isConst ||
    this->propId == Js::PropertyIds::_superReferenceSymbol ||
    this->propId == Js::PropertyIds::_superReferenceSymbol)
  {
      pOMDisplay->SetDefaultTypeAttribute(....);
  }
  ....
}

В условии присутствует две одинаковые проверки. Возможно, при написании кода, в меню IntelliSense случайно была выбрана такая же константа, например, вместо «Js::PropertyIds:: _superCtorReferenceSymbol».

V501 There are identical sub-expressions 'GetVarSymID(srcIndexOpnd->GetStackSym())' to the left and to the right of the '==' operator. globopt.cpp 20795

void GlobOpt::EmitMemop(....)
{
  ....
  IR::RegOpnd *srcBaseOpnd = nullptr;
  IR::RegOpnd *srcIndexOpnd = nullptr;
  IRType srcType;
  GetMemOpSrcInfo(...., srcBaseOpnd, srcIndexOpnd, srcType);
  Assert(GetVarSymID(srcIndexOpnd->GetStackSym()) ==        // <=
         GetVarSymID(srcIndexOpnd->GetStackSym()));         // <=
  ....
}

Ещё два одинаковых сравнения. Скорее всего, хотели сравнить «srcIndexOpnd->GetStackSym()» c " srcBaseOpnd ->GetStackSym()".

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 3220, 3231. lower.cpp 3220

bool Lowerer::GenerateFastBrSrEq(....,
                                 IR::RegOpnd * srcReg1,
                                 IR::RegOpnd * srcReg2,
                                 ....)
{
  if (srcReg2 && IsConstRegOpnd(srcReg2))
  {
    ....
  }
  else if (srcReg1 && IsConstRegOpnd(srcReg1))
  {
    ....
  }
  else if (srcReg2 && (srcReg2->m_sym->m_isStrConst))
  {
    ....
  }
  else if (srcReg1 && (srcReg1->m_sym->m_isStrConst))       // <=
  {
    ....
  }
  else if (srcReg2 && (srcReg2->m_sym->m_isStrEmpty))
  {
    ....
  }
  else if (srcReg1 && (srcReg1->m_sym->m_isStrConst))       // <=
  {
    ....
  }

  return false;
}

В каскаде условных операторов найдены две одинаковые проверки, вследствие чего, блок кода в последнем условии никогда не получает управления. Полный код представленного примера очень велик и в нём сложно заметить опечатку. Это хороший пример, показывающий как бывает полезен статического анализатор при работе с однотипным кодом, от которого программист быстро устаёт и теряет внимательность.

Скорее всего, последние два условия хотели написать так:

....
else if (srcReg2 && (srcReg2->m_sym->m_isStrEmpty))
{
  ....
}
else if (srcReg1 && (srcReg1->m_sym-> m_isStrEmpty))       // <=
{
  ....
}

V713 The pointer scriptContext was utilized in the logical expression before it was verified against nullptr in the same logical expression. diaghelpermethodwrapper.cpp 214

template <bool doCheckParentInterpreterFrame>
void HandleHelperOrLibraryMethodWrapperException(....)
{
  ....
  if (!exceptionObject->IsDebuggerSkip() ||
    exceptionObject == scriptContext->GetThreadContext()->.... ||
    exceptionObject == scriptContext->GetThreadContext()->.... ||
    !scriptContext)    // <=
  {
    throw exceptionObject->CloneIfStaticExceptionObject(....);
  }
  ....
}

Разыменование указателя «scriptContext» выполняется раньше, чем проверяется его корректность. Благодаря везению, такая ошибка ещё не проявила себя и не была замечена. Подобные ошибки могут очень долго жить в коде, и проявляют себя в редких нетипичных ситуациях.

V570 The 'this->isInlined' variable is assigned to itself. functioncodegenjittimedata.h 625

void SetupRecursiveInlineeChain(
    Recycler *const recycler,
    const ProfileId profiledCallSiteId)
{
  if (!inlinees)
  {
    inlinees = RecyclerNewArrayZ(....);
  }
  inlinees[profiledCallSiteId] = this;
  inlineeCount++;
  this->isInlined = isInlined;   // <=
}

Очень подозрительно, что в булевскую переменную 'isInlined' кладётся тоже самое значение. Скорее всего хотели написать что-то другое.

Ещё одно место присваивания переменной самой себе:

  • V570 The 'sym->m_isTaggableIntConst' variable is assigned to itself. linearscan.cpp 3170

V590 Consider inspecting the 'sub[i] != '-' && sub[i] == '/'' expression. The expression is excessive or contains a misprint. rl.cpp 1388

const char *
stristr
(
  const char * str,
  const char * sub
)
{
  ....
  for (i = 0; i < len; i++)
  {
    if (tolower(str[i]) != tolower(sub[i]))
    {
      if ((str[i] != '/' && str[i] != '-') ||
            (sub[i] != '-' && sub[i] == '/')) {              / <=
           // if the mismatch is not between '/' and '-'
           break;
      }
    }
  }
  ....
}

Анализатор обнаружил, что часть условного выражения (sub[i] != '-') не влияет на результат проверки. Чтобы в этом убедиться, построим таблицу истинности. Скорее всего здесь имеет место какая-то опечатка, но каким должен быть правильный код, я затрудняюсь ответить.



V603 The object was created but it is not being used. If you wish to call constructor, 'this->StringCopyInfo::StringCopyInfo(....)' should be used. stringcopyinfo.cpp 64

void StringCopyInfo::InstantiateForceInlinedMembers()
{
    AnalysisAssert(false);

    StringCopyInfo copyInfo;
    JavascriptString *const string = nullptr;
    wchar_t *const buffer = nullptr;

    (StringCopyInfo());                     // <=
    (StringCopyInfo(string, buffer));       // <=
    copyInfo.SourceString();
    copyInfo.DestinationBuffer();
}

Программисты часто ошибаются, пытаясь явно вызвать конструктор для инициализации объекта. В данном примере создаются новые неименованные объекты типа «StringCopyInfo» и тут же разрушаются. В результате поля класса остаются неинициализированными.

Правильным вариантом было бы создать функцию инициализации и вызывать её из конструкторов и в этом месте.

V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. constants.h 39

class Constants
{
public:
  ....
  static const int Int31MinValue = -1 << 30;
  ....
};

Согласно современному стандарту языка C++, сдвиг отрицательного числа приводит к неопределённому поведению.

V557 Array overrun is possible. The value of 'i' index could reach 8. rl.cpp 2375

enum TestInfoKind::_TIK_COUNT = 9

const char * const TestInfoEnvLstFmt[] =
{
   " TESTFILE=\"%s\"",
   " BASELINE=\"%s\"",
   " CFLAGS=\"%s\"",
   " LFLAGS=\"%s\"",
   NULL,
   NULL,
   NULL,
   NULL    // <= TestInfoEnvLstFmt[7]
};

void
WriteEnvLst
(
   Test * pDir, TestList * pTestList
)
{
  ....
  // print the other TIK_*
  for(int i=0;i < _TIK_COUNT; i++) {
    if (variants->testInfo.data[i] && TestInfoEnvLstFmt[i]){// <=
       LstFilesOut->Add(TestInfoEnvLstFmt[i],               // <=
                        variants->testInfo.data[i]);
    }
    ....
  }
  ....
}

Анализатор обнаружил выход индекса за пределы массива. Дело в том, что цикл for() выполняет 9 итераций, а в массиве «TestInfoEnvLstFmt[]» всего 8 элементов.

Скорее всего забыли добавить ещё один NULL в конце:

const char * const TestInfoEnvLstFmt[] =
{
   " TESTFILE=\"%s\"",
   " BASELINE=\"%s\"",
   " CFLAGS=\"%s\"",
   " LFLAGS=\"%s\"",
   NULL,
   NULL,
   NULL,
   NULL    // <= TestInfoEnvLstFmt[7]
   NULL    // <= TestInfoEnvLstFmt[8]
};

Но может быть и пропустили какую-нибудь строку в середине массива!

Опасные указатели




Диагностика V595 находит участки кода, где выполняется разыменование указателя до его проверки на ноль. Обычно в проверяемых проектах всегда находится некоторое количество таких предупреждений. В нашей базе ошибок она занимает первое место по количеству найденных недочётов (см. примеры). Но дело в том, что диагностики V595 несколько скучны, чтобы выписывать много примеров из какого-то проекта. Также проверка и разыменование указателя могут находится довольно далеко в коде функции: в десятках или даже сотнях строк друг от друга, что усложняет описание ошибки в статье.

Далее я опишу всего несколько наиболее коротких и наглядных примеров кода, в которых скорее всего присутствует ошибка при работе с указателями.

V595 The 'instrLd' pointer was utilized before it was verified against nullptr. Check lines: 1823, 1831. flowgraph.cpp 1823

IR::Instr *
FlowGraph::PeepTypedCm(IR::Instr *instr)
{
 ....
 if (instrLd && !instrLd->GetSrc1()->IsEqual(instr->GetDst()))
 {
   return nullptr;
 }
 
 if(instrLd2 && !instrLd2->GetSrc1()->IsEqual(instrLd->GetDst()))
 {
   return nullptr;
 }
 ....
}

Обратите внимание на указатель с именем «instrLd». В первом условии разыменование этого указателя выполняется в паре с проверкой на ноль, но во втором условии это сделать забыли, поэтому может возникнуть ситуация, когда произойдёт разыменование нулевого указателя.

V595 The 'src2Val' pointer was utilized before it was verified against nullptr. Check lines: 9717, 9725. globopt.cpp 9717

bool GlobOpt::TypeSpecializeIntBinary(....)
{
  ....
  bool isIntConstMissingItem = src2Val->GetValueInfo()->....

  if(isIntConstMissingItem)
  {
      isIntConstMissingItem = Js::SparseArraySegment<int>::....
  }

  if (!src2Val || !(src2Val->GetValueInfo()->IsLikelyInt()) ||
      isIntConstMissingItem)
  {
      return false;
  }
  ....
}

Указатель «src2Val» используется в начале функции, но потом разработчики активно начали проверять, является ли указатель равным нулю.

V595 The 'm_lastInstr' pointer was utilized before it was verified against nullptr. Check lines: 214, 228. irbuilderasmjs.cpp 214

void
IRBuilderAsmJs::AddInstr(IR::Instr * instr, uint32 offset)
{
  m_lastInstr->InsertAfter(instr);                  // <=
  if (offset != Js::Constants::NoByteCodeOffset)
  {
    ....
  }
  else if (m_lastInstr)                             // <=
  {
      instr->SetByteCodeOffset(m_lastInstr->GetByteCodeOffset());
  }
  m_lastInstr = instr;
  ....
}

Ещё один пример неаккуратного использования указателя, который потенциально может быть нулевым.

Список похожих мест:

  • V595 The 'arrayData' pointer was utilized before it was verified against nullptr. Check lines: 868, 870. immutablelist.h 868
  • V595 The 'pMembersList' pointer was utilized before it was verified against nullptr. Check lines: 2012, 2015. diagobjectmodel.cpp 2012
  • V595 The 'walkerRef' pointer was utilized before it was verified against nullptr. Check lines: 3191, 3193. diagobjectmodel.cpp 3191
  • V595 The 'block->loop' pointer was utilized before it was verified against nullptr. Check lines: 981, 1002. globopt.cpp 981
  • V595 The 'src2Val' pointer was utilized before it was verified against nullptr. Check lines: 12528, 12536. globopt.cpp 12528
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 1966, 1967. irbuilderasmjs.cpp 1966
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 2010, 2011. irbuilderasmjs.cpp 2010
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 2076, 2077. irbuilderasmjs.cpp 2076
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 3591, 3592. irbuilderasmjs.cpp 3591
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 4113, 4114. irbuilderasmjs.cpp 4113
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 4510, 4511. irbuilderasmjs.cpp 4510
  • V595 The 'm_lastInstr' pointer was utilized before it was verified against nullptr. Check lines: 1102, 1116. irbuilder.cpp 1102

В списке представлены наиболее простые и понятные примеры. Для изучения всех подобных мест разработчикам следует самим изучить отчёт анализатора.

V522 Dereferencing of the null pointer 'tempNumberTracker' might take place. backwardpass.cpp 578

void
BackwardPass::MergeSuccBlocksInfo(BasicBlock * block)
{
  TempNumberTracker * tempNumberTracker = nullptr; // <= line 346
  ....
  if (!block->isDead)
  {
      ....
      if(!IsCollectionPass())
      {
          ....
          if (this->DoMarkTempNumbers())
          {
              tempNumberTracker = JitAnew(....);   // <= line 413
          }
      ....
  ....
  if (blockSucc->tempNumberTracker != nullptr)
  {
      ....
      tempNumberTracker->MergeData(....);          // <= line 578
      if (deleteData)
      {
          blockSucc->tempNumberTracker = nullptr;
      }
  }
  ....
}

Пример другой диагностики, но тоже про указатели. Здесь представлен фрагмент функции MergeSuccBlocksInfo(), она довольно длинная — 707 строк. Но с помощью статического анализа удалось найти указатель «tempNumberTracker», инициализация которого потенциально может не выполнится из-за ряда условий. В результате при неудачном стечении обстоятельств произойдёт разыменование нулевого указателя.

Остановись! Проверь Assert!




Assert, размещённый в программе, указывает на то, что разработчик предполагает, что некоторое выражение является истинным для корректно работающей программы. Но можно ли доверять таким «успешным» проверкам?

V547 Expression 'srcIndex — src->left >= 0' is always true. Unsigned type value is always >= 0. sparsearraysegment.inl 355

class SparseArraySegmentBase
{
public:
    static const uint32 MaxLength;
    ....
    uint32 size;
    ....
}

template<typename T>
SparseArraySegment<T>* SparseArraySegment<T>::CopySegment(....,
  uint32 srcIndex, ....)
{
  ....
  AssertMsg(srcIndex - src->left >= 0,                    // <=
    "src->left > srcIndex resulting in \
     negative indexing of src->elements");
  js_memcpy_s(dst->elements + dstIndex - dst->left,
              sizeof(T) * inputLen,
              src->elements + srcIndex - src->left,
              sizeof(T) * inputLen);
  return dst;
}

Обратите внимание на сравнение «srcIndex — src->left >= 0». Разность двух беззнаковым чисел всегда будет больше или равна нулю. Далее эта формула используется в функции для работы с памятью. Результат может быть не таким, как ожидал программист.

V547 Expression is always true. Probably the '&&' operator should be used here. bytecodegenerator.cpp 805

void ByteCodeGenerator::AssignRegister(Symbol *sym)
{
  AssertMsg(sym->GetDecl() == nullptr ||
            sym->GetDecl()->nop != knopConstDecl ||      // <=
            sym->GetDecl()->nop != knopLetDecl, "...."); // <=
            
  if (sym->GetLocation() == Js::Constants::NoRegister)
  {
    sym->SetLocation(NextVarRegister());
  }
}

В этом Assert'е тестирование некоторых значений выполняется частично. Если предположение «sym->GetDecl() == nullptr» ложно, то следующие условия всегда истинны. В этом можно убедиться, построив таблицу истинности:



V547 Expression 'callSiteId >= 0' is always true. Unsigned type value is always >= 0. inline.cpp 1181

typedef uint16 ProfileId;

Func * Inline::BuildInlinee(Js::FunctionBody* funcBody, ....)
{
  ....
  Js::ProfileId callSiteId = static_cast<Js::ProfileId>(....);
  Assert(callSiteId >= 0);
  ....
}

В этом и ещё двух других местах анализатор обнаружил некорректное сравнение беззнакового числа с нулём:

  • V547 Expression 'callSiteId >= 0' is always true. Unsigned type value is always >= 0. inline.cpp 2627
  • V547 Expression 'callSiteId >= 0' is always true. Unsigned type value is always >= 0. inline.cpp 3657

Заключение


У Microsoft замечается позитивная тенденция открывать свои проекты под свободными лицензиями. Для нас это дополнительное тестирование анализатора на новых проектах, а также возможность продемонстрировать полезность и эффективность статического анализа на проектах такого крупного и известного производителя программного обеспечения.

Возможно, вам будет интересен список всех проверенных проектов, включающих и другие проекты от Microsoft, таких как .NET CoreCLR, .NET CoreFX и Microsoft Code Contracts.



Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. ChakraCore: analysis of JavaScript-engine for Microsoft Edge.

Прочитали статью и есть вопрос?
Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
Автор: @SvyatoslavMC
PVS-Studio
рейтинг 237,04
Скачай PVS-Studio и найди ошибки в С, С++,C# коде

Комментарии (25)

  • –3
    github.com/Microsoft/ChakraCore/pulls
    Думаю, будет полезно всем заинтересованным, в том числе и вам как компании, как считаете?
    • +10
      Самим лучше не исправлять, а уведомить об этом разработчиков. Но ребята уже сделали это.
      • +5
        Действительно, исправлять ошибки должны ТОЛЬКО разработчики проекта. Увы, уже были случаи, когда сторонний человек вдохновившись нашей статьей, успевал «наломать дров».
  • 0
    Интересно почему при разработке продукта не используются подобные инструменты. Даже если он платный, неужели у компании нет средств на покупку лицензии на команду, чтобы улучшить качество продукта и облегчить жизнь разработчикам.
    • +1
      Не все команды готовы вкладываться в качество кода. Это определенный уровень развития процессов разработки. Команда (ну к примеру ее тим-лид, если говорить конкретно) должна задать вопрос: «А что ЕЩЕ мы можем сделать для того, чтобы наш код был более качественный?» И тогда ответом будет: «Например, использовать статический анализатор кода».
      • 0
        Вы правы. Просто это как бы Microsoft, а не какие-то студенты) Думал у них требования к разработке жестче и процессы более налажены.
        • +9
          На самом деле у них очень качественный код. Просто в серьезном проекте всегда можно что-то найти.
          • +2
            И от размера зависит. Чем больше размер проекта, тем больше там ошибок. Люди ошибаются, увы.
    • 0
      Возможно статический анализатор у них есть, но правила настроены по другому.
      И не всегда статический анализ показывает на ошибки. Иногда просто на сложный(но возможно быстрый) код.
  • –3
    Если сравнивать результаты анализатора, то как на ваш взгляд — на фоне других открытых проектов поделие от Микрософт выглядит лучше или хуже?
    • +3
      Лучше. Часто написать статью вообще не о чем. :) Пример: www.viva64.com/ru/b/0189
    • +1
      Если сравнить результаты, то по собственному опыту могу сказать, что если в проекте не использовались инструменты статического анализа, то там с большой вероятностью есть ошибки. В любом проекте.

      Мы проверяли проекты, которые были всегда отрытыми, и писали статьи. Проверяли только что открытые проекты Microsoft и других компаний, и также пишем статьи.
  • 0
    Объясните, зачем анализатор выдает предупреждения на сравнение беззнаковых чисел в assert-ах? assert стоит для того, чтобы гарантировать, что условие не нарушится, если вдруг переменная, участвующая в assert-е поменяет тип на знаковый. Смысл выдавать здесь предупреждение?
    • +3
      Вообще во многих случаях PVS-Studio молчит, когда встречает assert-ы. Есть десятки исключений на эту тему. Но иногда есть смысл придираться и к ним.

      Эти assert ничего не проверяют. Возможно, это не страшно. Быть может даже assert специально задуман так, что сработает в тот момент, когда беззнаковую переменную заменят на знаковую и вычисления пойдут другим способом.

      Но в 90% случаев это просто ошибка и человек написал assert, который не выполняет своего предназначения.

      Рассмотрим случай, приводимый в статье:

      AssertMsg(srcIndex - src->left >= 0,
          "src->left > srcIndex resulting in \
           negative indexing of src->elements");
      

      Человек хотел защитится assert-ом от неправильного индекса. Лучше поймать assert(), чем разбираться почему программа упадёт при вызове последующей функции js_memcpy_s.

      Но на самом деле assert() ничего не проверяет. Условие всегда истинно. Ассерт не сработает, функция сгенерирует исключения. Таким образом программист не добился своей цели, когда писал assert().
      • +1
        Да, тут я упустил из виду, что проверяется не то условие, которое подразумевается. Но в других ваших статьях часто указывается как ошибка проверка какой-либо беззнаковой переменной (а не выражения, как здесь) в assert-е на >= 0. Вот в этих случаях мне кажется, что предупреждение выдавать не нужно. Так можно и юнит тесты (да и вообще все тесты) выкидывать — а зачем они, если анализ показывает, что ошибок нет… пока нет и пока анализатор запускается.
        • 0
          Да, бывают ложные срабатывания. Но часто имелось совсем не то в виду, что получилось.

          Например, функция возвращает -1 в случае ошибки. Предполагается, что такой ситуации в программе не будет, но решают написать assert() на всякий случай. В результате имеем:

          unsigned i = foo();
          assert(i >= 0);


          Вновь assert() не выполняет назначенной для себя функции.

          И посмотрите, сколько блестящих фрагментов находит диагностика V547! Так что лучше ругаться :). Это ведь шедевры:

          inline void CXmlReader::CXmlInputStream::UnsafePutCharsBack(
            const TCHAR* pChars, size_t nNumChars)
          {
            if (nNumChars > 0)
            {
              for (size_t nCharPos = nNumChars - 1;
                   nCharPos >= 0;
                   --nCharPos)
                UnsafePutCharBack(pChars[nCharPos]);
            }
          }
          


          • 0
            Например, функция возвращает -1 в случае ошибки.

            В таком случае достаточно ругани на то, что знаковая переменная записывается в беззнаковую (а надеюсь, такое есть?), именно тут создается ошибка.
            И посмотрите, сколько блестящих фрагментов находит диагностика V547! Так что лучше ругаться :). Это ведь шедевры:

            Так я не спорю, что в обычных фрагментах кода нужно выдавать это предупреждение. Я говорю, что в assert-ах оно неуместно, когда у нас сценарий assert(unsignedVar >= 0). В таком сценарии я вижу только подстраховку на тот случай, если тип станет знаковым, ну и явно подчеркнуть намерение в коде, что отрицательных чисел нет (так как глядя на текст, тип переменной может быть не видно).
            • 0
              В таком случае достаточно ругани на то, что знаковая переменная записывается в беззнаковую (а надеюсь, такое есть?)

              Ну конечно же нет. Если сделать в лоб такую диагностику, анализатор можно сразу выкидывать. Количество бессмысленных сообщений зашкалит. Сколько угодно можно говорить, что это плохой код. Но количество таких преобразований в коде столь велико, что ругаться на это бессмысленно. Компиляторам и анализаторам приходится молчать куда на более бесшабашные вещи, например:

              char *a = "foo";

              Этот код вообще компилироваться не должен, так как «foo» это константная строка, а мы помещаем её в неконтактную. Но столько кода уже понаписано, что поздно пить боржоми.

              • 0
                char *a = "foo";
                

                Это все же немного другой пример. Тут информация не теряется и не меняется, в отличие от молчаливого приведения типа из знакового в беззнаковый (и наоборот, а также усекания более крупных типов). Фактически, единственное, что создает такой код — перекладывает часть проверок на из compile-time на runtime (вместо ругани компилятора получим Aссess violation при попытке записать в такой указатель).

                Кроме того, не вижу, почему бы не добавить такую диагностику. Если код проверяют, значит, его пишут, и значит, руки не отвалятся дописать несчастные 6 символов. А если либа настолько древняя, что в нее уже не заглядывают, то сомневаюсь, что ее проверяют статическим анализатором, там же наверняка не только такие вещи найдутся. А если их все равно править никто не собирается, то и анализировать безсмысленно.

                Но количество таких преобразований в коде столь велико, что ругаться на это бессмысленно.

                Но ведь это такие грабли могут быть скрыты! Как это безсмысленно ругаться? На неявные стоит ругаться (а современные компиляторы так и делают, к счастью). А особенно стоит, если есть подозрение, что функция возвращает код ошибки. Кстати, предложение для новой диагностики: если функция возвращает знаковый тип, который сохраняется в беззнаковую переменную, которая в свою очередь потом проверяется на >= 0 — полагать, что она возвращает отрицательные коды ошибок и ругаться именно на место сохранения результата (в дополнение к ругани на безсмысленность сравнения >= 0, за исключением случаев assert-ов, как я писал выше).
  • 0
    V595 The 'instrLd' pointer was utilized before it was verified against nullptr. Check lines: 1823, 1831. flowgraph.cpp 1823
    IR::Instr *
    FlowGraph::PeepTypedCm(IR::Instr *instr)
    {

    if (instrLd && !instrLd->GetSrc1()->IsEqual(instr->GetDst()))
    {
    return nullptr;
    }

    if(instrLd2 && !instrLd2->GetSrc1()->IsEqual(instrLd->GetDst()))
    {
    return nullptr;
    }

    }

    А разве тут есть ошибка? Ведь, если instrLd == null, то сработает еще первое условие и произойдет return nullptr, до второго даже не дойдет. Или речь про instr?
    • +3
      Не сработает. Если instrLd == nullptr то в случае когда instrLd2 != nullptr вызовется instrLd->GetDst() — вот тут и проблема. Все-таки сложно без анализатора находить такие места, ага.
      • 0
        А, туплю, первый if неправильно понял.
  • 0
    • 0
      Еще не смерджили
    • 0
      Они ещё не видели, что в CNTK нашлось..., но всему своё время ;-)

Только зарегистрированные пользователи могут оставлять комментарии. Войдите, пожалуйста.

Самое читаемое Разработка