Pull to refresh

Comments 45

Интересно, они из принципа не пользуются статическим анализатором кода? Уже и не думал, что кто-то пишет на С/С++ без статического анализатора. Что стоит, сделал задачу (функцию, класс), проверь его статическим анализатором… Тем более, в таких больших проектах.
Тем более, если маленькая команда разработки, то исправлять замечания после — прямой путь к бесконечной поддержке и отнимании ресурсов от добавления новых фич и развития проекта. Дешевле и быстрее проверить статическим анализатором маленькую задачку, на этапе её реализации сразу, чем потом, при нахождении ошибки, переделывать зависимые куски кода…
чем потом при нахождении ошибки, переделывать зависимые куски кода…
Кстати, на днях они решили удалить один компонент с найденными ошибками и просто взять другой)

Мне больше интересно, что у них за компилятор такой. Предупреждения на "нет возврата в non-void функции" и "переменная используется неинициализированной" вроде бы уже любой нормальный компилятор сам выдает.

Вы будете смеяться (плакать), но такое можно встретить даже в Clang:
SequenceNumberManager &operator=(SequenceNumberManager &&Other) {
  NextSequenceNumber = std::move(Other.NextSequenceNumber);
  FreeSequenceNumbers = std::move(Other.FreeSequenceNumbers);
}

И неинициализированные переменные тоже:
static Error mapNameAndUniqueName(....) {
  ....
  size_t BytesLeft = IO.maxFieldLength();
  if (HasUniqueName) {
    ....
    if (BytesNeeded > BytesLeft) {
      size_t BytesToDrop = (BytesNeeded - BytesLeft);
      size_t DropN = std::min(N.size(), BytesToDrop / 2);
      size_t DropU = std::min(U.size(), BytesToDrop - DropN);
      ....
    }
  } else {
    size_t BytesNeeded = Name.size() + 1;
    StringRef N = Name;
    if (BytesNeeded > BytesLeft) {
      size_t BytesToDrop = std::min(N.size(), BytesToDrop); // <=
      N = N.drop_back(BytesToDrop);
    }
    error(IO.mapStringZ(N));
  }
  ....
}

Ндаа. Знаете, смеяться как-то не очень хочется.
Но это как-то совсем странно. Предупреждения отключены что ли у них? Или их просто не читают?

Причин несколько:
  1. Мы ведь не зря берём деньги за PVS-Studio :). Мы рассматриваем больше ошибочных паттернов и больше конструкций, в которых ошибка может проявить себя. Соответственно мы опережаем возможности компилятора. Компиляторы развиваются, и возможно сейчас Clang уже смог бы сам в себе обнаружить эту ошибку. Но ведь и мы на месте не стоим и за это время научились делать много всякого нового :). См. про технологии и развитие.
  2. Мы очень серьёзно работаем над сокращением количества ложных срабатываний. Многие анализаторы и компиляторы сильно не заморачиваются на эту тему. Вроде диагностика есть и ладно. Но фича в том, что если ложных срабатываний много, то сообщения этого типа просто перестают смотреть или отключают. Поэтому мы часто замечаем то, что просто не смотрят в силу неудобства. Эта мысль подробнее.
  3. Программисты считают, что не допускают таких ошибок :).

Ну, с первым можно слегка поспорить — ибо есть некоторые предупреждения, которые компиляторы выдают надежнее, чем PVS (например, "не все поля класса были проинициализированы в конструкторе").
Но в целом вы правы, конечно.

Ха. Здесь как раз та ситуация, что выдать много предупреждений дело не хитрое. Нет явной инициализации в конструкторе — ругаемся. Толку только от таких сообщений мало. В том смысле, что пользоваться нельзя. Слишком много шума. И мы немало потрудились, чтобы достичь разумного баланса между небольшим количеством сообщений и боязнью что-то пропустить. Заметка про то, как писалась эта диагностика: Поиск неинициализированных членов класса.

Если не ошибаюсь, Haiku до сих пор собирают компилятором GCC v2.9, чтобы иметь совместимость с BeOS. А когда она будет достигнута, будет совершен переход на что-то свежее (хотя бы не мертвое).

Боже. Полагаю, спрашивать, зачем им в 2019 году совместимость с BeOS, бесмысленно?

Была изсчерпывающая статья на Хабре, в которой все расписано :)
Вкратце — хотят достичь 100% работы старых скомпилированных приложений, это цель номер один, а дальше уже можно будет развиваться в ином направлении.
А когда она будет достигнута, будет совершен переход на что-то свежее

