FoxCanFly 19 мар 2015 в 17:41

Проверка Vim при помощи PVS-Studio в GNU/Linux

8 мин

34K

Блог компании PVS-StudioOpen source*C++*VIM*

+52

Комментарии 64

Eivind 19 мар 2015 в 18:13

Это уже похоже на ошибку. Анализатор говорит о лишней проверке, но на самом деле проблема прежде всего в другом. Указатель retval инициализирован 0, и я не нашел ни одного места в этой функции, где бы его значение менялось. При этом в него делается много раз делается strncpy. После чего его внезапно решили проверить на NULL.

char_u	*retval = NULL;
...
for (round = 1; round <= 2; ++round)
{
    ...
    if (lnum < 0 || submatch_mmatch->endpos[no].lnum < 0)
	return NULL;

    ...
    if (s == NULL)
	break;
    ...
	if (round == 2)
	    vim_strncpy(retval, s, len);
	...
	if (round == 2)
	{
	    STRCPY(retval, s);
	    retval[len] = '\n';
	}
	...
    if (round == 2)
	STRCPY(retval + len, s);
   	...
    if (round == 2)
	retval[len] = '\n';
	...
	if (round == 2)
	    STRNCPY(retval + len, reg_getline_submatch(lnum),
				     submatch_mmatch->endpos[no].col);
	...
	if (round == 2)
	    retval[len] = NUL;
	...
    if (retval == NULL)
    {
	retval = lalloc((long_u)len, TRUE);
	if (retval == NULL)
	    return NULL;
    }
}

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

eyeless_watcher 19 мар 2015 в 19:30

Ошибки нет, но качественным кодом я бы это не назвал.
Неужели нельзя было порезать эти два раунда на две отдельные процедурки, раз уж количество раундов заранее известно?

Eivind 19 мар 2015 в 20:21

Внимательно изучив код, я понял, что немного ошибся — это никакая не оптимизация, первый проход — холостой — делается для расчета длины, а второй для операций над строкой, о чем и написано в комментарии (которые мой внутренний парсер давно привык игнорировать)

/*
* First round: compute the length and allocate memory.
* Second round: copy the text.
*/

Так как выделение памяти происходит исходя из расчетной длины, «неоптимизированный» вариант с выделением памяти заранее невозможен.
А разделение на две процедуры породило бы дублирование кода в обе процедуры, что грозит ошибками в случае его модификации. Однако, сделать его более «качественным» можно было бы, например, заменой абстрактного int round на bool length_calculation_only.

eyeless_watcher 19 мар 2015 в 20:36

А разделение на две процедуры породило бы дублирование кода в обе процедуры, что грозит ошибками в случае его модификации.

Я не очень понимаю, что мешает пойти чуть дальше и нарезать эти две процедурки на несколько мелких (которые всё равно заинлайнятся), используемых по необходимости.
Здесь же по факту одна функция делает две вещи: считает размер и копирует данные. Прямое нарушение single responsibility principle, которое еще и наглядно затрудняет понимание происходящего.

mark_ablov 19 мар 2015 в 18:16

Здорово, какой качественный код у vim'a.
Фактически ни одного найденного бага, пусть и одним инструментом.

+13

ShapovalovTS 19 мар 2015 в 18:29

Интересно было бы сравнить с качеством кода neovim.

xvilka 20 мар 2015 в 16:25

По-идее, там было много исправлено ошибок, которые нашел Coverity. С другой стороны огромный пласт кода просто переписан с нуля. Но код vim качественным для чтения я бы не назвал.

cubuanic 19 мар 2015 в 19:09

я могу сказать, что весьма проникся

Здорово, нашего полку прибыло! :)

robert_ayrapetyan 19 мар 2015 в 19:20

В заключение хотелось бы сказать, что сейчас проверка проектов с помощью PVS-Studio под GNU Linux без использования машины с Windows является вполне реальной и довольно удобной.

Где можно скачать эту версию?

danfe 19 мар 2015 в 19:35

Присоединяюсь к вопросу. Еще в январе EvgeniyRyzhkov говорил, что ни линукс-, ни вайн-версию в ближайшее время ожидать не стоит, ибо нет спроса; и в FAQ у вас до сих пор висит ответ «к сожалению, нет, извините» по этому поводу.

