Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-30 Thread Christian Kruse
Hi, On 30/01/14 10:01, Tom Lane wrote: > While I haven't actually read the gettext docs, I'm pretty sure that > gettext() is defined to preserve errno. It's supposed to be something > that you can drop into existing printf's without thinking, and if > it mangled errno that would certainly not be

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-30 Thread Alvaro Herrera
Tom Lane wrote: > Christian Kruse writes: > > Have a look at the psprintf() call: we first have a _("failed to look > > up effective user id %ld: %s") as an argument, then we have a (long) > > user_id and after that we have a ternary expression using errno. Isn't > > it possible that the first _()

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-30 Thread Tom Lane
Christian Kruse writes: > Have a look at the psprintf() call: we first have a _("failed to look > up effective user id %ld: %s") as an argument, then we have a (long) > user_id and after that we have a ternary expression using errno. Isn't > it possible that the first _() changes errno? While I h

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-30 Thread Christian Kruse
Hi, On 30/01/14 10:15, Andres Freund wrote: > > While I understand most modifications I'm a little bit confused by > > some parts. Have a look at for example this one: > > > > + *errstr = psprintf(_("failed to look up effective user id %ld: %s"), > > + (long) user_i

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-30 Thread Andres Freund
On 2014-01-30 08:32:20 +0100, Christian Kruse wrote: > Hi Tom, > > On 29/01/14 20:06, Tom Lane wrote: > > Christian Kruse writes: > > > Your reasoning sounds quite logical to me. Thus I did a > > > grep -RA 3 "ereport" src/* | less > > > and looked for ereport calls with errno in it. I found quit

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-29 Thread Christian Kruse
Hi Tom, On 29/01/14 20:06, Tom Lane wrote: > Christian Kruse writes: > > Your reasoning sounds quite logical to me. Thus I did a > > grep -RA 3 "ereport" src/* | less > > and looked for ereport calls with errno in it. I found quite a few, > > attached you will find a patch addressing that issue.

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-29 Thread Tom Lane
Christian Kruse writes: > Your reasoning sounds quite logical to me. Thus I did a > grep -RA 3 "ereport" src/* | less > and looked for ereport calls with errno in it. I found quite a few, > attached you will find a patch addressing that issue. Committed. I found a couple of errors in your patch,

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-29 Thread Tom Lane
Christian Kruse writes: > Your reasoning sounds quite logical to me. Thus I did a > grep -RA 3 "ereport" src/* | less > and looked for ereport calls with errno in it. I found quite a few, > attached you will find a patch addressing that issue. Excellent, thanks for doing the legwork. I'll take c

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-29 Thread Christian Kruse
Hi, On 29/01/14 21:37, Christian Kruse wrote: > […] > attached you will find a patch addressing that issue. Maybe we should include the patch proposed in <20140129195930.gd31...@defunct.ch> and do this as one (slightly bigger) patch. Attached you will find this alternative version. Best regard

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-29 Thread Christian Kruse
Hi, On 29/01/14 13:39, Tom Lane wrote: > No, what I meant is that the ereport caller needs to save errno, rather > than assuming that (some subset of) ereport-related subroutines will > preserve it. > […] Your reasoning sounds quite logical to me. Thus I did a grep -RA 3 "ereport" src/* | less

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-29 Thread Tom Lane
Christian Kruse writes: > Ok, so I propose the attached patch as a fix. No, what I meant is that the ereport caller needs to save errno, rather than assuming that (some subset of) ereport-related subroutines will preserve it. In general, it's unsafe to assume that any nontrivial subroutine prese

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-29 Thread Christian Kruse
Hi, On 28/01/14 22:35, Tom Lane wrote: > >> Absolutely. Probably best to save errno into a local just before the > >> ereport. > > > I think just resetting to edata->saved_errno is better and sufficient? > > -1 --- that field is nobody's business except elog.c's. Ok, so I propose the attached

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Tom Lane
Andres Freund writes: >> Absolutely. Probably best to save errno into a local just before the >> ereport. > I think just resetting to edata->saved_errno is better and sufficient? -1 --- that field is nobody's business except elog.c's. regards, tom lane -- Sent via pg

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Alvaro Herrera
Stephen Frost wrote: > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > > Well, we already have Coverity reports and the VIVA64 stuff posted last > > month. Did they not see these problems? Maybe they did, maybe not, but > > since there's a large number of false positives it's hard to tell.

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Well, we already have Coverity reports and the VIVA64 stuff posted last > month. Did they not see these problems? Maybe they did, maybe not, but > since there's a large number of false positives it's hard to tell. I > don't know how many false

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Andres Freund
On 2014-01-28 18:31:59 -0300, Alvaro Herrera wrote: > Jason Petersen wrote: > > I realize Postgres’ codebase is probably intractably large to begin > > using a tool like splint (http://www.splint.org ), but this is exactly > > the sort of thing it’ll catch. I’m pretty sure it would have warned in >

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Alvaro Herrera
Jason Petersen wrote: > I realize Postgres’ codebase is probably intractably large to begin > using a tool like splint (http://www.splint.org ), but this is exactly > the sort of thing it’ll catch. I’m pretty sure it would have warned in > this case that the code relies on an ordering of side effec

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Andres Freund
On 2014-01-28 16:19:11 -0500, Tom Lane wrote: > Christian Kruse writes: > > According to http://llvm.org/bugs/show_bug.cgi?id=18644#c5 this is not > > a compiler bug but a difference between gcc and clang. Clang seems to > > use a left-to-right order of evaluation while gcc uses a right-to-left >

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Jason Petersen
I realize Postgres’ codebase is probably intractably large to begin using a tool like splint (http://www.splint.org ), but this is exactly the sort of thing it’ll catch. I’m pretty sure it would have warned in this case that the code relies on an ordering of side effects that is left undefined b

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Tom Lane
Christian Kruse writes: > According to http://llvm.org/bugs/show_bug.cgi?id=18644#c5 this is not > a compiler bug but a difference between gcc and clang. Clang seems to > use a left-to-right order of evaluation while gcc uses a right-to-left > order of evaluation. So if errmsg changes errno this w

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Christian Kruse
Hi, On 28/01/14 16:43, Christian Kruse wrote: > ereport(FATAL, > (errmsg("could not map anonymous shared memory: > %m"), >(errno == ENOMEM) ? >errhint("This error usually means that > Post

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Tom Lane
Christian Kruse writes: > just a word of warning: it seems as if there is compiler bug in clang > regarding the ternary operator when used in ereport(). While working > on a patch I found that this code: > ... > did not emit a errhint when using clang, although errno == ENOMEM was > true. Huh. I

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Christian Kruse
Hi, when I remove the errno comparison and use a 1 it works: ereport(FATAL, (errmsg("could not map anonymous shared memory: %m"), 1 ? errhint("This error usually means that PostgreSQL's request " "for a shared memory segment exceeded available me

[HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

2014-01-28 Thread Christian Kruse
Hi, just a word of warning: it seems as if there is compiler bug in clang regarding the ternary operator when used in ereport(). While working on a patch I found that this code: ereport(FATAL, (errmsg("could not map anonymous shared memory: %m"),