Получится один из тех переездов, которые равны двум пожарам… Пока что-то отстроят на месте пепелища — пора будет гореть переезжать опять.
UFO just landed and posted this here
Если не ошибаюсь, Haiku до сих пор собирают компилятором GCC v2.9, чтобы иметь совместимость с BeOS

Стандарт языка вряд ли повлиял на выбор инструмента статического анализа в данном случае)
  delete fBigIcon;
  fBigIcon = NULL;

  fVectorIcon = NULL;            // <=
  free(fVectorIcon);             // <=

Из этого куска не ясно, но есть подозрение что вместо free нужно было вызывать delete.
Это просто совпадение, что в проекте нашлось много ошибок с заменой delete/free, но прям тут просто перепутали строки местами. Сообщения у анализатора разные на этот счёт.
Скорее всего, в коде перепутали операторы '|' и '||'. Такая ошибка приводит к лишним вызовам функции SetDecoratorSettings.

А по-моему, это обычное применение паттерна Observer. Интересно, если бы запись была вида "changed |= listener->SetDecoratorSettings(window, settings);"
сработало бы предупреждение?

Паттерн такой есть. Но на каждый N-й такой паттерн программисты допускают ошибки, поэтому кое-что выписываем в статьях. Я подумал, что подозрительно дёргать сеттер настроек лишний раз.

Возможно так сделали специально, чтобы уведомить об изменениях всех слушателей.

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

Ну ок. Осталось пройтись по 2999 другим предупреждениям уровня High) Сейчас в телеграмм-канале несколько человек вдохновились, чтобы мы помогли с исправлениями… вот поэтому мы не правим код и не отправляем PR после проверки проектов :-)
У них там никакой политики работы с памятью нет что-ли? Адская каша из new/delete и malloc/free в коде C++ как бы намекает что в разработке имеет место определенный бардак.
Скорее что в разработке С++ имеет место С и его библиотеки :)
Учитывая специфику проекта — что это ОС, которую пишут с 2002 года — это не так плохо.
За это время стандарт С++ неоднократно менялся.
 case HASH_EMPTY_DELETE:
        // TODO: destructors are not called!
        delete entry->value;
        break;
      case HASH_EMPTY_FREE:
        free(entry->value);
        break;


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

Или вот такое
void* _GetBuffer()
{
  ....
  void* buffer = malloc(fBufferSize);
  if (buffer == NULL && !fBuffers.AddItem(buffer)) {
    free(buffer);
    throw std::bad_alloc();
  }
  return buffer;
}


malloc/free никаких о ctor/dtor не знают и они не вызываются, а это верный путь для того чтобы с разбегу запрыгнуть на грабли. Ну и печальный коммент "// TODO: destructors are not called!" как итог. И правда, почему же деструкторы не вызываются.
Никаким использованием C функций не оправдывается вообще.
Ну мало ли что там происходило в творческом процессе. Возможно автор пришёл к выводу, что уничтожение ключей и значений, помещённых в Хеш-таблицу, является ответственностью тех, кто их туда засовывал — и просто забил на этот кусок кода :)

Во втором примере кроме описанной в статье ошибки пара malloc-free вроде нормально используется.
C++ код с использованием malloc/free вместо штатных new/delete сам по себе выглядит странно.

Вызов конструктора это жесть. Они из мира Джавы что ли пришли?

Каша из new/delete и malloc/free еще хуже.
Переменная rval не была инициализирована при объявлении, поэтому её сравнение с нулевым значением приведёт к неопределённому результату.

Это при условии, если выполнится тело if, а выполнится оно только если кто-то вызовет longjmp(), но так как longjmp() явно не выполнится до того как rval инициализируется в следующем за if оператором for, то это и не ошибка вовсе:

