Вести с полей

На сайт загружена новая тестовая сборка IBProvider — v3.55.0.29115.

В неё, в том числе, вошли результаты борьбы с предупреждениями PVS-Studio.

95% это замена assert на assert_hint.

//-V:assert_hint:779, 547
#define assert_hint(cond) assert(cond);

Из этих 95% большая часть — в моей переработанной версии libtommath, которую перед тем как затащить в проект пришлось перетряхнуть и нафаршировать ассертами.

Для остальных 5% пришлось напрячься.

Самый мутный случай в этой самой libtommath — функция mp_div. PVS говорит что в строке 361 условие всегда false:

t2.dp[0] = (i < 2) ? 0 : x.dp[i - 2];
t2.dp[1] = (i < 1) ? 0 : x.dp[i - 1]; // [2018-12-24] Always = x.dp[i - 1];
t2.dp[2] = x.get_safe(i);
t2.used  = 3;

И судя по моим выводам, это действительно так. Сам код пока не трогал, но добавил ассерты, которые это подтверждают. Будем посмотреть на работу отладочной сборки провайдера под тестами.

Остальные изменения можно описать так — «пишите просто, еще проще». Например.

Было:

const wchar_t* TDBID::GetUnicodeNamePtr(const DBID& dbid,bool NotNull)
{
 const wchar_t* ptr=NULL;

 if(self_type::HasName(dbid))
  ptr=dbid.uName.pwszName;

 if(NotNull && ptr==NULL)
  return L"";

 return ptr;
}//GetUnicodeNamePtr

Стало:

const wchar_t* TDBID::GetUnicodeNamePtr(const DBID& dbid,bool const NotNull)
{
 if(!self_type::HasName(dbid))
 {
  if(NotNull)
   return L"";

  return structure::null_ptr;
 }//if no name

 if(NotNull)
 {
  if(!dbid.uName.pwszName)
   return L"";

  assert_hint(dbid.uName.pwszName);
 }//if NotNull

 return dbid.uName.pwszName;
}//GetUnicodeNamePtr

PVS спотыкался об локальную переменную ptr и говорил, что в коде

   if(wcscmp(self_type::GetUnicodeNamePtr(*pid1,/*NotNULL*/true),
             self_type::GetUnicodeNamePtr(*pid2,/*NotNULL*/true))!=0)
   {
    return false;
   }

возможна передача нулевых указателей в wcscmp. Не рюхает он такие косвенные вещи.

Второй пример. Метод чтения из storage-объекта.
Было:

template<class Allocator>
HRESULT __stdcall basic_dbvariant_sstream<Allocator>::Read(void*  const pv,
                                                           ULONG        cb,
                                                           ULONG* const pcbRead)
{
 OLE_LIB_IMETHOD_PROLOG

 ::SetErrorInfo(0,structure::null_ptr);

 if(pcbRead!=NULL)
  (*pcbRead)=0;

 if(pv==NULL && cb!=0)
  return E_INVALIDARG;

 OLE_LIB__DECLARE_HR(S_OK);

 _OLE_TRY_
 {
  const lock_guard_type __lock__(m_guard);

  assert(m_pos<=m_size);

  const size_type cb_tail=size_type(m_size-m_pos);

  if(cb_tail==0)
  {
   hr=S_FALSE;
  }
  else
  {
   assert_hint(cb_tail>0);

   if(cb_tail<cb)
    structure::static_numeric_cast(&cb,cb_tail);

   assert(cb<=cb_tail);

   if(cb>0)
   {
    memcpy(pv,&m_data[m_pos],cb);

    m_pos+=cb;

    if(pcbRead)
     (*pcbRead)=cb;
   }//if cb>0

   assert_hint(hr==S_OK);
  }//else
 }
 _OLE_CATCHES2_CODE_

 assert(hr==S_OK || hr==S_FALSE || FAILED(hr));

 return hr;
}//Read

Стало:

