Using DEFINE_string causes error in optimized compiles

[GoogleCodeExporter]

What steps will reproduce the problem?

Compile a C++ file that uses DEFINE_string with g++ 4.4.1. Use the -O2 flag 
while compiling.

What is the expected output? What do you see instead?

The expectation is that the file should compile without any warnings or 
errors. Instead, here is the error that g++ emits:

error: dereferencing type-punned pointer will break strict-aliasing rules

One can get rid of this error by using the -fno-strict-aliasing flag while 
compiling, but using this makes the optimization algorithms less effective.  
It'll be great if the next release of gflags could fix the DEFINE_string 
macro to be more compiler friendly instead.

What version of the product are you using? On what operating system?

gflags 1.2. Ubuntu 9.10.

Please provide any additional information below.

Original issue reported on code.google.com by mohit.a...@gmail.com on 8 Jan 2010 at 9:10

[GoogleCodeExporter]

I guess gcc 4.4 turned on strict aliasing by default?  Or maybe the detection 
just 
got better; I'm not seeing any problems with gcc 4.2, even with 
-fstrict-aliasing 
explicitly enabled.

In any cased, I thought strict aliasing problems were a warning, not an error.  
Are 
you using -Werror as well?

I don't have gcc 4.4.1 handy.  Can you try compiling, say, the unittests (via 
'make 
check'), and attaching the full output?  With line numbers, it should be easy 
to 
diagnose the problem and, hopefully, fix it.

Original comment by csilv...@gmail.com on 8 Jan 2010 at 3:57

[GoogleCodeExporter]

Yes, I'm using -Werror for my compiles - that's why I see the error. Otherwise, 
its a 
warning - which is still ugly.

Attached is a file that shows the complete output when a 'make' is done inside 
the 
gflags-1.2 source. You can see the relevant warnings when the unittests are 
being 
compiled. Here's an excerpt:

src/gflags_unittest.cc:84: warning: dereferencing type-punned pointer will 
break 
strict-aliasing rules
src/gflags_unittest.cc:85: warning: dereferencing type-punned pointer will 
break 
strict-aliasing rules
src/gflags_unittest.cc:94: warning: dereferencing type-punned pointer will 
break 
strict-aliasing rules
src/gflags_unittest.cc:104: warning: dereferencing type-punned pointer will 
break 
strict-aliasing rules
src/gflags_unittest.cc:105: warning: dereferencing type-punned pointer will 
break 
strict-aliasing rules
src/gflags_unittest.cc:106: warning: dereferencing type-punned pointer will 
break 
strict-aliasing rules
src/gflags_unittest.cc:109: warning: dereferencing type-punned pointer will 
break 
strict-aliasing rules
src/gflags_unittest.cc:123: warning: dereferencing type-punned pointer will 
break 
strict-aliasing rules
src/gflags_unittest.cc:134: warning: dereferencing type-punned pointer will 
break 
strict-aliasing rules

Original comment by mohit.a...@gmail.com on 8 Jan 2010 at 6:45

Attachments:

• out.txt

[GoogleCodeExporter]

Just a little more information to help out with fixing this bug. The line 
numbers 
reported by the compiler while emitting the warning correspond to the location 
where 
the DEFINE_string macro is used. It doesn't give the precise line within the 
macro's 
implementation that's causing the warning to be emitted. 

So what I did was replace one of the uses of the macro DEFINE_string with its 
implementation (after fixing the implementation appropriately to make it valid 
outside a macro definition). The compiler reported the warning this time at the 
following line within the macro's implementation:

   std::string& FLAGS_##name = *(reinterpret_cast<std::string*>(s_##name[0].s));

Original comment by mohit.a...@gmail.com on 8 Jan 2010 at 7:10

[GoogleCodeExporter]

I found that changing the offending line:
   std::string& FLAGS_##name = *(reinterpret_cast<std::string*>(s_##name[0].s));
to:
   std::string& FLAGS_##name = *const_cast<std::string*>(FLAGS_no##name);
fixes the issue. Given below is the complete macro implementation with the fix.

#define DEFINE_string(name, val, txt)                                     \
  namespace fLS {                                                         \
    static union { void* align; char s[sizeof(std::string)]; } s_##name[2]; \
    const std::string* const FLAGS_no##name = new (s_##name[0].s) std::string(val); \
    static ::google::FlagRegisterer o_##name(                \
      #name, "string", MAYBE_STRIPPED_HELP(txt), __FILE__,                \
      s_##name[0].s, new (s_##name[1].s) std::string(*FLAGS_no##name));   \
    extern std::string& FLAGS_##name;                                     \
    using fLS::FLAGS_##name;                                              \
    std::string& FLAGS_##name = *const_cast<std::string*>(FLAGS_no##name);   \
  }                                                                       \
  using fLS::FLAGS_##name

Original comment by mohit.a...@gmail.com on 8 Jan 2010 at 7:26

[GoogleCodeExporter]

Nice fix!  But I'm wondering now why we'd bother to have the const at all in 
this 
case.  What if you changed it to this:
---
#define DEFINE_string(name, val, txt)                                     \
  namespace fLS {                                                         \
    static union { void* align; char s[sizeof(std::string)]; } s_##name[2]; \
    std::string* const FLAGS_no##name = new (s_##name[0].s) std::string(val); \
    static ::google::FlagRegisterer o_##name(                \
      #name, "string", MAYBE_STRIPPED_HELP(txt), __FILE__,                \
      s_##name[0].s, new (s_##name[1].s) std::string(*FLAGS_no##name));   \
    extern std::string& FLAGS_##name;                                     \
    using fLS::FLAGS_##name;                                              \
    std::string& FLAGS_##name = *FLAGS_no##name;   \
  }                                                                       \
  using fLS::FLAGS_##name
---

Does that still work for you?  If so, I'll make it part of the next release.

Original comment by csilv...@gmail.com on 8 Jan 2010 at 7:51

• Changed state: Accepted

[GoogleCodeExporter]

> Does that still work for you?  If so, I'll make it part of the next release.

That was actually my first fix and that does work too. The only reason I even 
suggested 
the other fix was because I wanted to keep the suggested changes to a minimum. 
:)

Original comment by mohit.a...@gmail.com on 8 Jan 2010 at 7:54

[GoogleCodeExporter]

Original comment by csilv...@gmail.com on 30 Apr 2010 at 11:28

• Changed state: Started

[GoogleCodeExporter]

This should be fixed in gflags 1.4, just released.

Original comment by csilv...@gmail.com on 14 Oct 2010 at 1:21

• Changed state: Fixed