Andrey2008 19 мар 2015 в 20:02

Поддержка Linux развивается в направлении custom-версий. Напишите нам письмо, как сотрудник компании, в которой работаете. Обсудим эти вопросы. Никакой публичной версии нет и пока не планируется.

danfe 20 мар 2015 в 18:29

Ясно. Наверное, вам стоит обновить ответ про Linux-версию в FAQ. А есть ли в планах сделать Windows-версию запускаемой (работающей) из-под Wine?

EvgeniyRyzhkov 19 мар 2015 в 20:06

Голосом из Варкрафта 2: «Кто ко мне взывал?»

Напишите нам с корпоративного ящика, публичной версии не планируется.

EvgeniyRyzhkov 19 мар 2015 в 20:07

-2

Andrey2008 25 окт 2016 в 09:55

PVS-Studio для Linux.

AoD314 19 мар 2015 в 20:01

Кстати, существует еще и форк: neovim.org/

encyclopedist 19 мар 2015 в 20:07

Лишняя проверка
…
Указатель ptr уже был проверен на NULL, в случае истинности этого условия ему был присвоен указатель comp_leader, который точно не нулевой. Вторая проверка не требуется.

А если была выполнена ветка, помеченная do nothing?

Области видимости
…
С какой целью так делать вместо объявления еще одной переменной внутри области видимости buffer (неужели для экономии места на стеке?), мне не понятно. Такой код плохо поддерживаем и неудобно читаем.

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

Ошибка с signed и unsigned типами в одном выражении
…
В данным случае код ведет себя верно, цель свою (проверка, является ли символ цифрой) выполняет, но не думаю, что есть причины использовать в своем коде подобные приемы без нужды. Цикл можно было сделать от '0' и обойтись без приведения к unsigned.

Это считалось оптимизацией (и до сих пор иногда рекомендуется в книжках и интернете), потому что позволяет использовать одно сравнение вместо двух.

Небезопасное использование realloc
…
К счастью, судя по комментарию, это скоро исправят.

Хммм… код был написан в мае 2013, комментарий автор добавил 10 февраля 2015, но код не исправил, так что вероятно он и дальше так останется.

DarkByte 19 мар 2015 в 20:18

А если была выполнена ветка, помеченная do nothing?

Там return.

encyclopedist 19 мар 2015 в 20:19

Самого главного то я и не заметил.

khim 19 мар 2015 в 23:45

Это считалось оптимизацией (и до сих пор иногда рекомендуется в книжках и интернете), потому что позволяет использовать одно сравнение вместо двух.

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

$ cat test.cc
bool is_digit(char c) {
  return (c >= '0') && (c <= '9');
}
$ gcc -O3 -S test.cc -o- | c++filt
	.file	"test.cc"
	.text
	.p2align 4,,15
	.globl	is_digit(char)
	.type	is_digit(char), @function
is_digit(char):
.LFB0:
	.cfi_startproc
	subl	$48, %edi
	cmpb	$9, %dil
	setbe	%al
	ret
	.cfi_endproc
.LFE0:
	.size	is_digit(char), .-is_digit(char)
	.ident	"GCC: (Ubuntu 4.8.2-19ubuntu1) 4.8.2"
	.section	.note.GNU-stack,"",@progbits

Оставьте ему работу, не засоряйте голову программисту всеми этими мелкими оптимизациями.

ZyXI 20 мар 2015 в 08:27

Vim до сих пор поддерживает многие старые версии компиляторов. А конкретно эту оптимизацию, к примеру, не умеет современный pcc-1.1.0.

encyclopedist 19 мар 2015 в 20:06

Удалён дубликат

BalinTomsk 19 мар 2015 в 22:24

---A call of the 'memset' function will lead to underflow of the buffer '& debug_saved'.

а почему программа решила что это memset? пользовательская функция, внутри может быть все что угодно

Andrey2008 19 мар 2015 в 22:33

Анализатор работает с препроцессированными файлами. И если макрос vim_memset объявлент как