template<class Allocator>
HRESULT __stdcall basic_dbvariant_sstream<Allocator>::Read(void*  const pv,
                                                           ULONG        cb,
                                                           ULONG* const pcbRead)
{
 OLE_LIB_IMETHOD_PROLOG

 ::SetErrorInfo(0,structure::null_ptr);

 if(pcbRead!=NULL)
  (*pcbRead)=0;

 if(cb!=0)
 {
  assert_hint(cb>0);

  if(pv==NULL)
   return E_INVALIDARG;
 }//if cb!=0

 OLE_LIB__DECLARE_HR(S_OK);

 _OLE_TRY_
 {
  const lock_guard_type __lock__(m_guard);

  assert(m_pos<=m_size);

  const size_type cb_tail=size_type(m_size-m_pos);

  if(cb_tail==0)
  {
   hr=S_FALSE;
  }
  else
  if(cb==0)
  {
   //none
   assert_hint(hr==S_OK);
  }
  else
  {
   assert_hint(cb_tail>0);
   assert_hint(cb>0);

   if(cb_tail<cb)
    structure::static_numeric_cast(&cb,cb_tail);

   assert(cb<=cb_tail);
   assert(cb>0);

   assert(pv);

   memcpy(pv,&m_data[m_pos],cb);

   m_pos+=cb;

   if(pcbRead)
    (*pcbRead)=cb;

   assert_hint(hr==S_OK);
  }//else
 }
 _OLE_CATCHES2_CODE_

 assert(hr==S_OK || hr==S_FALSE || FAILED(hr));

 return hr;
}//Read

Если я правильно помню, PVS говорила что в memcpy может заехать нулевой pv. Перестроил. Новый вариант PVS-у понравился. Мне, определенно, тоже.

Еще один случай. По поводу которого я даже написал в ООН PVS и получил ответ из местного жека — «не выпендривайтесь, пишите проще» 🙂

 if(n!=0 && (pSourceData==NULL || pTargetData==NULL))
  return E_FAIL;

 for(;n;--n)
 {
  const HRESULT
   copy_hr=ole_lib::Variant_Copy(reinterpret_cast<VARIANT*>(pTargetData),
                                 reinterpret_cast<const VARIANT*>(pSourceData));

  assert(copy_hr==S_OK || FAILED(copy_hr));

  if(FAILED(copy_hr))
   return copy_hr;

  pSourceData+=cbSourceElement;
  pTargetData+=cbTargetElement;
 }//for n

Стало:

 if(n)
 {
  assert_hint(n>0);

  if(pSourceData==NULL || pTargetData==NULL)
   return E_FAIL;

  for(;;)
  {
   assert(n>0);
   assert(pSourceData);
   assert(pTargetData);

   const HRESULT
    copy_hr=ole_lib::Variant_Copy(reinterpret_cast<VARIANT*>(pTargetData),
                                  reinterpret_cast<const VARIANT*>(pSourceData));

   assert(copy_hr==S_OK || FAILED(copy_hr));

   if(FAILED(copy_hr))
    return copy_hr;

   pSourceData+=cbSourceElement;
   pTargetData+=cbTargetElement;

   --n;

   if(n==0)
    break;
  }//for[ever]
 }//if n

 assert_hint(n==0);

Ну да, код стал проще и лучше. Но больше 🙂


А баги, Карл, баги PVS нашла?

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

1.1          (dima     08-Oct-04): template<class t_level,class t_string>
1.1          (dima     08-Oct-04): t_pair_level_name<t_level,t_string>::t_pair_level_name(const self_type& x)
1.1          (dima     08-Oct-04):  :level(level),
1.1          (dima     08-Oct-04):   name (name)
1.1          (dima     08-Oct-04): {;}
1.1          (dima     08-Oct-04): //------------------------------------------------------------------------
1.1          (dima     08-Oct-04): template<class t_level,class t_string>
1.1          (dima     08-Oct-04): RELEASE_CODE(inline)
1.1          (dima     08-Oct-04): t_pair_level_name<t_level,t_string>&
1.1          (dima     08-Oct-04): t_pair_level_name<t_level,t_string>::operator = (const self_type& x)
1.1          (dima     08-Oct-04): {
1.1          (dima     08-Oct-04):  level =level;
1.1          (dima     08-Oct-04):  name  =name;
1.1          (dima     08-Oct-04): 
1.1          (dima     08-Oct-04):  return *x;
1.1          (dima     08-Oct-04): }//operator =

