Андрей Карпов

14 Ноя 2013

Теги:

#Cpp

Продолжение проверки Geant4

14 Ноя 2013

Автор: Андрей Карпов

Краткое содержание предыдущей статьи
Новые подозрительные фрагменты кода
Заключение

Написал правильную статью про проверку проекта Geant4. Напомню предысторию. Недавно была проверена старая версия библиотеки Geant4 и написана статья "Copy-Paste и мюоны". Почему была проверена старая версия? Люди не совершенны. Суть оплошности можно узнать в предыдущей статье. Теперь же вашему вниманию предлагается краткий отчет о проверке Geant4 версии 10.0-beta.

Краткое содержание предыдущей статьи

В статье "Copy-Paste и мюоны" рассказывается о пользе использования методологии статического анализа и возможностях PVS-Studio. Был проверен бородатый проект Geant4 версии 4.9.4. Было найдено множество подозрительных фрагментов кода, которые и были описаны статье.

Geant4 это программа для моделирования прохождения элементарных частиц через вещество с использованием методов Монте-Карло. Разработана в CERN на объектно-ориентированном языке программирования С++. Является дальнейшим развитием предыдущих версий GEANT, существенно переработанным и дополненным.

Сайт проекта: http://geant4.org.

В предыдущей статье было описано 16 крайне подозрительных фрагментов кода. Новая проверка выявила только 10 из них. Остальные видимо или исправлены, или этого кода больше не существует. Я не буду вновь описывать в новой статье эти фрагменты кода. Их можно увидеть в предыдущей статье. В ней помечено, исчезло предупреждение анализатора или нет в новой версии.

Прошу прощения, что получился такой странный формат исследования. Но надеюсь, это не помешает исправить ряд недочетов в проекте Geant4 и привлечь внимание людей к инструменту PVS-Studio.

Если я не ошибаюсь, предыдущая версия библиотеки относится к 2011 году. За это время многое изменилось, и не удивительно, что найдены некоторые новые странные места в коде. Давайте посмотрим, что есть новенького или то, что я не заметил ранее.

Новые подозрительные фрагменты кода

Полный список подозрительных мест, на которые я обратил внимание, находится в файле geant4_new.txt. Хочу подчеркнуть, что не стоит полагаться исключительно на этот список. Намного лучше, будет проверить проект самостоятельно и проанализировать все предупреждения. Мы готовы предоставить для этого на некоторое время ключ разработчикам Geant4: форма обратной связи.

Одинаковые функции

G4double G4CsvAnalysisManager::GetH2Xmin(G4int /*id*/) const
{
  ExceptionForHistograms("GetH2Xmin");
  return 0;
}

G4double G4CsvAnalysisManager::GetH2Xmax(G4int /*id*/) const
{
  ExceptionForHistograms("GetH2Xmin");
  return 0;
}

Предупреждение PVS-Studio: V524 It is odd that the body of 'GetH2Xmax' function is fully equivalent to the body of 'GetH2Xmin' function. _G4analysis-archive g4csvanalysismanager.cc 933

Функция GetH2Xmax(), по всей видимости, должна вызывать функция ExceptionForHistograms() с другим параметром:

ExceptionForHistograms("GetH2Xmax");

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

Энергия равна нулю

Функции CalculateTotalEnergy () суммирует значения в переменной 'Etot'. Неожиданно, эта переменная обнуляется. Это очень подозрительно.

G4double G4RKFieldIntegrator::CalculateTotalEnergy(const
  G4KineticTrackVector& Barions)
{
  G4double Etot = 0;
  ....
  for(G4int c2 = c1 + 1; c2 < nBarion; c2++)
  {  
    ....
    //  Esk2
    Etot += t1*std::pow(Alpha/pi, 3/2)*
            std::exp(-Alpha*r12*r12);

    // Eyuk
    Etot += ....;

    // Ecoul
    Etot += 1.44*p1->GetDefinition()->GetPDGCharge()*
            p2->GetDefinition()->GetPDGCharge()/r12*
            Erf(std::sqrt(Alpha)*r12);

    // Epaul
    Etot = 0;
    ....
  }
  ....
}

Предупреждение PVS-Studio: V519 The 'Etot' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 80, 83. _G4processes-archive g4rkfieldintegrator.cc 83

Подозрительная логика

