longjmp clobber warnings are utterly broken in modern gcc
От | Tom Lane |
---|---|
Тема | longjmp clobber warnings are utterly broken in modern gcc |
Дата | |
Msg-id | 13955.1422212567@sss.pgh.pa.us обсуждение исходный текст |
Ответы |
Re: longjmp clobber warnings are utterly broken in modern gcc
(Greg Stark <stark@mit.edu>)
Re: longjmp clobber warnings are utterly broken in modern gcc (Martijn van Oosterhout <kleptog@svana.org>) Re: longjmp clobber warnings are utterly broken in modern gcc (Andres Freund <andres@2ndquadrant.com>) Re: longjmp clobber warnings are utterly broken in modern gcc (Robert Haas <robertmhaas@gmail.com>) |
Список | pgsql-hackers |
I've been looking for other instances of the problem Mark Wilding pointed out, about missing "volatile" markers on variables that are modified in PG_TRY blocks and then used in the PG_CATCH stanzas. There definitely are some. Current gcc versions do not warn about that. If you turn on -Wclobbered, you don't always get warnings about the variables that are problematic, and you do get warnings about variables that are perfectly safe. (This is evidently why that option is not on by default: it's *useless*, or even counterproductive if it gives you false confidence that the compiler is protecting you.) I thought maybe the gcc folk no longer care about this because the compiler is now smart enough to compile safe code in these situations. A simple experiment disabused me of that notion. I took guc.c's AlterSystemSetConfigFile, which is at risk because of this sequence: int Tmpfd = -1; ... Tmpfd = open(AutoConfTmpFileName, O_CREAT | O_RDWR | O_TRUNC, S_IRUSR | S_IWUSR); if (Tmpfd < 0) ereport(ERROR, (errcode_for_file_access(), errmsg("could not open file \"%s\": %m", AutoConfTmpFileName))); PG_TRY(); { ... close(Tmpfd); Tmpfd = -1; ... } PG_CATCH(); { if (Tmpfd >= 0) close(Tmpfd); ... } PG_END_TRY(); and compared the assembly language generated with and without adding "volatile" to Tmpfd's declaration. Without "volatile" (ie, in the code as shipped), gcc optimizes away the assignment "Tmpfd = -1" within PG_TRY, and it also optimizes away the if-test in PG_CATCH, apparently believing that control cannot transfer from inside the PG_TRY to the PG_CATCH. This is utterly wrong of course. The issue is masked because we don't bother to test for a failure return from the second close() call, but it's not hard to think of similar coding patterns where this type of mistaken optimization would be disastrous. (Even here, the bogus close call could cause a problem if we'd happened to open another file during the last part of the PG_TRY stanza.) Not only does -Wclobbered fail to point out this risk, but to add insult to injury it does whinge about two *other* variables in the same function that are actually perfectly safe. I'm not sure what algorithm it's using to decide what to warn about, but the algorithm has approximately nothing to do with reality. I tested this on gcc 4.4.7 (current on RHEL6), and gcc 4.8.3 (current on Fedora 20); they behave the same. Even if this were fixed in bleeding-edge gcc, we'd definitely still need the "volatile" marker to get non-broken code compiled on most current platforms. This is scary as hell. I intend to go around and manually audit every single PG_TRY in the current source code, but that is obviously not a long-term solution. Anybody have an idea about how we might get trustworthy mechanical detection of this type of situation? regards, tom lane
В списке pgsql-hackers по дате отправления:
Следующее
От: Greg StarkДата:
Сообщение: Re: longjmp clobber warnings are utterly broken in modern gcc