#define vim_memset(ptr, c, size)   memset((ptr), (c), (size))

то значит это функция memset(). Никакой магии. :)

BalinTomsk 20 мар 2015 в 00:51

Это ни о чем не говорит, я могу в линкере легко заменить это чем угодно.

Собственно часто так и делаю.

-4

Andrey2008 20 мар 2015 в 07:58

Гм… Замените — предупреждения не будет. В чём собственно тогда Ваш вопрос?

FoxCanFly 19 мар 2015 в 22:34

По названию и сигнатуре можно предположить, что вероятнее всего это memset. А в случае с макросом еще проще

mbait 19 мар 2015 в 23:22

Я уже задавал этот вопрос в другом вашем посте, но тогда внятного ответа не получил. Почему вы не хотите оформить вызов анализатора как вызов компилятора? Подобно тому, как это делает distcc. Тогда, если сборка проекта позволяет указывать компилятор, никакие файлы править не надо, вызов анализатора сводится к

СС=«PVS-Studio ...» ./configure && make

Я очень рад, что так или иначе вы начали поддерживать GNU-style проекты, хотя начиналось всё с «у вас там никто не платит, поэтому не видать вам линукс версии».

EvgeniyRyzhkov 20 мар 2015 в 02:20

А у нас примерно так и работает:

PVS-Studio.exe --cl-params %ClArgs% --source-file %cppFile% --cfg %cfgPath% --output-file %ExtFilePath%

cl-params — те самые параметры компилятора.

Вот только без передачи других параметров пока что-то не можем обойтись. Хотя может и придумаем как это забороть.

mbait 20 мар 2015 в 09:08

Я не совсем это имел ввиду. Когда собирается проект, сконфигурированны через autotools, компилятор берется из переменной среды `CC`. Это позволяет легко менять компилятор на любой, поддерживающий флаги gcc. Например, я хочу собрать программу конкретной версией (более новой, стабильной, другая ветка). Тогда я могу написать:

CC=/opt/gcc/bin/gcc-1.2.3 ./configure && make

Это значение потом буквально подставляется во все места вызова компилятора во всех make-файлах. И если бы ваша программа так умела, то применение анализатора было бы делом пары секунд.

Еще хочу заметить, что когда программа принимает какие-то параметры, но не использует их сама, а передает дальше, обычно используют строку `--`, чтобы разделить «свои» и «не свои» аргументы. Например:

PVS-Studio --pvs-flag1 --pvs-flag2 — -Wall -ffast-math -o test

Тут все, что идет после `--`, передается gcc.

Если вам нужна помощь с интеграцией вашего инструмента с open-source — пишите, я с радостью помогу (бесплатно).

EvgeniyRyzhkov 20 мар 2015 в 09:18

Про CC я понимаю. Нам по разным причинам хочется передавать еще часть своих параметров (--source-file и --output-file). Что придумать, чтобы решить этот вопрос?

mbait 20 мар 2015 в 09:28

Ну, --source-file вы и так можете получить, ведь вам доступны все параметры командной строки, включая имена файлов для компиляции. Кстати, gcc умеет компилировать сразу несколько файлов, вам тоже было бы неплохо так уметь. Насчет --output-file — почему бы брать имя исходного файла с вашим расширением? Например, если компилируется test.cpp, то вы можете записывать в test.pvslog. К сожалению, ничего другого посоветовать не могу.

EvgeniyRyzhkov 20 мар 2015 в 09:33

Ну, --source-file вы и так можете получить

Но для этого придется парсить строку.

Кстати, gcc умеет компилировать сразу несколько файлов

Мы тоже умеем.

почему бы брать имя исходного файла с вашим расширением?

Потому что это один лог на все файлы.

Eivind 20 мар 2015 в 09:36

Соединить логи в один можно там, где работает линковщик.

EvgeniyRyzhkov 20 мар 2015 в 09:41

Так идея то нарушается! Ведь смысл в том, чтобы не править makefile.

phprus 20 мар 2015 в 09:49

Линковщик тоже можно подменить через переменную окружения LD, по аналогии с CC.
wiki.wxwidgets.org/Cross-Compiling_Under_Linux#environment_variables

