Как непродуманные предупреждения компиляторов помогают портить совершенно правильный код
Это пост о сложностях взаимодействия искусственного и естественного интеллекта. Современные компиляторы имеют довольно развитые средства статического анализа, которые умеют выдавать предупреждения на разные подозрительные конструкции в коде. В теории эти предупреждения должны помочь разработчикам делать меньше ошибок в коде.
На практике далеко не всегда предупреждения компилятора одинаково полезны. Зачастую они не помогают разработчикам, а мешают им и могут провоцировать на исправление совершенно правильного кода, т.е. на нарушение правила «работает — не трогай».
Для примера будет история об одной строке кода из довольно популярной библиотеки с открытым кодом для работы с XML. Контекст этой строки такой:
fseek( fp, 0, SEEK_END );
const long filelength = ftell( fp );
fseek( fp, 0, SEEK_SET );
if( filelength == -1L ) {
return FILE_READ_ERROR;
}
if( filelength >= (size_t)-1 ) {// ВОТ ЭТА СТРОКА
return FILE_READ_ERROR;
}
const size_t size = filelength;
_charBuffer = new char[size+1];
size_t read = fread( _charBuffer, 1, size, fp );
if( read != size ) {
return FILE_READ_ERROR;
}
Этот фрагмент пытается прочитать файл на диске целиком. Для этого переводит файловый указатель на конец файла и получает текущую позицию в файле. Дальше делается попытка выделить память под данные всего файла. Перед выделением памяти стоит проверить, поместится ли требуемый размер в size_t, чтобы избежать труднодиагностируемых логических ошибок при дальшейшей работе.
Выполняется (по сути, а в коде стоит противоположная и возврат кода ошибки) проверка, что filelength строго меньше, чем максимальное значение size_t. Проверка выполняется на «строго меньше», потому что нужно выделить памяти на один элемент больше, чтобы записать завершающий нулевой символ перед дальнейшим разбором содержимого файла.
Все бы хорошо, но компилятор негодуэ — ведь с одной стороны сравнения знаковый тип long, а с другой стороны — беззнаковый size_t. ПРЕДУПРЕЖДЕНИЕ -Wsign-compare от gcc!!!
Что же делать, куда бежать??? Это же предупреждение!!!
Один из разработчиков исправляет проверку:
if( (size_t)filelength >= (size_t)-1 ) {
… и компилятор УЗБАГОИЛСЯ доволен. Успех? Увы, изначально правильный код теперь работает уже не так, как было задумано. Размеры long и size_t не связаны друг с другом. Размер любого из этих типов может быть больше размера второго на конкретной платформе. Первоначальный код требовал в таком случае преобразования обоих значений к одному и тому же типу — достаточного для обоих размера. В «исправленном» коде произойдет потеря части значащих разрядов в случае, если размер long больше размера size_t.
Так для избавления от предупреждения в изначально правильном коде этот код исправили и сломали.
Ошибка могла бы остаться надолго и еще многие годы радовать пользователей при работе с очень большими файлами на некотором подмножестве систем. Не осталась — код библиотеки открыт и даже лежит на Github, вскоре приходит пул-запрос с правкой этого кода. В исправленном коде проверка выглядит так:
if( (unsigned long)filelength >= (size_t)-1 ) {
В исправленном коде сравниваемые значения оба беззнаковых типов, поэтому ужасная проблема (тм), о которой gcc сообщал предупреждением -Wsign-compare, здесь отсутствует. Приведение long к unsigned long здесь не приводит к неверной интерпретации данных, потому что единственно возможное отрицательное значение обработано ранее.
Успех? Нет, не так быстро.
Правку вливают в основную ветвь, после чего начинают поступать сообщения о новом предупреждении. При компиляции под платформы, где размер unsigned long меньше размера size_t, clang выдает предупреждение -Wtautological-constant-out-of-range-compare «ПРЕДУПРЕЖДЕНИЕ!!! Диапазон значений unsigned long так уныл и безнадежно ограничен, что сравнение всегда дает один и тот же результат и не имеет решительно никакого смысла».
Полностью переносимый код для любой платформы, говорите? Нет, когда явное приведение long к size_t могло повлечь потерю разрядов, это всех устраивало. А теперь clang выдает бесполезное предупреждение и пользователи библиотеки оставляют комментарии к правке и сообщения о дефектах. Это же предупреждение!!!
Что же делать… ведь нужно сделать так, чтобы на платформах, где размер unsigned long меньше размера size_t, сравнения не было, а на остальных платформах — было.
А, вот решение «проблемы»:
template = sizeof(size_t))>
struct LongFitsIntoSizeTMinusOne {
static bool Fits( long value )
{
return (unsigned long)value < (size_t)-1;
}
};
template <> bool LongFitsIntoSizeTMinusOne::Fits( long /*value*/ )
{
return true;
}
— Что это, Бэрримор???
— Это шаблон Баскервилей, сэр!!!
Здесь можно было бы применить шаблон функции, но значения параметров шаблона по умолчанию для шаблонов функций поддерживаются только начиная с C++11, а библиотека — чтобы расширить аудиторию — старается без него обходиться. struct вместо class используется, чтобы не указывать видимость public явно.
Применяется так:
if ( !LongFitsIntoSizeTMinusOne<>::Fits( filelength ) ) {
В зависимости от соотношения между размерами long и size_t компилятор выбирает либо специализацию, либо реализацию по умочанию. В первом случае бессмысленной проверки не будет, и компилятору не о чем будет ПРЕДУПРЕЖДАТЬ.
В основную ветвь вливают проверку с использованием этого шаблона — и «проблема» решена.
Краткое содержание предыдущего текста… Совершенно правильный код с одним сравнением целочисленной переменной и целочисленной константы правили трижды, чтобы сделать счастливыми два очень популярных компилятора. Первая же из правок этот совершенно правильный код сломала, зато сделала счастливым компилятор, ради которого вносили правку. В итоге вместо простого сравнения получили что-то ближе к FizzBuzz Enterprise. Зато нет предупреждений.
Теперь давайте порассуждаем…
Насколько популярным должен быть сферический кусок открытого кода в вакууме, чтобы он имел шансы пройти весь путь от первой правки до последней, и насколько более вероятно, что менее популярный код так и остался бы сломанным первой правкой? Понадобилось в общей сложности явно больше одного часа работы разработчиков с не самой низкой квалификацией, чтобы найти показанное выше решение, которое даже не исправляет ошибку, а превращает просто правильный код в правильный код без предупреждений.
Почему так произошло? Причин ровно две.
Первая причина — разработчики почти безоговорочно доверяют компилятору. Рядом с такой сложной программой многие разработчики ощущают себя как рядом со 107-метровым монументом «Покорителям космоса» на проспекте Мира в Москве. Современный компилятор — это очень сложная программа, и многие считают, что она не может ошибаться. Увы, это мнение неверно — чем программа сложнее, тем больше у нее возможностей ошибиться. Компилятор может выдать предупреждение на совершенно правильный код.
Вторая причина — разработчики статических анализаторов (особенно входящих в состав компиляторов) обычно считают, что ложное срабатывание — это не проблема и ничего страшного, главное ПРЕДУПРЕДИТЬ, а «разработчик должен разобраться». Разберется непременно, но далеко не факт, что в результате код станет лучше.
В обсуждениях этой проблемы проскакивают идеи, что компилятор никак не может разобраться, есть ли в коде проблема, и только «разработчик должен».
Действительно ли компилятор так безнадежно беспомощен и не может «понять» код? Самое время вспомнить об оптимизациях кода. Нетривиальные оптимизации кода — автоматическая векторизация и размотка циклов, вычисление инвариантов, удаление лишних проверок и другие полезные и умные преобразования — требуют от компилятора очень вдумчивого анализа кода.
Насколько вдумчивый анализ нужен в приведенном примере? Проверяемое значение получено от выполнения функции ftell (), ее поведение задокументировано. gcc, например, уже умеет оптимизировать — и «доламывать» не соответствующий Стандарту — код, используя требования Стандарта передавать в «строковые функции» — например, memcpy () — только ненулевые указатели, подробности есть в этой публикации. Как он это делает? Разработчики gcc добавили в него такую возможность — опознавать ряд функций как «строковые» и делать некоторые предположения о значениях передаваемых в эти функции указателей.
Точно так же разработчики компилятора могут добавить в него возможность использовать данные о поведении других функций стандартной библиотеки — вряд ли тривиально, но определенно выполнимо. Сторонние статические анализаторы также «знают» о функциях стандартной библиотеки и популярных сред, это позволяет им сообщать, например, о возможной логической ошибке в приведенном выше коде в случае, когда результат ftell () не проверяется на сообщающее об ошибке значение -1L.
Техническая возможность для значительных улучшений есть. Это вопрос приоритетов и отношения к проблеме.
Можно «научить» компилятор оптимизировать и «доламывать» не соответствующий Стандарту код с вызовом строковых функций, а можно «научить» его выдавать меньше бесполезных предупреждений.
Что на выходе? Многие средства статического анализа пока работают по принципам «главное предупредить» и «разработчик должен», этим они провоцируют разработчиков вносить в код ненужные правки, попутно иногда ломать работающий код. Можно, конечно, объявить этих разработчиков недостойными и криворукими, но не стоит забывать, что от их работы зависит судьба миллионов строк кода, используемых каждый день сотнями миллионов человек.
Статический анализ был задуман как средство содействия написанию правильного кода — для достижения этой цели средства статического анализа должны помогать разработчикам, а не мешать им бесполезными сообщениями.
Каждый раз как средство статического анализа выдает предупреждение на правильный код, где-то грустит один котик (тм) и, возможно, в совершенно правильный код вносится ненужная правка, провоцирующая очередную ошибку с последствиями как у Heartbleed.
Дмитрий Мещеряков,
департамент продуктов для разработчиков
