Bug 36424

Summary: i586 FTBFS: 2.3.2-alt1 не пересобирается посредством gcc8 по-умолчанию
Product: Sisyphus Reporter: Sergey Y. Afonin <asy>
Component: libslang2Assignee: Sergey Y. Afonin <asy>
Status: CLOSED FIXED QA Contact: qa-sisyphus
Severity: normal    
Priority: P3 CC: aen, asy, darktemplaralt, iv, ldv, vsu
Version: unstable   
Hardware: all   
OS: Linux   
Attachments:
Description Flags
slarray-ub.patch
none
slarray-ub.patch none

Description Sergey Y. Afonin 2019-03-26 15:40:22 MSK
С момента появления gcc8 в Сизифе не проходит тестовая пересборка: возникает ошибка в двух тестах: "2 tests failed:  array.sl array.slc".

Пересобирается, если для i586 указать сборку посредством gcc7, либо собирать посредством gcc8 с параметром -O0:
https://lists.altlinux.org/pipermail/devel/2019-March/207351.html
Comment 1 Dmitry V. Levin 2019-03-26 15:55:09 MSK
Надо исправить пакет slang2.
Comment 2 Dmitry V. Levin 2019-03-26 15:56:13 MSK
Только у пакета slang2 нет мантейнера.
Comment 3 Sergey Y. Afonin 2019-03-26 20:31:16 MSK
(In reply to comment #2)

> Только у пакета slang2 нет мантейнера.

Это не хорошо... А на сколько плохи варианты с gcc7, либо с -O0 для i586 на время отсутствия мантейнера?
Comment 4 Dmitry V. Levin 2019-03-26 21:25:30 MSK
(In reply to comment #3)
> (In reply to comment #2)
> 
> > Только у пакета slang2 нет мантейнера.
> 
> Это не хорошо... А на сколько плохи варианты с gcc7, либо с -O0 для i586 на
> время отсутствия мантейнера?

Очень плохи.
Comment 5 Sergey Y. Afonin 2019-04-08 08:56:35 MSK
(In reply to comment #1)

> Надо исправить пакет slang2.

Вот тут http://lists.jedsoft.org/lists/slang-users/2019/0000001.html есть патч, с ним тесты проходят (на i586, остальное не смотрел пока).  Но автор предполагает, что проблема всё же в оптимизаторе GCC.
Comment 6 Repository Robot 2019-04-08 11:31:15 MSK
slang2-2.3.2-alt2 -> sisyphus:

Mon Apr 08 2019 Sergey Y. Afonin <asy@altlinux.ru> 2.3.2-alt2
- fixed gcc8 optimization on i586 (ALT #36424)
Comment 7 AEN 2019-04-08 11:35:02 MSK
Спасибо!
Comment 8 Dmitry V. Levin 2019-04-08 14:20:05 MSK
(In reply to comment #5)
> (In reply to comment #1)
> 
> > Надо исправить пакет slang2.
> 
> Вот тут http://lists.jedsoft.org/lists/slang-users/2019/0000001.html есть патч,
> с ним тесты проходят (на i586, остальное не смотрел пока).  Но автор
> предполагает, что проблема всё же в оптимизаторе GCC.

Автор не умеет азы знаковой целочисленной арифметики: просто не в курсе, что результат знакового целочисленного переполнения не определён.  Прискорбно.
Comment 9 Aleksei Nikiforov 2019-04-08 14:26:31 MSK
(В ответ на комментарий №8)
> (In reply to comment #5)
> > (In reply to comment #1)
> > 
> > > Надо исправить пакет slang2.
> > 
> > Вот тут http://lists.jedsoft.org/lists/slang-users/2019/0000001.html есть патч,
> > с ним тесты проходят (на i586, остальное не смотрел пока).  Но автор
> > предполагает, что проблема всё же в оптимизаторе GCC.
> 
> Автор не умеет азы знаковой целочисленной арифметики: просто не в курсе, что
> результат знакового целочисленного переполнения не определён.  Прискорбно.

Не менее прискорбно и состояние кодогенерации в gcc-8, которое часто (заметно чаще, чем прошлые версии) уравнивает "неопределённое поведение" с генерацией мусора.
Comment 10 Dmitry V. Levin 2019-04-08 14:36:01 MSK
(In reply to comment #9)
> (В ответ на комментарий №8)
> > (In reply to comment #5)
> > > (In reply to comment #1)
> > > 
> > > > Надо исправить пакет slang2.
> > > 
> > > Вот тут http://lists.jedsoft.org/lists/slang-users/2019/0000001.html есть патч,
> > > с ним тесты проходят (на i586, остальное не смотрел пока).  Но автор
> > > предполагает, что проблема всё же в оптимизаторе GCC.
> > 
> > Автор не умеет азы знаковой целочисленной арифметики: просто не в курсе, что
> > результат знакового целочисленного переполнения не определён.  Прискорбно.
> 
> Не менее прискорбно и состояние кодогенерации в gcc-8, которое часто (заметно
> чаще, чем прошлые версии) уравнивает "неопределённое поведение" с генерацией
> мусора.

Это публичная позиция проекта gcc.
В каждой новой версии gcc появляются новые оптимизации, часть из них полагается на отсутствие UB.  Не хотите мусора - либо убирайте UB, либо выключайте оптимизацию.
Comment 11 Sergey Y. Afonin 2019-04-08 16:26:14 MSK
(In reply to comment #8)

> Автор не умеет азы знаковой целочисленной арифметики: просто не в курсе, что
> результат знакового целочисленного переполнения не определён.  Прискорбно.

А может быть автор в курсе, что в этом месте не может быть переполнения ввиду наличия каких-то иных условий в других частях кода? Оптимизация, как я понимаю, в new_num_elements/dims[i] упёрлась?
Comment 12 Dmitry V. Levin 2019-04-08 16:31:26 MSK
(In reply to comment #11)
> (In reply to comment #8)
> 
> > Автор не умеет азы знаковой целочисленной арифметики: просто не в курсе, что
> > результат знакового целочисленного переполнения не определён.  Прискорбно.
> 
> А может быть автор в курсе, что в этом месте не может быть переполнения

Нет, он же проверяет результат переполнения.  А результат этот не определён.
Вы видели патч, который приложили?  Там изменена проверка того самого результата переполнения, который не определён.
Comment 13 Sergey Y. Afonin 2019-04-08 16:58:10 MSK
(In reply to comment #12)

> Нет, он же проверяет результат переполнения.  А результат этот не определён.
> Вы видели патч, который приложили?  Там изменена проверка того самого
> результата переполнения, который не определён.

Видел. Добавлением проверки new_num_elements на отрицательное значение, которого в принципе может и не ожидаться никогда, но это надо уже код смотреть подробно, а то вдруг я напрасно предполагаю. Хотя там есть SLuindex_Type и не очень тогда понятно, почему автор им не воспользовался, если отрицательное значение не ожидается. Такой вот патч тоже помогает, как минимум в сборке:

--- a/slang/src/slarray.c
+++ b/slang/src/slarray.c
@@ -366,7 +366,7 @@ SLang_create_array1 (SLtype type, int read_only, VOID_STAR data,
    num_elements = 1;
    for (i = 0; i < num_dims; i++)
      {
-       SLindex_Type new_num_elements;
+       SLuindex_Type new_num_elements;
        at->dims[i] = dims[i];
        new_num_elements = dims[i] * num_elements;
        if (dims[i] && (new_num_elements/dims[i] != num_elements))

Или gcc8 как-то проверяет, что new_num_elements действительно может уйти в отрицательное значение, а не просто предполагат на основе фрагмента кода?
Comment 14 Ivan A. Melnikov 2019-04-08 17:27:38 MSK
(In reply to comment #13)
> Или gcc8 как-то проверяет, что new_num_elements действительно может уйти в
> отрицательное значение, а не просто предполагат на основе фрагмента кода?

Не очень важно, что делает gcc; просто не нужно использовать результат, значение которого не определено. Правильно было бы как-то так, наверное:

-       if (dims[i] && (new_num_elements/dims[i] != num_elements))
+       if (dims[i] && (INT_MAX/dims[i] < num_elements))

если бы мы могли сказать, что SLindex_Type всегда int.

Там, кстати, ниже, около строки 400, ещё одна похожая проверка:

http://git.altlinux.org/gears/s/slang2.git?p=slang2.git;a=blob;f=slang/src/slarray.c;h=d26aaece9c67711a96904e3bcf73f5b53b7bcc6c;hb=sisyphus#l399

Подозреваю, что там такого ещё много.
Comment 15 Dmitry V. Levin 2019-04-08 18:06:38 MSK
Зачем там вообще знаковая арифметика?
Неужели num_elements может легально принимать отрицательные значения?
Почему вообще num_elements знаковый?
Comment 16 Sergey Vlasov 2019-04-08 19:33:02 MSK
(В ответ на комментарий №14)

> -       if (dims[i] && (new_num_elements/dims[i] != num_elements))
> +       if (dims[i] && (INT_MAX/dims[i] < num_elements))

Есть вероятность, что даже такая проверка уже не поможет после того, как код с undefined behavior уже выполнен (при вычислении new_num_elements произошло переполнение).

> Подозреваю, что там такого ещё много.

Да, и не только в этом файле. Если подходить к данному вопросу формально, то даже арифметические операции в slarith.inc, выполняемые при интерпретации кода на SLang, нужно как-то защищать от переполнения (хотя с этим кодом оптимизатор вряд ли способен сделать что-то неожиданное).

Либо нужно собирать весь этот код с -fwrapv (или вообще -fno-strict-overflow — кто знает, что они там делают с указателями), поскольку имеющиеся там проверки на переполнение рассчитаны именно на такое поведение компилятора.
Comment 17 Dmitry V. Levin 2019-04-08 19:41:20 MSK
(In reply to comment #16)
> (В ответ на комментарий №14)
> Либо нужно собирать весь этот код с -fwrapv (или вообще -fno-strict-overflow —
> кто знает, что они там делают с указателями), поскольку имеющиеся там проверки
> на переполнение рассчитаны именно на такое поведение компилятора.

Я бы рекомендовал -fno-strict-overflow для всех таких пакетов.
Comment 18 Sergey Y. Afonin 2019-04-08 20:40:53 MSK
(In reply to comment #16)

> Есть вероятность, что даже такая проверка уже не поможет после того, как код с
> undefined behavior уже выполнен (при вычислении new_num_elements произошло
> переполнение).

Кстати да, действительно. Что-то я на if зациклился. А почему тогда у компилятора оптимизатор срабатывать начинает при добавлении ещё одной проверки с вероятно неопределённым значением? Или это просто "так получилось"?
Comment 19 Sergey Y. Afonin 2019-04-08 21:07:58 MSK
(In reply to comment #17)

> Я бы рекомендовал -fno-strict-overflow для всех таких пакетов.

То есть, это делает поведение предсказуемым в плане однозначного получения отрицательного значения при переполнении?
Comment 20 Sergey Y. Afonin 2019-04-09 08:36:41 MSK
Created attachment 8091 [details]
slarray-ub.patch

Автор согласен с ситуацией в плане ub и предлагает такой вариант.
Comment 21 Ivan A. Melnikov 2019-04-09 08:44:27 MSK
(In reply to comment #20)
> Created an attachment (id=8091) [details]
> slarray-ub.patch
> 
> Автор согласен с ситуацией в плане ub и предлагает такой вариант.

Из check_overflow_mult_ui я не понял условия

> (a/(SLuindex_Type)b) > UINT_MAX)
Comment 22 Sergey Y. Afonin 2019-04-09 09:00:55 MSK
(In reply to comment #21)

> Из check_overflow_mult_ui я не понял условия
> 
> > (a/(SLuindex_Type)b) > UINT_MAX)

В том плане, каким образом результат может превысить UINT_MAX, если a имеет тип UINT?
Comment 23 Sergey Y. Afonin 2019-04-09 14:09:05 MSK
(In reply to comment #21)

> Из check_overflow_mult_ui я не понял условия
> 
> > (a/(SLuindex_Type)b) > UINT_MAX)

Видимо должно быть так: (a > UINT_MAX/(SLuindex_Type)b)
Comment 24 Sergey Y. Afonin 2019-04-09 14:17:09 MSK
(In reply to comment #8)

> Автор не умеет азы знаковой целочисленной арифметики: просто не в курсе, что
> результат знакового целочисленного переполнения не определён.  Прискорбно.

Кстати, если посмотреть краткое резюме на сайте, он астрофизик и работает по специальности. И, наверное, имеет какое-то право не уследить за изменениями в C/C++. Надо просто подсказать иногда.
Comment 25 Sergey Y. Afonin 2019-04-09 21:52:37 MSK
Created attachment 8094 [details]
slarray-ub.patch

Патч со скорректированным if (см. Comment #23)
Comment 26 Sergey Y. Afonin 2019-04-10 14:47:12 MSK
Дубль два:

* Tue Apr 09 2019 Sergey Y. Afonin <asy@altlinux> 2.3.2-alt3

- added slang-2.3.2-slarray-ub.patch (fixed UB in slarray, ALT #36424#c25)
- added -fno-strict-overflow to %optflags (ALT #36424#c17)

И забыл про %% у optflags - развернулся в changelog. Надо будет добавить в следующий раз.