… и не встать. Я помню этот долбанный октябрь 2004 года. Повезло, что этот код нигде не использовался. Но, подключался.

Вывод — не надо писать код «про запас». Это отчетливо понимаешь только с ближе к пенсии.

Еще PVS-нашла неинициализированный член класса в примерах на C++ для IBProvider. Не критично, но тем не менее.


В целом.

Штука полезная. Я бы даже сказал «у моих программ есть два друга — тесты и статические анализаторы кода«.

Но хуже семечек.

IBProvider она анализирует 28 минут (~1440 файлов). На десятиядерном процессоре.

Ни разу не упала 🙂

PS. Простите меня за то, что пользовался триалом PVS 🙂

4 комментария

Dmitry Kovalenko  on Декабрь 26th, 2018

Чисто для сведения. Отладочная 64-битная сборка проехала через нагрузочное тестирование. Без проблем. Так что вроде ничего не сломалось 🙂

Process ID: 30252

[TEST HEAP] Initialize ... OK

[25.12.2018 00:23:18] [ExecuteTests] CommandLine:

target\ibp_oledb_test_vc15_x64_Release.exe  /thread_count 4 /log_file_prefix fb30_fb.direct_ram_w64_d3 /auto /log_dir ..\_log\__ReleaseTests1s\ /new_db_location "inet4://localhost/d:\database\ram\" /cn_str "provider=LCPI.IBProvider.3.Private.vc15.debug;location=inet4://localhost/d:\database\ram\ibp_test_fb30_d3_2.gdb;user id=GAMER;password=vermut;ctype=win1251;temp_file_dir=d:\database\ram\temp;dbclient_type=fb.direct;icu_library=icuuc52.dll" /oledb_svc_cmp "LCPI.OleDbServices.DataInitManager.Local.1.Private.vc15.debug"   /test component* /test ds* /test sess* /test odbc* /test cmd* /test column* /test array.write* /test array.read* /test array*001* /test icr* /test rowset* /test schema.002* /test schema.003* /test schema.004* /test schema.005* /test schema.008* /test schema.009* /test schema.010* /test schema.011* /test octet* /test blob*large*65536*1251* /test array*1251* /test charset*1251* /test *4053* /test *wchars_in_utf8_symbol*

