Re: tools/widl/typegen.c pointer initialization
On Mon, 29 Oct 2007, Dan Hipschman wrote: Compiling with -O2 -W -Wall using either gcc 4.0 or 3.4 I don't get any warnings. Even adding -Wuninitialized doesn't do anything: I am mostly using GCC 3.4 with -O2 -Wall as you did, and occasionally a snapshot of GCC 4.3, with and without -Wextra in addition. It took me a bit to find the combination that triggered it, and it is GCC 4.3.0 20071029 with -O2 -Wall -Wextra that issues typegen.c: In function 'get_required_buffer_size_type': typegen.c:2288: warning: 'uname' may be used uninitialized in this function typegen.c: In function 'write_user_tfs': typegen.c:953: warning: 'name' may be used uninitialized in this function Gerald
Re: tools/widl/typegen.c pointer initialization
On Sun, 28 Oct 2007, Dan Hipschman wrote: The logic is as follows: Thanks for the explanation, Dan! Better than this would be to put assert(is_user_type(type)); above the initializations to convince the programmer at least that name will get initialized correctly in get_user_type. If that doesn't convince the compiler then I don't think it's worth it to try to placate it since we're supposed to be wrting code for humans, not computers. I think the question is whether a compiler can reasonably be expected to deduce that the source is fine. If that deduction involves solving the halting problem (or similar) hacking the source to avoid the warning actually doesn't occur to be that bad. ;-) After the work I did again in the last weeks Wine now builds more or less without warnings for most versions of GCC (and our default set of options). If things continue to proceed well, this may be the only warning left in the whole codebase in a week or two. I think the real problem is that the code is just not clear enough. I've been meaning to add asserts. Where asserts are impractical: comments. Does this mean you are going to submit some patches to address this? Gerald
Re: tools/widl/typegen.c pointer initialization
On Sun, Oct 28, 2007 at 07:07:15PM -0700, Dan Kegel wrote: To get uninitialized warnings, you have to also specify optimization (-O2). Without -O, gcc doesn't do the analysis that can detect uninitialized variables. Compiling with -O2 -W -Wall using either gcc 4.0 or 3.4 I don't get any warnings. Even adding -Wuninitialized doesn't do anything: $ make clean make -j1 CFLAGS=-O2 -W -Wall -Wuninitialized 21 | fgrep warning widl.c:189: warning: unused parameter 'sig' parser.yy.c:1276: warning: comparison between signed and unsigned
Re: tools/widl/typegen.c pointer initialization
On Sun, Oct 28, 2007 at 01:51:34PM +0100, Gerald Pfeifer wrote:
In tools/widl/typegen.c we have the following snippet
static void write_user_tfs(FILE *file, type_t *type, unsigned int *tafsoff)
{
unsigned int start, absoff, flags;
unsigned int align = 0, ualign = 0;
const char *name;
type_t *utype = get_user_type(type, name);
which passes name to get_user_type without initializing it. get_user_type
in return has the following piece of code:
static type_t *get_user_type(const type_t *t, const char **pname)
{
for (;;)
{
type_t *ut = get_attrp(t-attrs, ATTR_WIREMARSHAL);
if (ut)
{
if (pname)
*pname = t-name;
return ut;
}
It is unclear to this reader (and various versions of GCC I tested),
whether we don't really use *name (*pname) uninitialized here. Now
it is possible that through some of the, hmm, tricky logic in these
two functions combined with some environmental guarantees it happens
that we don't, but it seems better to be proactive here.
The logic is as follows: We wouldn't be in write_user_tfs if the
predicate is_user_type wasn't true for that type, which means *pname
(name) will always be initialized since it gets set precisely when
get_user_type returns non-NULL (which is the is_user_type test). I
don't think initializing it to NULL is helpful since it would crash just
the same as if it weren't initialized. name *must* have a valid
(non-NULL) value. This may silence a warning, but it's making the logic
of the code more confusing by setting the variable to a value that
doesn't make any sense in that context. get_user_type is the
initialization.
Better than this would be to put assert(is_user_type(type)); above the
initializations to convince the programmer at least that name will get
initialized correctly in get_user_type. If that doesn't convince the
compiler then I don't think it's worth it to try to placate it since
we're supposed to be wrting code for humans, not computers. Some
projects use a macro like name IF_LINT(= NULL); to quiet warnings
while informing the reader that the initialization is not necessary. I
suppose that's slightly better as well.
I think the real problem is that the code is just not clear enough.
I've been meaning to add asserts. Where asserts are impractical:
comments.
Re: tools/widl/typegen.c pointer initialization
On Mon, Oct 29, 2007 at 01:26:45AM +0100, Gerald Pfeifer wrote: I think the question is whether a compiler can reasonably be expected to deduce that the source is fine. If that deduction involves solving the halting problem (or similar) hacking the source to avoid the warning actually doesn't occur to be that bad. ;-) Nope, you can't depend on the compiler to verify your code is correct. That's why you should try to write it in such a way that people can understand it. Hacking the code to make the compiler happy at the cost of making the code less clear is not a good idea. I think the real problem is that the code is just not clear enough. I've been meaning to add asserts. Where asserts are impractical: comments. Does this mean you are going to submit some patches to address this? If you tell me what options you build with and I can reproduce the warning then I'll be more than happy to try to fix it. I build widl with -W -Wall and get two warnings. One of which is in the bison-generated code; the other I sent a patch to silence and Alexandre rejected it.
re: tools/widl/typegen.c pointer initialization
Dan H. wrote: If you tell me what options you build with and I can reproduce the warning then I'll be more than happy to try to fix it. I build widl with -W -Wall and get two warnings. To get uninitialized warnings, you have to also specify optimization (-O2). Without -O, gcc doesn't do the analysis that can detect uninitialized variables. - Dan