Eivind 20 мар 2015 в 09:53

В чем нарушается то?

${CC} ${CCFLAGS} -c -o $@ $<
=>
PVS-Studio -cc=gcc — ${CCFLAGS} -c -o $@ $<

На выходе получим $@ и $@.pvs

${LD} -o ${TARGET} ${OBJS} ${LIBS}
=>
PVS-Studio -ld=gcc — -o ${TARGET} ${OBJS} ${LIBS}

На выходе получаем ${TARGET} и готовый лог

EvgeniyRyzhkov 20 мар 2015 в 09:55

Да, похоже так можно сделать. Попробуем, спасибо.

mbait 20 мар 2015 в 09:44

Для парсинга командной строки в линуксе есть, так что это совсем не сложно. Зачем вам один файл, я не очень понимаю. Наверно, я не понял вашу задумку в целом. Как пользователю мне было бы неудобно смотреть весь лог в поисках ошибок для конкретного файла. Vim читает stderr, так что вы можете дублировать туда.

-1

EvgeniyRyzhkov 20 мар 2015 в 09:53

Мне кажется вы теоретик. Извините если не прав, но все, кто пользовался хоть раз статическим анализом типа lint говорили какой там отстой что при проверке каждого нового файла он выдает сообщения на одну и ту же .h-ку много раз. И как круто, что в PVS-Studio единый лог, где дубликаты фильтруются.

mbait 20 мар 2015 в 09:50

Если вы хотите хранить состояние, типа «сделать первый проход, вывести все ошибки, а во втором проходе какие-то убрать, какие-то добавить», то в этом случае создается обычно sqlite база в корне проекта.

EvgeniyRyzhkov 20 мар 2015 в 09:52

Создается один лог с полным списком сообщений. А затем его специальной утилитой можно преобразовывать с кучей фильтров: показать все ошибки с таким-то кодом, все ошибки из такой-то папки и т.п.

symbix 19 мар 2015 в 23:33

Вот тут кто-то проверял vim 7.4 в coverity: code.google.com/p/vim/issues/detail?id=333

Упомянутый neovim, кстати, им проверяют на регулярной основе — scan.coverity.com/projects/2227

datacompboy 20 мар 2015 в 13:52

Так как получить linux-бету для пощупать?

-1

EvgeniyRyzhkov 20 мар 2015 в 14:00

Написать нам с корпоративного e-mail и обсудить условия контракта.

datacompboy 20 мар 2015 в 14:12

А публичная бета не ожидается? :) []

EvgeniyRyzhkov 20 мар 2015 в 14:26

Нет.

Andrey2008 25 окт 2016 в 09:56

Прошу сюда: PVS-Studio для Linux.

datacompboy 25 окт 2016 в 10:35

ООоо! Спасибо! Разгребусь, и за ключиком приду опять — я проделал кой-какую работу над ошибками с прошлой попытки, и теперь готов еще раз предметно над интеграцией поработать, глядишь в этот раз взлетит.

amarao 20 мар 2015 в 15:42

Традиционный вопрос: баги разработчикам заслали?

-2

Andrey2008 20 мар 2015 в 15:44

Да. Нас уже поблагодарили. Правда особенно не за что. В общем то ничего интересного не нашлось в этот раз.

amarao 20 мар 2015 в 16:42

Спасибо.

-1

Andre_487 26 мар 2015 в 02:43

Можно узнать, почему категорически не планируется публичных версий под Linux?

EvgeniyRyzhkov 26 мар 2015 в 09:39

У нас нет законченного продукта под Linux. Сейчас это на уровне alpha-версии. Соответственно выкладывать на публику это не имеет смысла, так как требуется набор напильников для запуска.

Andre_487 26 мар 2015 в 13:59

То есть, все-таки что-то планируется, просто позже, когда продукт дойдет до кондиции?

EvgeniyRyzhkov 26 мар 2015 в 14:02

Не факт. Возможно по результатам первых продаж мы решим убить это направление.

danfe 27 мар 2015 в 17:18