[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [component*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [ds*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [sess*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [odbc*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [cmd*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [column*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [array.write*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [array.read*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [array*001*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [icr*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [rowset*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [schema.002*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [schema.003*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [schema.004*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [schema.005*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [schema.008*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [schema.009*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [schema.010*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [schema.011*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [octet*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [blob*large*65536*1251*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [array*1251*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [charset*1251*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [*4053*]
[25.12.2018 00:23:18] [ExecuteTests] Test Mask  : [*wchars_in_utf8_symbol*]

[25.12.2018 00:23:18] [info] Provider DLL    :lcpi.ibprovider_v3_vc15_w64_prof_d.dll
[25.12.2018 00:23:18] [info] Provider Version:3.55.0.29115
[25.12.2018 00:23:18] [info] Server Name     :Firebird
[25.12.2018 00:23:18] [info] Server Version  :3.0.5.33083
[25.12.2018 00:23:18] [info] Client Name     :LCPI.IBProvider.RemoteFB
[25.12.2018 00:23:18] [info] Client Version  :3.55.0.29115
[25.12.2018 00:23:18] [info] Database ODS    :12.0
[25.12.2018 00:23:18] [info] Database Dialect:3
[25.12.2018 00:23:18] [info] ConnectionString:

provider=LCPI.IBProvider.3.Private.vc15.debug;location=inet4://localhost/d:\database\ram\ibp_test_fb30_d3_2.gdb;user id=GAMER;password=vermut;ctype=win1251;temp_file_dir=d:\database\ram\temp;dbclient_type=fb.direct;icu_library=icuuc52.dll

[25.12.2018 00:23:18] [ExecuteTests] 
[25.12.2018 00:23:18] [ExecuteTests] Enter [cancel] for interrupt execution
[25.12.2018 00:23:18] [ExecuteTests] 
[25.12.2018 00:23:18] Creation 4 thread(s)...
[25.12.2018 00:23:18] Start Thread01
[25.12.2018 00:23:18] Start Thread02
[25.12.2018 00:23:18] Start Thread03
[25.12.2018 00:23:18] Start Thread04

......

[26.12.2018 09:53:32] [summary] ------------------------------------------- [SUMMARY INFORMATION]
[26.12.2018 09:53:32] [summary] [TESTS]
[26.12.2018 09:53:32] [summary] EXECUTED      : 697420
[26.12.2018 09:53:32] [summary] SUCCEEDED     : 697407
[26.12.2018 09:53:32] [summary] FAILED        : 0
[26.12.2018 09:53:32] [summary] WITH WARNINGS : 13 [ 36 warning(s) ]
[26.12.2018 09:53:32] [summary] 
[26.12.2018 09:53:32] [summary] - - - - - - - - - - - - - - - - - - - - - -
[26.12.2018 09:53:32] [summary] [TEST TIMES]
[26.12.2018 09:53:32] [summary] REAL          : 4190085501130 [4 day(s)20:23:28.5501130]
[26.12.2018 09:53:32] [summary] USER          : 2608875625000 [3 day(s)00:28:07.5625000]
[26.12.2018 09:53:32] [summary] KERNEL        : 231711093750  [06:26:11.1093750]
[26.12.2018 09:53:32] [summary] TOTAL         : 2840586718750 [3 day(s)06:54:18.6718750]
[26.12.2018 09:53:32] 
[26.12.2018 09:53:32] [TEST HEAP] Test heap is empty
[26.12.2018 09:53:32] [TEST HEAP] Validate ... OK
[26.12.2018 09:53:32] [TEST HEAP] Destroy ... OK
[26.12.2018 09:53:32] 
[26.12.2018 09:53:32] [summary] ------------------------------------------- [PROCESS INFORMATION]
[26.12.2018 09:53:32] [summary] [VIRTUAL MEMORY]
[26.12.2018 09:53:32] [summary] 
[26.12.2018 09:53:32] [summary] PRIVATE BYTES      : 13384 KB       [13MB 72KB]
[26.12.2018 09:53:32] [summary] PEAK PRIVATE BYTES : 6473544 KB     [6GB 177MB 840KB]
[26.12.2018 09:53:32] [summary] VIRTUAL SIZE       : 154072 KB      [150MB 472KB]
[26.12.2018 09:53:32] [summary] PAGE FAULT COUNT   : 124788613
[26.12.2018 09:53:32] [summary] 
[26.12.2018 09:53:32] [summary] - - - - - - - - - - - - - - - - - - - - - -
[26.12.2018 09:53:32] [summary] [HANDLES]
[26.12.2018 09:53:32] [summary] 
[26.12.2018 09:53:32] [summary] HANDLES            : 157
[26.12.2018 09:53:32] [summary] 
[26.12.2018 09:53:32] [summary] - - - - - - - - - - - - - - - - - - - - - -
[26.12.2018 09:53:32] [summary] TOTAL ERRORS  : 0
[26.12.2018 09:53:32] [summary] TOTAL WARNINGS: 36
[26.12.2018 09:53:32] [summary] FULL PASSES   : 1
[26.12.2018 09:53:32] [summary] GEN TESTS     : 8295141

Andrey Karpov  on Декабрь 29th, 2018

Спасибо за заметку. Всегда интересно почитать, так сказать, сторонние впечатления. И с наступающим НГ!

Dmitry Kovalenko  on Декабрь 30th, 2018

И с наступающим НГ!

Симметрично!

Dmitry Kovalenko  on Декабрь 30th, 2018

Еще раз просмотрел заметку и увидел в определении assert_hint точку с запятой после assert.

Косяк, да.

Leave a Comment