Главная
Блог разработчиков phpBB
 
+ 17 предустановленных модов
+ SEO-оптимизация форума
+ авторизация через соц. сети
+ защита от спама

Идем по грибы позже Cppcheck

Anna | 24.06.2014 | нет комментариев

PVS-Studio, OpenMS

Позже жгучих обсуждений про “Огромный Калькулятор“, мне захотелось проверить ещё что-то из планов, связанных с проведением изысканий. Первое что нашлось, оказался открытый план OpenMS, связанный с protein mass spectrometry. Данный план оказалось написан с серьёзным подходом. При разработке применяется как минимум Cppcheck. Так что ничего сенсационного ожидать не доводилось. Впрочем был интерес, какие ошибки сумеет обнаружить PVS-Studio позже Cppcheck. Заинтересовавшихся приглашаю продолжить чтение статьи.

Существует план OpenMS. Для чего он необходим, я не берусь пересказать своими словами. Ещё ляпну бессмысленность. Легко процитирую фрагмент изложения с сайта Wikipedia:

OpenMS is an open-source project for data analysis and processing in protein mass spectrometry and is released under the 2-clause BSD licence. OpenMS has tools for many common data analysis pipelines used in proteomics, providing algorithms for signal processing, feature finding (including de-isotoping), visualization in 1D (spectra or chromatogram level), 2D and 3D, map mapping and peptide identification. It supports label-free and isotopic-label based quantification (such as iTRAQ and TMT and SILAC). Furthermore, it also supports metabolomics workflows and DIA/SWATH targeted analysis.

Первоисточник: Wikipedia. OpenMS

План среднего размера, но крайне трудный. Размер начального кода составляет больше 20 мегабайт. Плюс огромное число сторонних библиотек (Boost, Qt, Zlib и так дальше). В плане дюже энергично применяются образцы. Начальный код плана дозволено скачать с сайта SourceForge.

При разработке плана OpenMS используется статический обзор кода. Присутствие «cppcheck.cmake» и комментариев в духе:

if (i != peptide.size()) // added for cppcheck

свидетельствует, что применяются как минимум Cppcheck. Также встречается припоминание Cpplint и есть файл «cpplint.py». Высокопрофессиональный подход. Молодцы.

Давайте сейчас посмотрим, что удастся обнаружить с поддержкой анализатора PVS-Studio.

Примечание. Почему-то Си файлы носят растяжение ‘*.c’. Так что пускай вас не смущает, когда вы увидите пример кода на Си , находящийся в ‘*.c’ файле.

1. Недочёты с OpenMP

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

Среди выданных предупреждений есть ложные срабатывания, но нашлись предупреждения и по делу.

DoubleReal ILPDCWrapper::compute(....) const
{
  ....
  DoubleReal score = 0;
  ....
  #pragma omp parallel for schedule(dynamic, 1)
  for (SignedSize i = 0; i < (SignedSize)bins.size();   i)
  {
    score  = computeSlice_(fm, pairs, bins[i].first,
                           bins[i].second, verbose_level);
  }
  return score;
}

Предупреждение PVS-Studio: V1205 Data race risk. Unprotected concurrent operation with the ‘score’ variable. ilpdcwrapper.c 213

Ненормально считается сумма. Переменная ‘score’ не защищена от одновременного применения в различных потоках.

Другие примечания не столько критичны, но всё же на них стоит обратить внимание. Внутри параллельных сегментов все исключения обязаны быть перехвачены. Выход исключения за пределы параллельной сегменты, скорее каждого, приведет к аварийному заключению программы. Подробнее про эту тему дозволено прочитать в заметках: “OpenMP и исключения (exceptions)“, “Обработка исключений внутри параллельных сегментов“.

Исключение дозволено сгенерировать очевидно, применяя оператор throw. Также оно может появиться при вызове оператора new (std::bad_alloc).

1-й вариант. Функция getTheoreticalmaxPosition() может сгенерировать исключение.

Size getTheoreticalmaxPosition() const
{
  if (!this->size())
  {
    throw Exception::Precondition(__FILE__, __LINE__,
      __PRETTY_FUNCTION__,
      "There must be at least one trace to ......");
  }
  ....
}

virtual void run()
{
  ....
  #pragma omp parallel for
  for (SignedSize i = 0; i < (SignedSize)seeds.size();   i)
  {
    ....
    f.setMZ(
      traces[traces.getTheoreticalmaxPosition()].getAvgMZ());
    ....
  }
  ....
} 

