Bug 36038 - gcc8-c++ miscompiles functions returning non-void without return statements
Summary: gcc8-c++ miscompiles functions returning non-void without return statements
Status: CLOSED FIXED
Alias: None
Product: Sisyphus
Classification: Development
Component: gcc8-c++ (show other bugs)
Version: unstable
Hardware: all Linux
: P3 normal
Assignee: Gleb F-Malinovskiy
QA Contact: qa-sisyphus
URL: https://gcc.gnu.org/bugzilla/show_bug...
Keywords:
Depends on:
Blocks:
 
Reported: 2019-02-04 18:39 MSK by Aleksei Nikiforov
Modified: 2019-02-12 13:59 MSK (History)
6 users (show)

See Also:


Attachments
gxx-bug-36038.cpp (441 bytes, text/x-c++src)
2019-02-05 10:36 MSK, Aleksei Nikiforov
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Aleksei Nikiforov 2019-02-04 18:39:29 MSK
Если есть функция, возвращающая bool, но в данной функции нет ни одного return, то при использовании gcc8-c++ генерируется очень странный ассемблерный код.

Есть два примера: synaptic и packagekit.

synaptic:

http://git.altlinux.org/gears/s/synaptic.git?p=synaptic.git;a=blob;f=synaptic/gtk/rguserdialog.cc;h=83349c1e64c61013dac72fe293a8055d84558b36;hb=cfd2d6d12072ecb8bd1c38cdd1cdf39c71e1ba28#l211

Функция bool RGGladeUserDialog::init(const char *name).

При сборке с gcc8-c++ генерируется ассемблерный код, в котором в конце функции стоит jump в середину функции. При продолжении выполнения данной функции после этого прыжка в итоге второй раз вызывается g_free(filename) и приложение падает.

Имеется уже закрытый соответствующий баг:
https://bugzilla.altlinux.org/show_bug.cgi?id=35725

Воспроизведение: пересобрать synaptic без патча synaptic-0.58-alt-gcc8-crash-fix.patch (либо взять версию 0.58-alt17) и запустить, приложение падает. Если же не падает, то открыть пункт меню "Справка" -> "Краткое описание".

packagekit:

http://git.altlinux.org/gears/p/packagekit.git?p=packagekit.git;a=blob;f=backends/aptcc/apt-messages.cpp;h=38c58b16b9907dcbeee318fc4618ea7b1496d119;hb=8a5988e96d431b69b98efd438b289e0a7664ab71#l32

Функция bool show_errors(PkBackendJob *job, PkErrorEnum errorCode, bool errModify).

В данном случае падает в выражении errors.str().empty() на строке 55.

Воспроизведение: взять packagekit версии 1.1.12-alt4, машину с Sisyphus, которую не обновляли несколько дней, запустить apt-get update, apt-get dist-upgrade в одном терминале, дождаться пока apt-get запросит подтверждение на обновление. Во втором терминале запустить pkcon update и дождаться запроса на подтверждение обновления. После этого в первом терминале подтвердить обновление через apt-get dist-upgrade, и когда оно начнётся - подтвердить во втором терминале обновление через pkcon update. Если dist-upgrade будет проходить достаточно долго (более 10 секунд), то обновление через pkcon update не сможет получить нужные локи, получит timeout и попытается сообщить соответствующую ошибку, и упадёт пытаясь это сделать.

Если изменить возвращаемый тип на void в этой функции, то проблема пропадает.

В обоих случаях gcc8-c++ выдаёт предупреждение:
apt-messages.cpp: In function 'bool show_errors(PkBackendJob*, PkErrorEnum, bool)':
apt-messages.cpp:61:1: warning: no return statement in function returning non-void [-Wreturn-type]
 }

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