...
	if (sigsetjmp(toplevel, 1)) {
		if (connected)
			disconnect(0, NULL);
		if (rval > 0)
			rval = argpos + 1;
		return (rval);
	}
	(void)xsignal(SIGINT, intr);
	(void)xsignal(SIGPIPE, lostpeer);

	/*
	 * Loop through as long as there's files to fetch.
	 */
	for (rval = 0; (rval == 0) && (argpos < argc); argpos++) {
...


Да, запутано и не «в лучших традициях», но всё же rval не будет иметь неопределенного значения. Видимо, ваш анализатор не учитывает как выполняется setjmp().

PS: Было бы очень здорово если бы при публикации примеров были ссылки на оригинальные файлы (хотя бы пути в исходниках), а то пойди найди этот fetch.c в коде haiku — в мастере его уже нет…
У нас бывают задачи, когда нужно найти очень старый код по предупреждению. Указание имени исходника и имени функции (+немного кода) вполне достаточно. Полный путь будет слишком длинно.

Я читал в переписке разработчиков, что ftp компонент они просто выкинут из-за ошибок и возьмут другой. К привязке к коду не могу сказать. Но вы можете легко восстановить файл по истории коммитов.
Они то, может, и выкинут.

Но Вы свою ошибку анализа setjmp(), а, быть может, и иных методов обработки исключений, исправлять запланировали ли?
Мы провели работу по улучшению анализатора по результатам проверки ещё 2 недели назад, когда и выдали первый отчёт команде разработчиков.
Да, запутано и не «в лучших традициях», но всё же

Наверняка что-то осталось. Но если Ваш код нельзя описать этим предложением, то результаты анализа будут лучше. Как и мнение коллег по цеху)
Просто как-то странно, что статья «Как выстрелить себе в ногу...» начинается с выстрела в свою ногу, с явно ошибочного и ложного сообщения анализатора.

Согласно стандартов C/C++, а setjmp() часть стандартов, этот фрагмент кода не содержит обращения к неинициализированной переменной и имеет вполне определённое поведение.

Какая-то антиреклама получается.

P.S. А, скажем, в таком коде:
int cnt;
try {
    for(cnt = 0; cnt < n; cnt++) {
        ...
    }
} catch(...) {
    return(cnt);
}

Он тоже обнаружит неинициализированную переменную?!
P.S. Извините, частично был гражданин соврамши, заглянул в стандарт следует читать: «и имел бы вполне определённое поведение при наличии квалификатора “volatile”» (C11, 7.13.2.1 The longjmp function).
В чём путаница и нарушение традиций? Некоторые, конечно, пишут иначе:
if(0 == setjmp(buf, 1)) {
    for(cnt = 0; cnt < n; cnt++) {
        ...
    }
} else {
    return(cnt);
}
Я не зря поставил «традиции» в кавычки, но в целом ваш пример хорошо показывает как было бы лучше — всё же более логично если обработка исключений идёт после кода который может их вызывать, а не до (в пределах одной функции, по крайней мере).

А путаница — setjmp()/longjmp() имеют массу подводных камней (да ещё и в сочетании с обработчиками сигналов), их использование как poor man's exception handling несколько опасно и чревато боком. Впрочем, если принять во внимание остальной код из Haiku, это наименьшая из их проблем…

Вообще-то после вызова delete содержимое памяти по указанном адресу останется прежним, поэтому его еще можно безопасно использовать. Проблемы могут возникнуть только после следующего вызова new.
</irony-mode-off>

Это сильно зависит от деаллокатора и даже вполне вероятно
содержимое памяти по указанном адресу останется прежним
не факт, но скорее всего
его еще можно безопасно использовать
при некоторых условиях — да. Поскольку память выделяется страницами, если на той-же странице ещё остались выделенные блоки то ко всему содержимому страницы ещё можно «безопасно» обращаться. Увы это не долго продержится, и если это был последний блок в странице, то первое же обращение выдаст ошибку
Проблемы могут возникнуть только после следующего вызова new
не только, если страницу освободят то возникнут до этого, если удаляли ещё несколько объектов, то и после new могут не возникнуть.

Самое плохое, что в таком «особом» режиме обращения к освобождённой памяти программа может очень долго работать без ошибок. А когда ошибка возникнет — то её не так просто повторить. Есть способы отловить такую ошибку — как простые, например менеджер памяти, который выделяет по странице на объект (считаем что объект меньше страницы), так и более продвинутые.

Большое спасибо за развернутое разъяснение. Абсолютно согласен с вами насчет того, что это не просто выстрел в ногу, это скорее мина в рюкзаке, готовая взорваться от любой встряски.


Помнится, когда еще не было таких хороших компиляторов и статических анализаторов, я писал свои кастомные free, malloc, delete и new, где принудительно после выделения памяти, заполнял ее символами ‘+’, а после освобождения — символами ‘-‘, и это позволяло быстро обнаружить такие косяки, как и выход за границы буфера.


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

Это хорошо, но не очень интересно читать как анализатор отпределеяет разименование NULL.
А есть сложные, интересные ошибки? Такие что с первого взгляда и не кажутся ошибками, а скорее ложными срабатываниями анализатора, или например ошибки которые сложно обнаружить «методом пристального взгляда» но анализатор их находит? Конечно такую подборку сделать сложно, но кажется, она была бы полезна всем. В том числе и в качестве качественной рекламы.
Заранее прошу прощения если такая подборка уже есть, за всем не уследить.
Sign up to leave a comment.