Предупреждение PVS-Studio: V1301 The ‘throw’ keyword cannot be used outside of a try..catch block in a parallel section. featurefinderalgorithmpickedhelperstructs.h 199

2-й вариант. Вызов оператор ‘new’ может привести к происхождению исключения.

TraceFitter<PeakType>* chooseTraceFitter_(double& tau)
{
  // choose fitter
  if (param_.getValue("feature:rt_shape") == "asymmetric")
  {
    LOG_DEBUG << "use asymmetric rt peak shape" << std::endl;
    tau = -1.0;
    return new EGHTraceFitter<PeakType>();
  }
  ....
}

virtual void run()
{
  ....
  #pragma omp parallel for
  for (SignedSize i = 0; i < (SignedSize)seeds.size();   i)
  {
    ....
    TraceFitter<PeakType>* fitter = chooseTraceFitter_(egh_tau);
    ....
  }
  ....
}

Предупреждение PVS-Studio: V1302 The ‘new’ operator cannot be used outside of a try..catch block in a parallel section. featurefinderalgorithmpicked.h 1926

Другие схожие предупреждения:

  • V1301 featurefinderalgorithmpicked.h 1261
  • V1301 mzmlfile.h 114
  • V1301 rawmssignalsimulation.c 598
  • V1301 rawmssignalsimulation.c 1152
  • V1301 chromatogramextractor.h 103
  • V1301 chromatogramextractor.h 118
  • V1302 featurefinderalgorithmpicked.h 1931
  • V1302 rawmssignalsimulation.c 592
  • V1302 rawmssignalsimulation.c 601
  • V1302 openswathanalyzer.c 246

2. Опечатки

std::vector< std::pair<std::string, long> > spectra_offsets;
std::vector< std::pair<std::string, long> > chromatograms_offsets;

template <typename MapType>
void MzMLHandler<MapType>::writeFooter_(std::ostream& os)
{
  ....
  int indexlists;
  if (spectra_offsets.empty() && spectra_offsets.empty() )
  {
    indexlists = 0;
  }
  else if (!spectra_offsets.empty() && !spectra_offsets.empty() )
  {
    indexlists = 2;
  }
  else
  {
    indexlists = 1;
  }
  ....
}

Предупреждения PVS-Studio:

V501 There are identical sub-expressions ‘spectra_offsets.empty()’ to the left and to the right of the ‘&&’ operator. mzmlhandler.h 5288

V501 There are identical sub-expressions ‘!spectra_offsets.empty()’ to the left and to the right of the ‘&&’ operator. mzmlhandler.h 5292

Дюже необычные проверки. Два раза проверяется контейнер ‘spectra_offsets’. Скорее каждого, это опечатка и следует проверять два различных контейнера: spectra_offsets и chromatograms_offsets.

template <typename MapType>
void MzMLHandler<MapType>::characters(
  const XMLCh* const chars, const XMLSize_t)
{
  ....
  if (optionalAttributeAsString_(data_processing_ref,
                                 attributes,
                                 s_data_processing_ref))
  {
    data_.back().meta.setDataProcessing(
                        processing_[data_processing_ref]);
  }
  else
  {
    data_.back().meta.setDataProcessing(
                        processing_[data_processing_ref]);
  }
  ....
}

Предупреждение PVS-Studio: V523 The ‘then’ statement is equivalent to the ‘else’ statement. mzmlhandler.h 534

Если посмотреть на схожие фрагменты кода, то дозволено сделать итог, что нужно было применять:

  • processing_[data_processing_ref]
  • processing_[default_processing_]

Много опечаток оказалось связано с генерацией исключений. Ошибки тривиальны. Позабыто ключевое слово ‘throw’. Легко создается непостоянный объект и сразу уничтожается. Пример такой ошибки:

inline UInt asUInt_(const String & in)
{
  UInt res = 0;
  try
  {
    Int tmp = in.toInt();
    if (tmp < 0)
    {
      Exception::ConversionError(
        __FILE__, __LINE__, __PRETTY_FUNCTION__, "");
    }
    res = UInt(tmp);
  }
  catch (Exception::ConversionError)
  {
    error(LOAD, 
          String("UInt conversion error of "")   in   """);
  }
  return res;
}

Предупреждение PVS-Studio: V596 The object was created but it is not being used. The ‘throw’ keyword could be missing: throw ConversionError(FOO); xmlhandler.h 247

Одинаковые опечатки тут:

  • inclusionexclusionlist.c 281
  • inclusionexclusionlist.c 285
  • precursorionselectionpreprocessing.c 257
  • modificationsdb.c 419
  • modificationsdb.c 442
  • svmtheoreticalspectrumgeneratorset.c 103
  • logconfighandler.c 285
  • logconfighandler.c 315
  • suffixarraytrypticcompressed.c 488
  • tooldescription.c 147
  • tofcalibration.c 147

И последняя подмеченная мною опечатка:

inline typename Value<Pipe>::Type const & operator*() {
  tmp.i1 = *in.in1;
  tmp.i2 = *in.in2;
  tmp.i3 = *in.in2;
  return tmp;
}

Предупреждение PVS-Studio: V525 The code containing the collection of similar blocks. Check items ‘in1′, ‘in2′, ‘in2′ in lines 112, 113, 114. pipe_joiner.h 112

Должно быть написано:

tmp.i1 = *in.in1;
tmp.i2 = *in.in2;
tmp.i3 = *in.in3;

3. Необычное условие

CompressedInputSource::CompressedInputSource(
  const String & file_path, const char * header,
  MemoryManager * const manager) 
  : xercesc::InputSource(manager)
{
  if (sizeof(header) / sizeof(char) > 1)
  {
    head_[0] = header[0];
    head_[1] = header[1];
  }
  else
  {
    head_[0] = '';
    head_[1] = '';
  }
  ....
}

Предупреждение PVS-Studio: V514 Dividing sizeof a pointer ‘sizeof (header)’ by another value. There is a probability of logical error presence. compressedinputsource.c 52

Если мы поделим размер указателя на размер байта, то неизменно получим значение огромнее единицы. По крайней мере, я не знаю затейливую архитектуру, где это не так. Так что тут какая-то оплошность.

Аналогичная необычная проверка имеется тут: compressedinputsource.c 104

4. Возвращение ссылки на локальный объект

template <typename TStringSet, typename TSpec>
inline Iter<TStringSet, ConcatVirtual<TSpec> > const &
operator  (Iter<TStringSet, ConcatVirtual<TSpec> > & me, int)
{
    Iter<TStringSet, ConcatVirtual<TSpec> > before = me;
    goNext(me);
    return before;
}

Предупреждение PVS-Studio: V558 Function returns the reference to temporary local object: before. iter_concat_virtual.h 277

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

Мне кажется, нужно было не создавать новейший итератор, а сберечь ссылку:

Iter<TStringSet, ConcatVirtual<TSpec> > &before = me;

Аналогичная напасть имеется у оператора ‘–’: iter_concat_virtual.h 310

5. Неаккуратные вычисления

typedef size_t Size;
typedef double DoubleReal;
void updateMeanEstimate(const DoubleReal & x_t,
  DoubleReal & mean_t, Size t)
{
  DoubleReal tmp(mean_t);
  tmp = mean_t   (1 / (t   1)) * (x_t - mean_t);
  mean_t = tmp;
}

Предупреждение PVS-Studio: V636 The ’1 / (t 1)’ expression was implicitly casted from ‘int’ type to ‘double’ type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. masstracedetection.c 129

Выражение “(1 / (t 1))” неизменно равно нулю либо единицы. Это связано с тем, что это выражение целочисленное. Допустимо, задумывалось получить вовсе другой итог иное. Я не знаю логику работы программы, но мне кажется, имелось в виду следующее:

tmp = mean_t   (1.0 / (t   1)) * (x_t - mean_t);

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

bool PosteriorErrorProbabilityModel::fit(
  std::vector<double> & search_engine_scores)
{
  ....
  incorrectly_assigned_fit_param_.A =
    1 / sqrt(2 * 3.14159 *
             pow(incorrectly_assigned_fit_param_.sigma, 2));
  ....
}

Предупреждение PVS-Studio: V624 The constant 3.14159 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. posteriorerrorprobabilitymodel.c 92

Подобно:

  • posteriorerrorprobabilitymodel.c 101
  • posteriorerrorprobabilitymodel.c 110
  • posteriorerrorprobabilitymodel.c 155
  • posteriorerrorprobabilitymodel.c 162

6. Выход за рубеж массива

static const Int CHANNELS_FOURPLEX[4][1];
static const Int CHANNELS_EIGHTPLEX[8][1];
ExitCodes main_(int, const char **)
{
  ....
  if (itraq_type == ItraqQuantifier::FOURPLEX)
  {
    for (Size i = 0; i < 4;   i)
    {
      std::vector<std::pair<String, DoubleReal> > one_label;
      one_label.push_back(std::make_pair<String, DoubleReal>(
        String("Channel ")  
          String(ItraqConstants::CHANNELS_FOURPLEX[i][0]),
        DoubleReal(ItraqConstants::CHANNELS_FOURPLEX[i][0])));
      labels.push_back(one_label);
    }
  }
  else //ItraqQuantifier::EIGHTPLEX
  {
    for (Size i = 0; i < 8;   i)
    {
      std::vector<std::pair<String, DoubleReal> > one_label;
      one_label.push_back(std::make_pair<String, DoubleReal>(
        String("Channel ")  
          String(ItraqConstants::CHANNELS_FOURPLEX[i][0]),
        DoubleReal(ItraqConstants::CHANNELS_FOURPLEX[i][0])));
      labels.push_back(one_label);
    }
  }
  ....
}

Предупреждение PVS-Studio: V557 Array overrun is possible. The value of ‘i’ index could reach 7. itraqanalyzer.c 232

На самом деле, эту ошибку дозволено было отнести к ошибкам Copy-Paste. Но пускай уж будет «выход за рубеж массива». Так звучит больше пугающе. Да и вообще, это деление крайне условно. Одну и ту же ошибку дозволено систематизировать по-различному.

Тут, в ветке ‘else’ нужно было трудиться с массивов ‘CHANNELS_EIGHTPLEX’. Об этом свидетельствует комментарий:

else //ItraqQuantifier::EIGHTPLEX

Впрочем, скопированный фрагмент кода, изменили не до конца. В итоге там применяется массив CHANNELS_FOURPLEX, тот, что имеет меньший размер.

Аналогичная оплошность тут (ещё один Copy-Paste): tmtanalyzer.c 225

Разглядим ещё один пример.

DoubleReal masse_[255]; ///< mass table

EdwardsLippertIterator::EdwardsLippertIterator(const
 EdwardsLippertIterator & source) :
  PepIterator(source),
  f_file_(source.f_file_),
  actual_pep_(source.actual_pep_),
  spec_(source.spec_),
  tol_(source.tol_),
  is_at_end_(source.is_at_end_),
  f_iterator_(source.f_iterator_),
  f_entry_(source.f_entry_),
  b_(source.b_),
  e_(source.e_),
  m_(source.m_),
  massMax_(source.massMax_)
{
  for (Size i = 0; i < 256; i  )
  {
    masse_[i] = source.masse_[i];
  }
}

Предупреждение PVS-Studio: V557 Array overrun is possible. The value of ‘i’ index could reach 255. edwardslippertiterator.c 134

В конструкторе копирования ненормально работают с массивом masse_. Он состоит из 255 элементов. А копируется 256 элементов.

Верный цикл:

for (Size i = 0; i < 255; i  )
{
  masse_[i] = source.masse_[i];
}

А ещё отменнее вообще не применять волшебные константы.

7. Устаревший вызов оператора ‘new’

svm_problem * LibSVMEncoder::encodeLibSVMProblem(....)
{
  ....
  node_vectors = new svm_node *[problem->l];
  if (node_vectors == NULL)
  {
    delete[] problem->y;
    delete problem;
    return NULL;
  }
  ....
}

Предупреждение PVS-Studio: V668 There is no sense in testing the ‘node_vectors’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. libsvmencoder.c 177

Проверка «if (node_vectors == NULL)» не имеет смысла. Если не удастся выделить память, возникнет исключение. В итоге программа поведет себя не так, как планировал программист. Скажем, произойдет утрата памяти.

Схожие устаревшие провреки:

  • file_page.h 728
  • libsvmencoder.c 160

Завершение

Я думаю, будет благотворно применять не только Cppcheck, Cpplint, но PVS-Studio. Исключительно, если делать это регулярно. Приглашаю разработчиков плана написать нам на support@viva64.com. На некоторое время мы выделим ключ, для больше подробной проверки OpenMS.

 

Источник: programmingmaster.ru

 

Оставить комментарий
Форум phpBB, русская поддержка форума phpBB
Рейтинг@Mail.ru 2008 - 2017 © BB3x.ru - русская поддержка форума phpBB