Как минимум для synaptic при использовании gcc7-c++ такой проблемы замечено не было. Я считаю, что gcc8-c++ должен либо генерировать код, аналогичный более старым версиям gcc, либо должен выдавать ошибки, а не предупреждения, вместо генерации "сюрпризов".
Comment 1 Dmitry V. Levin 2019-02-04 22:30:24 MSK
(In reply to comment #0)
> Я считаю, что gcc8-c++ должен либо генерировать код, аналогичный более
> старым версиям gcc, либо должен выдавать ошибки, а не предупреждения, вместо
> генерации "сюрпризов".

Было бы неплохо, но поскольку это UB, компилятор вправе делать всё что угодно.
Comment 2 Aleksei Nikiforov 2019-02-05 10:09:24 MSK
(В ответ на комментарий №1)
> (In reply to comment #0)
> > Я считаю, что gcc8-c++ должен либо генерировать код, аналогичный более
> > старым версиям gcc, либо должен выдавать ошибки, а не предупреждения, вместо
> > генерации "сюрпризов".
> 
> Было бы неплохо, но поскольку это UB, компилятор вправе делать всё что угодно.

Я как знал, что такой комментарий будет :)
Да, это скорее всего UB, но поведение g++-8 по сравнению с g++-7 - явная деградация.
Comment 3 Aleksei Nikiforov 2019-02-05 10:36:47 MSK
Created attachment 7988 [details]
gxx-bug-36038.cpp

Файл с примером, на котором g++ генерирует кривой код.

На машине x86_64 при использовании g++-7 -O2 генерируется нормальный код и получается следующий вывод:
Calling function
Running 1 time
Successfully returned from function

При использовании g++-8 -O0 внезапно тоже всё хорошо, аналогично прошлому случаю. Однако, при использовании g++-8 -O2 внезапно программа падает:
Calling function
Running 1 time
Ошибка сегментирования (стек памяти сброшен на диск)

Если посмотреть код функции somefunction, то при использовании g++-8 -O2 получается следующий код:
(gdb) $ disassemble  somefunction  
Dump of assembler code for function somefunction():
   0x00000000004011e0 <+0>:     sub    $0x8,%rsp
   0x00000000004011e4 <+4>:     mov    $0x1,%edx
   0x00000000004011e9 <+9>:     mov    $0x402010,%esi
   0x00000000004011ee <+14>:    xor    %eax,%eax
   0x00000000004011f0 <+16>:    mov    $0x1,%edi
   0x00000000004011f5 <+21>:    callq  0x401030 <__printf_chk@plt>

Даже никакого ret нет, неудивительно что падает. В других вариантах (с g++-7 или с -O0) код функции намного длиннее и содержит retq.
Comment 4 Anton Farygin 2019-02-05 10:40:03 MSK
Дим, а давай включим Werror для этого предупреждения и посмотрим что сломается при сборке ?
Comment 5 Dmitry V. Levin 2019-02-05 15:05:48 MSK
(In reply to comment #2)
> (В ответ на комментарий №1)
> > (In reply to comment #0)
> > > Я считаю, что gcc8-c++ должен либо генерировать код, аналогичный более
> > > старым версиям gcc, либо должен выдавать ошибки, а не предупреждения, вместо
> > > генерации "сюрпризов".
> > 
> > Было бы неплохо, но поскольку это UB, компилятор вправе делать всё что угодно.
> 
> Я как знал, что такой комментарий будет :)
> Да, это скорее всего UB, но поведение g++-8 по сравнению с g++-7 - явная
> деградация.

Понятно же, почему так происходит: в gcc8 стало больше всяких оптимизаций, которые в случае UB делают фиг знает что.

(In reply to comment #4)
> Дим, а давай включим Werror для этого предупреждения и посмотрим что сломается
> при сборке ?

Было бы неплохо, но -Wreturn-type охватывает не только "no return statement in function returning non-void", но и другие.

В нынешнем Сизифе "no return statement in function returning non-void" встречается в логе сборки 98 пакетов, а всего предупреждения от -Wreturn-type встречаются в логе сборки 277 пакетов.
Comment 6 Aleksei Nikiforov 2019-02-05 17:31:09 MSK
(В ответ на комментарий №5)
> Было бы неплохо, но -Wreturn-type охватывает не только "no return statement in
> function returning non-void", но и другие.
> 
> В нынешнем Сизифе "no return statement in function returning non-void"
> встречается в логе сборки 98 пакетов, а всего предупреждения от -Wreturn-type
> встречаются в логе сборки 277 пакетов.

Я читаю это так: "в Сизифе до 98 пакетов потенциально сломано, но пересобрать надо будет 277 пакетов".

Возможно не каждое предупреждение говорит о подобной ошибке. Вопрос в том, сколько из этих пакетов реально уже незаметно сломано с gсс8-с++ (как минимум 1 уже починен, ещё 1 известен), сколько сломается при первой же пересборке, и когда на эти проблемы кто-нибудь нарвётся. И хорошо, если проблема будет вылезать сразу в виде крэша. Два случая, что я видел, приводят к крэшу. Но можно представить сценарии и похуже, UB ведь.
Comment 7 Dmitry V. Levin 2019-02-06 04:37:03 MSK
(In reply to comment #6)
> (В ответ на комментарий №5)
> > Было бы неплохо, но -Wreturn-type охватывает не только "no return statement in
> > function returning non-void", но и другие.
> > 
> > В нынешнем Сизифе "no return statement in function returning non-void"
> > встречается в логе сборки 98 пакетов, а всего предупреждения от -Wreturn-type
> > встречаются в логе сборки 277 пакетов.
> 
> Я читаю это так: "в Сизифе до 98 пакетов потенциально сломано, но пересобрать
> надо будет 277 пакетов".

Можно сделать так, чтобы gcc и/или g++ генерили ошибку вместо предупреждения только для "no return statement in function returning non-void".

Непонятно, сколько пакетов на самом деле потенциально сломано, и как пофиксить 98 пакетов.
Comment 8 Dmitry V. Levin 2019-02-06 05:24:29 MSK
Вот пример попроще:

static int foo(void)
{
}

int main(void)
{
    return foo();
}

gcc на нём делает обычный код:

main:
.LFB1:
	.cfi_startproc
	xorl	%eax, %eax
	ret
	.cfi_endproc

g++ на нём делает код, который падает:

main:
.LFB1:
	.cfi_startproc
	.cfi_endproc
Comment 9 Aleksei Nikiforov 2019-02-06 17:18:08 MSK
Нашёл соответствующий апстримный баг:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87515

tl;dr: won't fix, используйте -Werror=return-type.
Comment 10 Dmitry V. Levin 2019-02-07 00:36:47 MSK
Upstream statement:

C++ says it is undefined even if you don't use the return value.  This is different from C.

GCC assumes that the path leading to the return without a value will not happen so it uses __builtin_unreachable call there.
Comment 11 Dmitry V. Levin 2019-02-07 00:40:00 MSK
В качестве решения предлагаю сделать "no return statement in function returning non-void" неотключаемой ошибкой в g++.
Comment 12 Anton Farygin 2019-02-07 07:39:34 MSK
Давайте так и сделаем.
Comment 13 Aleksei Nikiforov 2019-02-07 10:25:36 MSK
Если починить g++ нельзя, то других вариантов не остаётся.
Comment 14 Dmitry V. Levin 2019-02-07 13:25:07 MSK
Как минимум, следующие 45 пакетов нуждаются в починке:

AlephOne-1.0.1-alt1.2
CGenius-1.9.9.6beta-alt1
OpenAD-20140315-alt4
antico-deluxe-0.1.96-alt1.2
arpage-0.3.3-alt3_24
bloboats-1.0.2-alt2
canorus-0.6svn-alt1.qa2
cooldown-24-alt1.1
duel3-0.1-alt3_0.24.20060225.qa1
extrema-4.4.5-alt2
fbg-0.9.1-alt3_12
fceux-2.2.3-alt1
freehdl-0.0.8-alt4
glob2-0.9.4.4-alt1.qa7.1
gnome-quod-0.2.3-alt2
imule-1.4.6-alt5
kbookocr-2.1.0-alt2
kismet-2014.02.R1-alt1
kumir-1.9.1.2810-alt2
lib2geom-20081103-alt1.5
libfreeimage-3.18.0-alt1
libplotmm-0.1.2-alt2
mapsoft-20180722-alt1
megafuse-1.0.0-alt3.3
mirall-1.0.2-alt5
nted-1.10.18-alt3
numptyphysics-0.3.160-alt1
openscada-0.9.0-alt2
packagekit-1.1.12-alt4
paris-traceroute-0.92-alt1
perl-JavaScript-V8-0.070-alt7.1
pfstools-2.1.0-alt3
printer-driver-splix-2.0.1-alt1.svn315
procbench-0.9.0a-alt3.1
qcat-0.5-alt5.1.qa1
qkismet-0.3.1-alt1.qa1
qt-fsarchiver-0.8.4.0-alt1
qtrainer-0.5.2-alt2.qa2
sprng-4.4-alt1
synaptic-0.58-alt20
verlihub-0.9.8e-alt2
verlihub-plugin-python-1.1-alt2.1.1.1
verlihub-plugins-0.1-alt2.qa2.1
verlihub-plugins-lua51-1.8.1-alt1.qa2
yaafe-0.64-alt2.git20130420
Comment 15 Dmitry V. Levin 2019-02-08 03:26:35 MSK
g++ -Wreturn-type генерит три вида ошибок:
1. no return statement in function returning non-void
2. control reaches end of non-void function
3. ISO C++ forbids declaration of 'main' with no type

Первый вариант мы уже обсудили, в Сизифе таких пакетов на С++ минимум 45.

Второй вариант не намного лучше первого, g++ точно так же генерит __builtin_unreachable() в конце функции, после чего в результате оптимизации от такой функции мало чего остаётся.  В Сизифе таких пакетов на С++ минимум 60, из которых 25 уже проходят по первому варианту.

Третий вариант выглядит безобидно, в Сизифе таких пакетов 5, из которых 2 уже проходят по первому варианту.

Всего пакетов на С++, которые в результате включения -Werror=return-type в g++ перестанут собираться, как минимум 83.
Comment 16 Anton Farygin 2019-02-08 07:40:12 MSK
Серёг, посмотри, как мы сможем помочь с исправлениями в этих пакетах ?
Comment 17 Sergey V Turchin 2019-02-08 11:00:16 MSK
(В ответ на комментарий №16)
> Серёг, посмотри, как мы сможем помочь с исправлениями в этих пакетах ?
Да, всем сможем. Наверняка там в большинстве случаев в контекст не нужно особо вдаваться.
Comment 18 Dmitry V. Levin 2019-02-08 17:09:04 MSK
Я решил просто добавить -Werror=return-type в g++ и не делать ошибку неотключаемой, поскольку в теории g++ может не увидеть, что из функции, в которой реализуется первый или второй вариант -Wreturn-type, на самом деле есть выход через какую-нибудь неразмеченную noreturn- или throw-функцию.
Comment 19 Anton Farygin 2019-02-09 08:50:01 MSK
отличное сбалансированное решение.
Может быть, перед или после выкладывания стоит рассказать об этой проблеме в рассылке (как и о стандартных способах починки) ?
Comment 20 Repository Robot 2019-02-09 12:24:34 MSK
gcc8-8.2.1-alt4 -> sisyphus:

Thu Feb 07 2019 Dmitry V. Levin <ldv@altlinux> 8.2.1-alt4
- Added ppc64le support (by glebfm@).
- Fixed profiledbootstrap build (by glebfm@).
- g++: enabled -Werror=return-type by default (closes: #36038).
- libcc1.so.0: cleaned up using a fixed libtool (closes: #36045).