Re: [HACKERS] Further news on Clang - spurious warnings
I realize it's a bit late to jump in here with the path already having been committed. But I think there's a point that was missed in the discussion. One reason to do the test as Tom recommended is that the warning probably indicates that the test as written was just going to be optimized away as dead code. I think the cast to unsigned is the least likely idiom to be optimized away whereas any of the formulations based on comparing the enum with enum labels is quite likely to be.
Re: [HACKERS] Further news on Clang - spurious warnings
On fre, 2011-08-12 at 11:46 -0700, David E. Wheeler wrote: I figure there might be warnings you haven't seen if you haven't been building with bonjour, perl, openssl, pam, libxml, or ossp-uuid, so I've attached the output. We have a build farm member (smew) that covers all of that except bonjour, but I can confirm that there are no additional warnings on account of that. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On Aug 12, 2011, at 12:09 PM, Peter Eisentraut wrote: I figure there might be warnings you haven't seen if you haven't been building with bonjour, perl, openssl, pam, libxml, or ossp-uuid, so I've attached the output. We have a build farm member (smew) that covers all of that except bonjour, but I can confirm that there are no additional warnings on account of that. Ah, great, thanks! David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 12 August 2011 20:10, David E. Wheeler da...@kineticode.com wrote: Ah, great, thanks! Unfortunately, you'll need to build your own Clang to be up to speed on the work I've done here, because the Clang developers both fixed the spurious warning from assigning past what appears to be the end of an array, plus a significant performance issue when building gram.c and other grammar files. Building Clang is not difficult, but it's a little time-consuming. I imagine we'll have to wait until the release and wide availability of Clang 3 before we start to see people using it for day-to-day hacking on Postgres. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 5 August 2011 20:07, Tom Lane t...@sss.pgh.pa.us wrote:
This patch is a truly horrid idea, as it eliminates the error checking
the compiler is trying to provide, and does so globally not only in the
trouble spots.
I think that the mistake I may have made here was assuming that the
existing code was perfect, and therefore it was a simply matter of
tweaking. With that assumption in mind, the only fix could have been a
global fix. I should not have acted in haste.
If I were trying to get rid of this warning, I'd be wondering why
ReplNodeTag is a distinct enum in the first place.
Indeed, that doesn't seem to be justified anywhere, and seems to be a
violation of the abstraction of Node as a pseudo base class which
would have broken any downcasting that we might have attempted to
do. Since ReplNodeTag wasn't being used directly, just its enumeration
constants, simply moving the constants to the global, generic NodeTag
enum fixes the issue.
That is what the attached patch does.
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index d8bc6b8..2c5e162 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -403,6 +403,13 @@ typedef enum NodeTag
T_CommonTableExpr,
/*
+ * TAGS FOR REPLICATION PARSER (replnodes.h)
+ */
+ T_IdentifySystemCmd = 950,
+ T_BaseBackupCmd,
+ T_StartReplicationCmd,
+
+ /*
* TAGS FOR RANDOM OTHER STUFF
*
* These are objects that aren't part of parse/plan/execute node tree
@@ -410,7 +417,7 @@ typedef enum NodeTag
* purposes (usually because they are involved in APIs where we want to
* pass multiple object types through the same pointer).
*/
- T_TriggerData = 950, /* in commands/trigger.h */
+ T_TriggerData = 1000, /* in commands/trigger.h */
T_ReturnSetInfo, /* in nodes/execnodes.h */
T_WindowObjectData, /* private in nodeWindowAgg.c */
T_TIDBitmap,/* in nodes/tidbitmap.h */
diff --git a/src/include/replication/replnodes.h b/src/include/replication/replnodes.h
index e027f92..8523e7e 100644
--- a/src/include/replication/replnodes.h
+++ b/src/include/replication/replnodes.h
@@ -18,16 +18,6 @@
#include nodes/primnodes.h
#include nodes/value.h
-/*
- * NodeTags for replication parser
- */
-typedef enum ReplNodeTag
-{
- T_IdentifySystemCmd = 10,
- T_BaseBackupCmd,
- T_StartReplicationCmd
-} ReplNodeTag;
-
/* --
* IDENTIFY_SYSTEM command
* --
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
Peter Geoghegan pe...@2ndquadrant.com writes: On 5 August 2011 20:07, Tom Lane t...@sss.pgh.pa.us wrote: If I were trying to get rid of this warning, I'd be wondering why ReplNodeTag is a distinct enum in the first place. Indeed, that doesn't seem to be justified anywhere, and seems to be a violation of the abstraction of Node as a pseudo base class which would have broken any downcasting that we might have attempted to do. Since ReplNodeTag wasn't being used directly, just its enumeration constants, simply moving the constants to the global, generic NodeTag enum fixes the issue. That is what the attached patch does. I did this plus moving replnodes.h into a saner spot: if they're full-fledged Nodes, they ought to be defined in src/include/nodes/. I did not renumber the existing node tags as your patch suggests. We might want to do that at some point to make a bit more breathing room, but I think it's too late in the 9.1 cycle to be doing that in 9.1. People have probably already built third-party code that incorporates values like T_ReturnSetInfo. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On Thu, Aug 4, 2011 at 4:49 PM, Peter Geoghegan pe...@2ndquadrant.com wrote: Can we please commit a fix for this problem? Done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 5 August 2011 17:12, Robert Haas robertmh...@gmail.com wrote: On Thu, Aug 4, 2011 at 4:49 PM, Peter Geoghegan pe...@2ndquadrant.com wrote: Can we please commit a fix for this problem? Done. Thanks Robert. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
Now, apart from the Flex warning, there are just 3 warnings left. They all look like this: repl_gram.y:106:30: warning: implicit conversion from enumeration type 'enum ReplNodeTag' to different enumeration type 'NodeTag' (aka 'enum NodeTag') [-Wconversion] (yyval.node) = (Node *) makeNode(IdentifySystemCmd); ^~~ ../../../src/include/nodes/nodes.h:475:64: note: expanded from: #define makeNode(_type_)((_type_ *) newNode(sizeof(_type_),T_##_type_)) ^ scratch space:180:1: note: expanded from: T_IdentifySystemCmd ^ ../../../src/include/nodes/nodes.h:452:19: note: expanded from: _result-type = (tag); \ ~ ^~~ Attached patch fixes all 3 warnings with an explicit cast, so the number of warnings with Clang is the same number as GCC 4.5 - 1. On GCC 4.6, there are still quite a few -Wunused-but-set-variable warnings left despite an effort to eradicate them. Perhaps I should look into that next. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h index d8bc6b8..5a87f92 100644 --- a/src/include/nodes/nodes.h +++ b/src/include/nodes/nodes.h @@ -472,7 +472,7 @@ extern PGDLLIMPORT Node *newNodeMacroHolder; #endif /* __GNUC__ */ -#define makeNode(_type_) ((_type_ *) newNode(sizeof(_type_),T_##_type_)) +#define makeNode(_type_) ((_type_ *) newNode(sizeof(_type_), (NodeTag) T_##_type_)) #define NodeSetTag(nodeptr,t) (((Node*)(nodeptr))-type = (t)) #define IsA(nodeptr,_type_) (nodeTag(nodeptr) == T_##_type_) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
Peter Geoghegan pe...@2ndquadrant.com writes: Now, apart from the Flex warning, there are just 3 warnings left. They all look like this: repl_gram.y:106:30: warning: implicit conversion from enumeration type 'enum ReplNodeTag' to different enumeration type 'NodeTag' (aka 'enum NodeTag') [-Wconversion] Attached patch fixes all 3 warnings with an explicit cast, This patch is a truly horrid idea, as it eliminates the error checking the compiler is trying to provide, and does so globally not only in the trouble spots. If I were trying to get rid of this warning, I'd be wondering why ReplNodeTag is a distinct enum in the first place. Surely it does not make sense to be using the Node mechanisms on something that isn't conforming to the standard node numbering. If these nodes ever got passed to anything else in the system, they'd get misinterpreted. So I'm inclined to think that clang has pointed out a real issue, rather than something we ought to paper over. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
Peter Geoghegan pe...@2ndquadrant.com writes: On 3 August 2011 21:03, Tom Lane t...@sss.pgh.pa.us wrote: I mean that it's unclear what you'll get if status has a bitpattern equivalent to a negative integer. If the compiler implements the comparison as signed, the test will yield TRUE; if unsigned, it's FALSE. On compilers on which the enum value is represented as an unsigned int, passing -1 is just the same as doing that with any function with an unsigned int argument for that argument - it will convert to a large unsigned value . So sure, that isolated part of the test's outcome will vary depending on whether or not the compiler opts to represent the enum as signed when it can, but I don't look at it as you do. I simply consider that to be a violation of the enum's contract, or the caller's failure to consider that the enum couldn't represent -1 -- they got what they asked for. This argument is completely missing the point of the test, which is to verify whether or not the caller adhered to the enum's contract. You can *not* assume that he did while arguing about whether the test is valid or accomplishes its goals. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 4 August 2011 07:08, Tom Lane t...@sss.pgh.pa.us wrote: Peter Geoghegan pe...@2ndquadrant.com writes: On 3 August 2011 21:03, Tom Lane t...@sss.pgh.pa.us wrote: I mean that it's unclear what you'll get if status has a bitpattern equivalent to a negative integer. If the compiler implements the comparison as signed, the test will yield TRUE; if unsigned, it's FALSE. On compilers on which the enum value is represented as an unsigned int, passing -1 is just the same as doing that with any function with an unsigned int argument for that argument - it will convert to a large unsigned value . So sure, that isolated part of the test's outcome will vary depending on whether or not the compiler opts to represent the enum as signed when it can, but I don't look at it as you do. I simply consider that to be a violation of the enum's contract, or the caller's failure to consider that the enum couldn't represent -1 -- they got what they asked for. This argument is completely missing the point of the test, which is to verify whether or not the caller adhered to the enum's contract. You can *not* assume that he did while arguing about whether the test is valid or accomplishes its goals. I did not assume anything about the caller or their trustworthiness. The whole point of my argument is that passing a negative integer where the enum is represented as unsigned is just another way of violating the contract (passing a negative integer where the enum is represented as signed is another), that is equally well handled by the test; the whole test though, not the isolated part of it that you referred to. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
Can we please commit a fix for this problem? -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 3 August 2011 00:52, Peter Geoghegan pe...@2ndquadrant.com wrote: Now, the only warning that remains is that same Correction - there are actually 3 additional warnings like this in repl_gram.c: /home/peter/build/Release/bin/clang -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wformat-security -fno-strict-aliasing -fwrapv -I. -I../../../src/include -D_GNU_SOURCE -c -o repl_gram.o repl_gram.c repl_gram.y:106:30: warning: implicit conversion from enumeration type 'enum ReplNodeTag' to different enumeration type 'NodeTag' (aka 'enum NodeTag') [-Wconversion] (yyval.node) = (Node *) makeNode(IdentifySystemCmd); ^~~ ../../../src/include/nodes/nodes.h:475:64: note: expanded from: #define makeNode(_type_)((_type_ *) newNode(sizeof(_type_),T_##_type_)) ^ scratch space:180:1: note: expanded from: T_IdentifySystemCmd ^ ../../../src/include/nodes/nodes.h:452:19: note: expanded from: _result-type = (tag); \ ~ ^~~ I'll take a look into them later. This tautology warning sounds completely valid: /home/peter/build/Release/bin/clang -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wformat-security -fno-strict-aliasing -fwrapv -pthread -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -fpic -DFRONTEND -DUNSAFE_STAT_OK -I. -I../../../src/include -D_GNU_SOURCE -I../../../src/port -I../../../src/port -DSO_MAJOR_VERSION=5 -c -o fe-exec.o fe-exec.c fe-exec.c:2408:13: warning: comparison of unsigned enum expression 0 is always false [-Wtautological-compare] if (status 0 || status = sizeof pgresStatus / sizeof pgresStatus[0]) ~~ ^ ~ 1 warning generated. So, there are 4 remaining warnings. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
Attached patch removes the tautologolical part of an evaluated
expression, fixing the problem flagged by this quite valid warning.
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 605d242..51af0d8 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -2386,7 +2386,7 @@ PQresultStatus(const PGresult *res)
char *
PQresStatus(ExecStatusType status)
{
- if (status 0 || status = sizeof pgresStatus / sizeof pgresStatus[0])
+ if (status = sizeof pgresStatus / sizeof pgresStatus[0])
return libpq_gettext(invalid ExecStatusType code);
return pgresStatus[status];
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 03.08.2011 12:25, Peter Geoghegan wrote: Attached patch removes the tautologolical part of an evaluated expression, fixing the problem flagged by this quite valid warning. The check is only tautological if the compiler implements enums as unsigned integers. Whether enums are signed or not is implementation-dependent. Perhaps cast status to unsigned or signed explicitly before the checks? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 3 August 2011 10:34, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: The check is only tautological if the compiler implements enums as unsigned integers. Whether enums are signed or not is implementation-dependent. Perhaps cast status to unsigned or signed explicitly before the checks? I don't believe that the standard allows for an implementation of enums as unsigned integers - after all, individual enum literals can be given corresponding negative integer values. This warning is only seen because the first enum literal in the enum is explicitly given the value 0, thus precluding the possibility of the value being 0, barring some abuse of the enum. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 03.08.2011 13:05, Peter Geoghegan wrote: I don't believe that the standard allows for an implementation of enums as unsigned integers - after all, individual enum literals can be given corresponding negative integer values. C99 says: Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined,110) but shall be capable of representing the values of all the members of the enumeration. See also: http://stackoverflow.com/questions/2579230/signedness-of-enum-in-c-c99-c-cx-gnu-c-gnu-c99 -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 3 August 2011 11:05, Peter Geoghegan pe...@2ndquadrant.com wrote: I don't believe that the standard allows for an implementation of enums as unsigned integers - after all, individual enum literals can be given corresponding negative integer values. It actually gives leeway to implement the enum as unsigned int when the compiler knows that it won't matter, because there are no negative integer values that correspond to some enum literal. The hint was in my original warning. :-) This warning is only seen because the first enum literal in the enum is explicitly given the value 0, thus precluding the possibility of the value being 0, barring some abuse of the enum. It's also seen if no explicit values are given, and the compiler opts to make the representation unsigned. It is not seen if it the value is -1, for example. Despite the fact that whether or not the value is unsigned is implementation defined, I think that the patch is still valid - the expression is at least logically tautological, even if it isn't necessarily bitwise tautological, because, as I said, barring some violation of the enum's contract, it should not be 0. That's precisely why the compiler has opted to make it unsigned. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 03.08.2011 14:13, Peter Geoghegan wrote: On 3 August 2011 11:05, Peter Geogheganpe...@2ndquadrant.com wrote: I don't believe that the standard allows for an implementation of enums as unsigned integers - after all, individual enum literals can be given corresponding negative integer values. It actually gives leeway to implement the enum as unsigned int when the compiler knows that it won't matter, because there are no negative integer values that correspond to some enum literal. The hint was in my original warning. :-) This warning is only seen because the first enum literal in the enum is explicitly given the value 0, thus precluding the possibility of the value being 0, barring some abuse of the enum. It's also seen if no explicit values are given, and the compiler opts to make the representation unsigned. It is not seen if it the value is -1, for example. Despite the fact that whether or not the value is unsigned is implementation defined, I think that the patch is still valid - the expression is at least logically tautological, even if it isn't necessarily bitwise tautological, because, as I said, barring some violation of the enum's contract, it should not be 0. That's precisely why the compiler has opted to make it unsigned. Right, but the purpose of that check is to defend from programmer error. If the programmer screws up and calls PQresStatus(-1), we want to give an error, not crash. If you assume that the programmer will only pass a valid enum constant as parameter, then you might as well remove the if-statement altogether. I don't think that would be an improvement. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 3 August 2011 12:19, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
Right, but the purpose of that check is to defend from programmer error. If
the programmer screws up and calls PQresStatus(-1), we want to give an
error, not crash. If you assume that the programmer will only pass a valid
enum constant as parameter, then you might as well remove the if-statement
altogether. I don't think that would be an improvement.
Ahh. I failed to consider the intent of the code.
Attached patch has a better solution than casting though - we simply
use an enum literal. The fact that PGRES_EMPTY_QUERY is the first
value in the enum can be safely assumed to be stable, not least
because we've even already explicitly given it a corresponding value
of 0.
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 605d242..b6e6363 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -2386,7 +2386,7 @@ PQresultStatus(const PGresult *res)
char *
PQresStatus(ExecStatusType status)
{
- if (status 0 || status = sizeof pgresStatus / sizeof pgresStatus[0])
+ if (status PGRES_EMPTY_QUERY || status = sizeof pgresStatus / sizeof pgresStatus[0])
return libpq_gettext(invalid ExecStatusType code);
return pgresStatus[status];
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
It occurred to me that you may be a little surprised that the patch actually prevents the warning from occurring. If you have any doubts, I can assure you that it does. Clang differentiates between 0 as an rvalue, 0 from an enum literal and 0 resulting from evaluating a macro expression (including a function-like macro with deep nesting). -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
Peter Geoghegan pe...@2ndquadrant.com writes: On 3 August 2011 12:19, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Right, but the purpose of that check is to defend from programmer error. If the programmer screws up and calls PQresStatus(-1), we want to give an error, not crash. If you assume that the programmer will only pass a valid enum constant as parameter, then you might as well remove the if-statement altogether. I don't think that would be an improvement. Ahh. I failed to consider the intent of the code. Attached patch has a better solution than casting though - we simply use an enum literal. The fact that PGRES_EMPTY_QUERY is the first value in the enum can be safely assumed to be stable, not least because we've even already explicitly given it a corresponding value of 0. No, this is not an improvement at all. The point of the code is that we are about to use the enum value as an integer array subscript, and we want to make sure it is within the array bounds. Tying that logic to some member of the enum is not a readability or safety improvement. We aren't trusting the caller to pass a valid enum value, and likewise this code doesn't particularly want to trust the enum definition to be anything in particular. There is another point here, though, which is that if we're not sure whether the compiler considers ExecStatusType to be signed or unsigned, then we have no idea what the test status PGRES_EMPTY_QUERY even means. So I think the most reasonable fix is probably if ((unsigned int) status = sizeof pgresStatus / sizeof pgresStatus[0]) which is sufficient to cover both directions, since if status is passed as -1 then it will convert to a large unsigned value. It's also a natural expression of what we really want, ie, that the integer equivalent of the enum value is in range. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 3 August 2011 15:29, Tom Lane t...@sss.pgh.pa.us wrote: No, this is not an improvement at all. The point of the code is that we are about to use the enum value as an integer array subscript, and we want to make sure it is within the array bounds. Tying that logic to some member of the enum is not a readability or safety improvement. We aren't trusting the caller to pass a valid enum value, and likewise this code doesn't particularly want to trust the enum definition to be anything in particular. I would think that if someone were to have a reason to change the explicit value set for the identifier PGRES_EMPTY_QUERY, they would carefully consider the ramifications of doing so. It's far more likely they'd just append new values to the end of the enum though. This is why I don't consider that what I've proposed exposes us to any additional risk. There is another point here, though, which is that if we're not sure whether the compiler considers ExecStatusType to be signed or unsigned, then we have no idea what the test status PGRES_EMPTY_QUERY even means. I'm sorry, but I don't know what you mean by this. So I think the most reasonable fix is probably if ((unsigned int) status = sizeof pgresStatus / sizeof pgresStatus[0]) which is sufficient to cover both directions, since if status is passed as -1 then it will convert to a large unsigned value. It's also a natural expression of what we really want, ie, that the integer equivalent of the enum value is in range. I'm not convinced that that is an improvement to rely on the conversion doing so, but it's not as if I feel very strongly about it. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On Aug 3, 2011, at 1:47 AM, Peter Geoghegan wrote: So, there are 4 remaining warnings. I haven't been paying attention to warnings, but FWIW 9.1beta3 has been building with LLVM by default with Xcode 4. Both on Snow Leopard and on Lion I saw something like this: try=# select version(); version - PostgreSQL 9.1beta3 on x86_64-apple-darwin11.0.0, compiled by i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2335.15.00), 64-bit (1 row) Which is just awesome. Things seem to work great. Best, David
Re: [HACKERS] Further news on Clang - spurious warnings
On ons, 2011-08-03 at 10:25 +0100, Peter Geoghegan wrote:
Attached patch removes the tautologolical part of an evaluated
expression, fixing the problem flagged by this quite valid warning.
I think these warnings are completely bogus and should not be worked
around. Even in the most trivial case of
{
unsigned int foo;
...
if (foo 0 ...)
...
}
I would not want to remove the check, because as the code gets moved
around, refactored, reused, whatever, the unsigned int might change into
a signed int, and then you're hosed. It's fine for -Wextra, but not for
the -Wall level.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On ons, 2011-08-03 at 10:33 -0700, David E. Wheeler wrote: I haven't been paying attention to warnings, but FWIW 9.1beta3 has been building with LLVM by default with Xcode 4. Both on Snow Leopard and on Lion I saw something like this: try=# select version(); version - PostgreSQL 9.1beta3 on x86_64-apple-darwin11.0.0, compiled by i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2335.15.00), 64-bit (1 row) Which is just awesome. Things seem to work great. Note that what you are using there is a GCC frontend with a LLVM backend. (So I suppose you would get mostly GCC warnings.) Clang is a new/different compiler frontend using the LLVM backend. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On Aug 3, 2011, at 11:17 AM, Peter Eisentraut wrote: Note that what you are using there is a GCC frontend with a LLVM backend. (So I suppose you would get mostly GCC warnings.) Clang is a new/different compiler frontend using the LLVM backend. Yeah, not sure whether or not to tweak the Makefile to use Clang. I guess not? David
Re: [HACKERS] Further news on Clang - spurious warnings
On 3 Aug 2011, at 19:20, David E. Wheeler wrote: Yeah, not sure whether or not to tweak the Makefile to use Clang. I guess not? export CC=clang ./configure ... -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On Aug 3, 2011, at 11:28 AM, Grzegorz Jaskiewicz wrote: export CC=clang ./configure ... Thanks, I'll give that a try the next time I build (RC1 I guess). David
Re: [HACKERS] Further news on Clang - spurious warnings
Peter Geoghegan pe...@2ndquadrant.com writes: On 3 August 2011 15:29, Tom Lane t...@sss.pgh.pa.us wrote: There is another point here, though, which is that if we're not sure whether the compiler considers ExecStatusType to be signed or unsigned, then we have no idea what the test status PGRES_EMPTY_QUERY even means. I'm sorry, but I don't know what you mean by this. I mean that it's unclear what you'll get if status has a bitpattern equivalent to a negative integer. If the compiler implements the comparison as signed, the test will yield TRUE; if unsigned, it's FALSE. So I think the most reasonable fix is probably if ((unsigned int) status = sizeof pgresStatus / sizeof pgresStatus[0]) which is sufficient to cover both directions, since if status is passed as -1 then it will convert to a large unsigned value. It's also a natural expression of what we really want, ie, that the integer equivalent of the enum value is in range. I'm not convinced that that is an improvement to rely on the conversion doing so, but it's not as if I feel very strongly about it. The C standard specifies that signed-to-unsigned conversions must work like that; and even if the standard didn't, it would surely work like that on any machine with two's-complement representation, which is to say every computer built in the last forty years or so. So I don't find it a questionable assumption. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On Wed, Aug 03, 2011 at 04:03:39PM -0400, Tom Lane wrote: The C standard specifies that signed-to-unsigned conversions must work like that; and even if the standard didn't, it would surely work like that on any machine with two's-complement representation, which is to say every computer built in the last forty years or so. So I don't find it a questionable assumption. I had the pleasure of working on a Univac 1108 in about 1978 and it was very definitely ones complement. I'm somewhat amazed to find that the Univac 1100 series architecture and instruction set lives on to this day. The last pure 1100 seems to be the Unisys 2200/3800 released in 1997. Even later U1100/Exec 8 descendants appear to still exist and are still actively supported: http://en.wikipedia.org/wiki/Unisys_OS_2200_operating_system So there are still ones complement machines out there. However I suggest we pretend otherwise and continue to ignore them. -dg -- David Gould da...@sonic.net 510 536 1443510 282 0869 If simplicity worked, the world would be overrun with insects. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 3 August 2011 21:03, Tom Lane t...@sss.pgh.pa.us wrote: I mean that it's unclear what you'll get if status has a bitpattern equivalent to a negative integer. If the compiler implements the comparison as signed, the test will yield TRUE; if unsigned, it's FALSE. On compilers on which the enum value is represented as an unsigned int, passing -1 is just the same as doing that with any function with an unsigned int argument for that argument - it will convert to a large unsigned value . So sure, that isolated part of the test's outcome will vary depending on whether or not the compiler opts to represent the enum as signed when it can, but I don't look at it as you do. I simply consider that to be a violation of the enum's contract, or the caller's failure to consider that the enum couldn't represent -1 -- they got what they asked for. To put it another way, you could say that the first part of the test only exists for the benefit of compilers that treat all enums as signed. Clang recognised that redundancy for its unsigned case, and complained. It doesn't make sense to me to look at that one part of the test in isolation though. With my patch, the test as a whole does its job (in an identical fashion to before, in fact). Come to think of it, you could argue that the warning is invalid on the basis that the enum being unsigned is implementation defined and therefore the first part of the test is conceivably not redundant on some platforms, and perhaps I will on the Clang list. Probably not though, because it was hard enough with the warning bug that I managed to get them to fix, and I thought that was open-and-shut. I'm not convinced that that is an improvement to rely on the conversion doing so, but it's not as if I feel very strongly about it. The C standard specifies that signed-to-unsigned conversions must work like that; and even if the standard didn't, it would surely work like that on any machine with two's-complement representation, which is to say every computer built in the last forty years or so. So I don't find it a questionable assumption. I wasn't questioning whether it would work. I just think that relying on the behaviour of the conversion like that doesn't make your intent very clear. If I thought that it would or could be plain broken under certain circumstances, I would naturally feel strongly about it; as I've said, I don't. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On Wed, Aug 03, 2011 at 01:40:42PM +0300, Heikki Linnakangas wrote: On 03.08.2011 13:05, Peter Geoghegan wrote: I don't believe that the standard allows for an implementation of enums as unsigned integers - after all, individual enum literals can be given corresponding negative integer values. C99 says: Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined,110) but shall be capable of representing the values of all the members of the enumeration. Are we moving to C99? C89 says: Each enumerated type shall be compatible with an integer type; the choice of type is implementation-defined. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further news on Clang - spurious warnings
On 04.08.2011 04:21, David Fetter wrote: On Wed, Aug 03, 2011 at 01:40:42PM +0300, Heikki Linnakangas wrote: On 03.08.2011 13:05, Peter Geoghegan wrote: I don't believe that the standard allows for an implementation of enums as unsigned integers - after all, individual enum literals can be given corresponding negative integer values. C99 says: Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined,110) but shall be capable of representing the values of all the members of the enumeration. Are we moving to C99? C89 says: Each enumerated type shall be compatible with an integer type; the choice of type is implementation-defined. Well, that's the same thing, just in less explicit words. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Further news on Clang - spurious warnings
I'm happy to report that thanks to some persistent complaining on my part, the one outstanding issue when building Postgres with Clang - the spurious warnings that occured as a result of it being statically detected that there are assignments past what appears to be the end of a single element array at the end of a struct - has been fixed in a recent revision. Now, the only warning that remains is that same, annoying Flex related bug also seen with GCC that we can't seem to do anything about, because the Flex people refuse to acknowledge that it's a bug. However, at least when you see this warning when using Clang, you get to see a comment beside the declaration that hints that the warning is spurious: [peter@laptop postgresql]$ /home/peter/build/Release/bin/clang -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wformat-security -fno-strict-aliasing -fwrapv -Wno-error -I. -I. -Isrc/include -D_GNU_SOURCE -c -o gram.o src/backend/parser/gram.c In file included from gram.y:12949: scan.c:16246:23: warning: unused variable 'yyg' [-Wunused-variable] struct yyguts_t * yyg = (struct yyguts_t*)yyscanner; /* This var may be unused depending upon options. */ ^ 1 warning generated. This is not the case with GCC 4.6: [peter@laptop postgresql]$ gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wformat-security -fno-strict-aliasing -fwrapv -Wno-error -I. -I. -Isrc/include -D_GNU_SOURCE -c -o gram.o src/backend/parser/gram.c In file included from gram.y:12949:0: scan.c: In function ‘yy_try_NUL_trans’: scan.c:16246:23: warning: unused variable ‘yyg’ [-Wunused-variable] -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
