Как непродуманные предупреждения компиляторов помогают портить совершенно правильный код

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

    На практике далеко не всегда предупреждения компилятора одинаково полезны. Зачастую они не помогают разработчикам, а мешают им и могут провоцировать на исправление совершенно правильного кода, т.е. на нарушение правила «работает — не трогай».

    Для примера будет история об одной строке кода из довольно популярной библиотеки с открытым кодом для работы с 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 <bool = (sizeof(long) >= sizeof(size_t))>
    struct LongFitsIntoSizeTMinusOne {
        static bool Fits( long value )
        {
            return (unsigned long)value < (size_t)-1;
        }
    };
    template <> bool LongFitsIntoSizeTMinusOne<false>::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.

    Дмитрий Мещеряков,
    департамент продуктов для разработчиков
    ABBYY
    114.04
    Решения для интеллектуальной обработки информации
    Share post

    Comments 35

      +4
      С одной стороны, разработчикам нужно было проявить твёрдость и просто оставить комментарий "здесь нет ошибки". А для упоротых впихнуть прагмы, пущай спят спокойно.
      С другой стороны, есть системы, для которых файлы даже в long не влезут: те же AVR упрутся в 31 бит при работе с SD-карточками. Я понимаю, это особый случай. Но так про особые речь и идёт, всегда будут исключения, которые не удастся покрыть.
        +2
        В целом согласен, что предупреждение о сравнении signed/unsigned хитрое: на первый взгляд безобидно, на второй — может скрывать в себе подобные ужасы.
        Однако не понимаю, как компилятор должен такое ловить. Следить за любой переменной, в которую присвоили результат вызова ftell()? Сравнение с указателями здесь не слишком подходит — проверить на NULL проще.

        На мой взгляд, в данном случае проблема в API. Зачем возвращать long? Хотел было предложить использовать stat(), но stat.st_size тоже обычно signed (off_t). Хотел предложить использовать C++ ifstream.tellg() — он, оказывается, может использовать C API под капотом. Когда казалось бы обыденная операция вызывает столько боли — что-то явно не так.
        Вы, кстати, в курсе, что делать fseek() по бинарному файлу — UB? Понимаю, что библиотека работает с текстом, но факт пугающий.
        Наверное, самым правильным решением здесь будет mmap() вместо всех этих плясок с буферами.
        Вот недавняя статья на ту же тему: http://cpp.indi.frih.net/blog/2014/09/how-to-read-an-entire-file-into-memory-in-cpp/. Кое-какую информацию оттуда взял.
          +2
          Вы, кстати, в курсе, что делать fseek() по бинарному файлу — UB?
          Почему же? Что-то не могу найти такого в документации…
            +4
            Тот самый случай, когда "не в лотерею, а в карты, и не выиграл, а проиграл."

            1. Неопределенное поведение там не в смысле стандарта, а в смысле "непонятно, что будет, иногда работает, иногда нет".
            2. Наблюдается такое поведение не у бинарных, а у текстовых файлов.
              The ftell function obtains the current value of the file position indicator for the stream pointed to by stream. For a binary stream, the value is the number of characters from the beginning of the file. For a text stream, its file position indicator contains unspecified information, usable by the fseek function for returning the file position indicator for the stream to its position at the time of the ftell call; the difference between two such return values is not necessarily a meaningful measure of the number of characters written or read.
              0
              В ссылке из моего комментария цитата из стандарта.

              Setting the file position indicator to end-of-file, as with fseek(file, 0, SEEK_END), has undefined behavior for a binary stream (because of possible trailing null characters) or for any stream with state-dependent encoding that does not assuredly end in the initial shift state.

              (2011 C standard draft (N1570) §7.21.3 “Files”, footnote to paragraph 9 (footnote 268). Similar wording exists in the 1999 standard, §7.19.3.)

              Also: http://stackoverflow.com/questions/21050603/understanding-undefined-behavior-for-a-binary-stream-using-fseekfile-0-seek-e

              Это не значит, что каждый с этим столкнётся, но это UB.
                +2
                Просто у вас слишком категорический имератив вышел:

                Вы, кстати, в курсе, что делать fseek() по бинарному файлу — UB?

                Делать fseek на результат ftell можно в любом случае, а вот на SEEK_END — уже UB, У вас же получается, что там UB во всех случаях.
                  0
                  А, здесь согласен, нечётко выразился.
          +16
          Компилятор все правильно сделал — указал на весьма нетривиальное место. Тот кто писал этот код, явно не думал о всех случаях соотношения размеров size_t и long. Собственно настоящую ошибку с обработкой больших файлов, программисты так и не исправили. Правильно в этом месте использовать fseeko и ftello, а коммиты с fseek и ftell вообще не должны попадать в репозиторий.
            +1
            Изначально проверка была написана с учетом возможно разных размеров size_t и long — оба значения по умолчанию были бы неявно приведены к одному и тому же беззнаковому типу достаточного размера.
          • UFO just landed and posted this here
              0
              Компилятор-то правильно указывает на то, что код кривой. Если уж автор предполагает, что размер файла может вылезти за size_t, то почему он не интересуется, как будет себя вести стек, если в нем выделить соответствующий кусок памяти?

              Вторая ,, кривизна'' — это использование fseek/ftell вместо fseeko/ftello.
                +3
                А откуда видно, что память выделяется на стеке? В коде явно написано:

                    const size_t size = filelength;
                    TIXMLASSERT( _charBuffer == 0 );
                    _charBuffer = new char[size+1];

                И проблема разработчиков — в том, что, несмотря на использование , они не использовали шаблон std::numeric_limits, который прямо описан, как определённый для арифметических типов и типов, которые через них определены (в том числе и size_t). Это, конечно, в дополнение к использованию fseek вообще.
                  –3
                  Да, new проглядел. Чаще пользую C.
                    +1
                    При компиляции с -std=c++11 clang может выдавать -Wtautological-constant-out-of-range-compare даже при сравнении с std::numeric_limits<size_t>::max()
                  0
                  Интересно, что бы сказали компиляторы на код
                  if( (unsigned long)filelength >= min((size_t)-1,(unsigned long)-1) ) {
                  При каких соотношениях типов будет предупреждение?
                    +4
                    Ну, да, в местах объединения С кода с С++ могут возникать сложности. Но все же они гораздо меньшего масштаба, чем при объединении кода других различных языков программирования. Тем более, что все решается стандартным путем

                    f (static_cast<std::common_type<decltype(x), std::size_t>::type>(x) >= std::numeric_limits<std::size_t>::max()) {
                      0
                      В статье вроде как сказано, что C++11 для расширения аудитории, стараются не использовать. А вот что стало с памятью
                      _charBuffer
                      
                      после срабатывания
                      if( read != size )
                      
                      — вот это интересно. Код не смотрел на гитхабе.
                        0
                        Ну сейчас все-таки 2016 год, С++11 вышел уже 5 лет назад; и gcc 4.4 этот код уже поддерживает
                      +1
                      Боже мой, что это за издевательство над здравым смыслом! Шаблоны-маблоны.

                      typedef unsigned long filesize_t; // логично, что размер файла не может быть отрицательным?
                      
                      const size_t max_memory_size = (size_t)-1;
                      const filesize_t max_memoized_filesize = (filesize_t) max_memory_size;
                         // сделаем явный каст - показывая, что можно как срезать, так и расширить
                      
                      filesize_t filelength;
                      if (filelength < max_memoized_filesize) { ... }
                      
                      // ну или всю эту дребедень в одну строку! если уж хочется, чтобы коллеги каждый раз чесали репу
                      if ((unsigned long)filelength < (unsigned long)(size_t)-1) { ... }
                        –1
                        Плохо, что это в рантайме делается, а хотелось бы в compile time разрулить ситуацию.
                          +2
                          Что делается в рантайме и надо перенести в компиляцию?!
                          Фактический размер файла?
                          Или -1 превращается в filesize_t в рантайме?
                        +8
                        Во-первых, все замечательно решается без шаблонов и предупреждений:
                        if (filelength & ~(unsigned long)(size_t)-1) {
                        /* error fillength is too large */
                        }


                        Во-вторых, считается хорошим тоном нужность подобных проверок определять в configure time (когда запускается cmake или иной аналог configure-скриптов), соответственно в итоге мы никогда не получим «condition always true» для совпадающих по размеру unsigned long и size_t.

                        В-третьих, если почитать стандарт, то можно написать намного понятнее и также без предупреждений (gcc-4.8.5):
                        #include <limits.h>

                        if (filelength > SIZE_MAX) {
                        /* file too large */
                        }
                        • UFO just landed and posted this here
                            +1
                            Для третьего способа нужно включать stdint.h, и на такую проверку clang тоже может выдавать -Wtautological-constant-out-of-range-compare
                          • UFO just landed and posted this here
                              +1
                              при написании кода на obj-c в Xcode — лично встречается 2 случая, когда рекомендуется мутить компилятор clang прагмами.

                              В первом случае — когда объекту базового класса (вообще-то динамически типизированный язык) пытаюсь передать селектор (нечто вроде указателя на функцию) конкретного класса — он выдает варнинг, что не факт, что метод с таким селектором будет у конечного объекта. Приходится мутить, потому-что перед этим обычно выполняется ручная проверка на то, есть ли данный селектор в таблице селектора объекта (таблица указателей методов). И компилятор не понимает эту проверку!

                              И еще довольно специфичная работа с памятью в iOS (что-то типа умных указателей со счетчиком ссылок), которые живут в retain-циклах (retain увеличивает число ссылок на 1). Существуют случаи, когда приходится мутить сообщения комилятора, связанные с его анализом этих циклов (особенно при использовании блоков). Блок — это что-то вроде функции, определенной внутри другой функции (функции второго порядка). Причем данного рода прагмы я встречал при анализе кода более, чем в нескольких разных фреймворках

                              Так и живем (по поводу торжества компилятора на наших костях) ...
                                +1
                                Верните gcc 2.95 !

                                И да, в c/c++ сильно не хватает @suppresswarnings из java
                                +2
                                Перед выделением памяти стоит проверить, поместится ли требуемый размер в size_t, чтобы избежать труднодиагностируемых логических ошибок при дальшейшей работе.

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

                                template <typename T1, typename T2>
                                static bool Fits(T1 &, T2 &)
                                {
                                    return sizeof(T1) >= sizeof(T2);
                                }

                                const size_t size = filelength;
                                if( ! Fits(size, filelength) ) {
                                    return FILE_READ_ERROR;
                                }
                                  +2
                                  Хорошо, возьмем систему, где long восемь байт, а size_t четыре байта (например, при компиляции gcc под 32-разрядную систему). Размер файла, допустим, пять килобайт. Предлагаемая вами функция Fits() вернет "false", хотя число около пяти тысяч вполне помещается в четырехбайтный size_t.
                                    +1
                                    О, я не сразу понял масштаб бедствия.
                                    Вы понимаете, насколько ужасен код, если посторонние люди даже с комментариями, не могут понять, что он должен делать? :)
                                    А в этом случае вас не будут смущать ворнинги типа "преобразование к меньшему типу" в этой строке?

                                    const size_t size = filelength;

                                    Но хорошо, так еще проще.

                                        const size_t size = filelength;
                                        if ((long)size != filelength ) {
                                            return FILE_READ_ERROR;
                                        }

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

                                    Но подождите, нужно еще прибавить единичку (оставим в стороне вопрос, зачем это нужно), и костыли, которые раньше неявно учитывали кейс с переполнением, теперь отсутствуют. (А что, если придется прибавить 2, а не 1?)
                                    Придется немного усовершенствовать.

                                        const size_t size = filelength;
                                        if ((long)size != filelength || (size + 1) == 0) ) {
                                            return FILE_READ_ERROR;
                                        }
                                      +1
                                      Предлагаете явное приведение беззнакового типа к знаковому. При преобразовании беззнакового типа в знаковый, если исходное значение не может быть представлено в результирующем типе, результат преобразования зависит от реализации (implementation-defined). Вы уверены, что стоит полагаться на зависящее от реализации поведение в коде для обработки довольно редких ситуаций, предназначенном для неизвестного заранее множества платформ и компиляторов?
                                        +1
                                        Теоретически, исходное значение не может быть представлено, если размер long меньше, чем значение в size_t, т.е. sizeof(size_t) > sizeof(long). Практически, в этом случае (с учетом того, что отрицательные значения в filelength — уже само по себе странно) значение в size_t никогда не может быть больше, чем, исходное значение в long filelength.

                                        3 If the destination type is signed, the value is unchanged if it can be represented in the destination type (and
                                        bit-field width); otherwise, the value is implementation-defined.
                                  +3
                                  Из личного опыта смелой профессиональной молодости. Был backend-демон, который контролировал единственность запущенного инстанса аж сразу двумя способами — блокировка pid-файла и pid-порта (реализация была сделана вручную, без врапперов). IDEA подсветила обе static-переменные как "can be converted to local variables", а я взял и поддался. Телефонный звонок тимлида застал меня в отпуске и я на втором слове понял, какую дурацкую ошибку я совершил (java gc закрывал эти объекты спустя какое-то время после старта приложения).
                                    +2
                                    … и снова ошибка на самом деле совсем не там, где ее видит IDE :) Если бы для этих объектов использовался try-with-resources (ну или просто вызывался метод close) — то gc бы такой объект не собрал раньше времени.

                                  Only users with full accounts can post comments. Log in, please.