Побуду занудой (раз уж на комментарий выше мне так никто и не ответил) и снова вас спрошу: почему бы не сделать Windows-версию (которая вполне себе перспективная, и которую явно никто не собирается убивать) поддерживающей работу из-под Wine? На первый взгляд это кажется проще, чем разрабатывать consumer-grade версию для GNU/Linux, и хоть как-то обрадует не-Windows пользователей и (возможно) потенциальных новых клиентов вашего бизнеса.

EvgeniyRyzhkov 27 мар 2015 в 19:56

Ответ простой — спроса нет. Ни на версию на Wine, ни на версию для Linux. Всего пара команд отписалась в почту с интересом.

danfe 28 мар 2015 в 15:53

Да-да, вы мне так в январе и ответили, что нет спроса. Я, наверное, недостаточно четко выразился: речь не про Windows-версию, обернутую в Wine для непосредственного запуска в линуксе и готовую к проверке нативных проектов под Unix/Linux. Это как раз понятно, что большой труд, мало кому нужный одновременно.

Я лишь про то, чтобы допилить обычную Windows-версию для запуска под вайном (версия 5.21 вываливается с Internal Error: Invalid escape sequence: '\b'), чтобы проверять вендовые же проекты без необходимости устанавливать Windows. Эта задача, на мой взгляд, вполне решаема. Никакой специальной вайн-версии не появляется — просто ваша обычная вендовая версия начинает работать под вайном (ничего про Linux при этом не зная).

Уж если Photoshop CS2 вполне так работает, почему PVS-Studio не может? :-)

EvgeniyRyzhkov 28 мар 2015 в 17:39

Слушайте, эта проблема не стоит стольких комментариев, сколько Вы уже написали. Если это действительно вас беспокоит, то вопрос решается ОЧЕНЬ просто. Пишите нам в поддержку: «Я лицензионный пользователь PVS-Studio, лицензия такая-то. У меня вот тут немного не работает — допилите, пожалуйста.» И все. В абсолютно рабочем порядке такие доработки скорее всего будут сделаны.

danfe 28 мар 2015 в 19:04

Я не являюсь вашим лицензионным пользователем, и не имею никакого права требовать что-то от вашего саппорта. Мне интересен ваш продукт, но прежде чем говорить с руководством о перспективах приобретения, мне нужно будет не то что доказать, что покупка оправдана, но хотя бы продемонстрировать возможность запуска не-из-под Windows. Увы, сделать этого я не могу.

Кроме того, даже если бы я был лицензионным пользователем, нет никаких гарантий, что вы не ответите «извините, мы не обещали работу в Wine; вы же покупали версию для Windows? Там (нативно) всё работает? Ну вот видите, а наша (и ваша) поддержка распространяется исключительно на неё».

И последнее: имхо, «в абсолютно рабочем порядке такие доработки» обычно делаются просто потому, что неработающая в Wine user-space программа это странно: либо баг в программе (значит, надо пофиксить), либо в Wine (хорошо бы заслать bug report). В конце-концов, ведь для того, чтобы зарепортить проблему в публичной, ознакомительной версии, не должна требоваться покупка, правда?

EvgeniyRyzhkov 28 мар 2015 в 20:37

К сожалению, поддержка Wine не в приоритете.

-2

ZyXI 27 мар 2015 в 21:13

А смысл? Основную часть (анализатор) нужно ещё умудриться написать некросплатформенно, консольный UI тоже, а вот что придётся допиливать, так это ложные срабатывания из‐за заголовочных файлов, диагностики для несколько другого набора стандартных функций, … Это уже где‐то представители компании объясняли — сделать код кроссплатформенным легко. Научить анализатор работать с linux‐специфичными вещами сложнее.

danfe 28 мар 2015 в 16:02

Смысл, например, в том, чтобы проверять свой код PVS-студией, не устанавливая при этом Windows. Я не предлагаю «научить анализатор работать с linux‐специфичными вещами», пусть это будет моей головной болью — как бы так подсунуть свой код, чтобы PVS-студия его успешно проверила (посчитав вендовым). А вот ставить Windows и/или возиться с виртуалками я не хочу.

Зарегистрируйтесь на Хабре, чтобы оставить комментарий