G4double 
G4EmBiasingManager::ApplySecondaryBiasing(....)
{
  ....
  if(0 == nsplit) { 
    if(safety > fSafetyMin) ....
  } if(1 == nsplit) { 
    weight = ApplyRussianRoulette(vd, index);
  } else {
    G4double tmpEnergy = pPartChange->GetProposedKineticEnergy();
    G4ThreeVector tmpMomDir = ....
    weight = ApplySplitting(vd, track, currentModel, index, tcut);
    pPartChange->SetProposedKineticEnergy(tmpEnergy);
    pPartChange->ProposeMomentumDirection(tmpMomDir);
  }
  ....
}

Предупреждение PVS-Studio: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. _G4processes-archive g4embiasingmanager.cc 299

Код оформлен так, как будто используется конструкция "else if". Но это не так. Если оформить код правильно, то получим вот это:

if(0 == nsplit) { 
  if(safety > fSafetyMin) ....
}

if(1 == nsplit) { 
  weight = ApplyRussianRoulette(vd, index);
} else {
  G4double tmpEnergy = pPartChange->GetProposedKineticEnergy();
  G4ThreeVector tmpMomDir = ....
  weight = ApplySplitting(vd, track, currentModel, index, tcut);
  pPartChange->SetProposedKineticEnergy(tmpEnergy);
  pPartChange->ProposeMomentumDirection(tmpMomDir);
}

Обратите внимание, что блок, относящийся к ветке 'else' выполняется всегда, если "1 != nsplit". Есть подозрение, что задумывалась иная логика работы.

Аналогичную ситуацию можно увидеть здесь:

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. _G4processes-archive g4embiasingmanager.cc 347

Недописанный код?

void G4MolecularDecayTable::AddExcitedState(const G4String& label)
{
  channelsMap::iterator channelsIter =
    fDecayChannelsMap.find(label);
  if(channelsIter != fDecayChannelsMap.end())
  {
    G4String errMsg = "Excited state" + label +
                      " already registered in the decay table.";
    G4Exception("G4MolecularDecayTable::AddExcitedState",
                "G4MolecularDecayTable003",
                FatalErrorInArgument, errMsg);
     return;
  }
  fDecayChannelsMap[label] ;
}

Предупреждение PVS-Studio: V607 Ownerless expression 'fDecayChannelsMap[label]'. _G4processes-archive g4moleculardecaytable.cc 140

Конец функции выглядит очень подозрительно:

fDecayChannelsMap[label] ;

Что это? Здесь чего-то не хватает? Что хотели сделать с ячейкой массива?

Кинематика

Следующий пример достаточно длинный. К сожалению, мне трудно сократить его ещё больше. Посмотрите, какие значения может принимать переменная 'id'.

void G4QMDCollision::CalKinematicsOfBinaryCollisions(
  G4double dt)
{
  ....
  G4int id = 0;
  ....
  if ( secs )
  {
    ....
    id++;
    ....
  }
  if ( std::abs ( eini - efin ) < fepse*10 ) 
    ....
  else
  {  
    ....             
    for ( G4int i0i = 0 ; i0i < id-1 ; i0i++ )
    {
      theSystem->DeleteParticipant( i0i+n0 );
    }
    ....
  }
  ....
}

Предупреждение PVS-Studio: V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. _G4processes-archive g4qmdcollision.cc 228

Если условие "if ( secs )" не выполнится, то переменная 'id' останется равна нулю. Тогда, может возникнуть цикл вида:

for ( G4int i0i = 0 ; i0i < -1 ; i0i++ )

Это будет очень странный цикл. Думаю, стоит проверить логику работы функции CalKinematicsOfBinaryCollisions().

Прочее

Есть ещё ряд предупреждений, которые я не описывал в прошлой статье. Не буду и в этой. Для примера, покажу один случай:

class G4HadronicException : public std::exception
{
  ....
};

inline G4double G4GeneralPhaseSpaceDecay::Pmx(
  G4double e, G4double p1, G4double p2)
{
   if (e-p1-p2 < 0 )
   {  
    G4HadronicException(__FILE__, __LINE__,
      "G4GeneralPhaseSpaceDecay::Pmx "
      "energy in cms > mass1+mass2");
   }
  ....
}

Предупреждение PVS-Studio. V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw G4HadronicException(FOO); _G4processes-archive g4generalphasespacedecay.hh 116

Есть несколько ошибок, когда забыт оператор 'throw'. В результате, создаётся и тут же уничтожается объект, имеющий тип 'G4HadronicException'. И дальше программа продолжает работать с некорректными данными.

Вот такие опечатки, неописанные в статье, можно найти в файле geant4_new.txt. Там же приводятся предупреждения, относящиеся к микрооптимизациям.