when building with GCC >=4.0, should add -Wno-invalid-offsetof to C++ options

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: zwol, Assigned: bsmedberg)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
GCC >=4.0 on the Mozilla code base spews thousands of warnings about applying offsetof() to non-POD types.  On some files there are so many of these warnings that you lose actual error messages past the top of your scrollback buffer.  Perhaps the code should be changed someday, but most of the cases I've actually looked at would be very hard to change and are unlikely to cause actual trouble.  Thus, it would be nice if we could get -Wno-invalid-offsetof added to the C++ command line flags when using these versions of GCC.
(Assignee)

Comment 1

9 years ago
Created attachment 334283 [details] [diff] [review]
supress offsetof warnings, rev. 1

Yeah, I think we should do this.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #334283 - Flags: review?(dbaron)
Comment on attachment 334283 [details] [diff] [review]
supress offsetof warnings, rev. 1

r=dbaron, although I think it would be better to put the whole thing inside a test for GNU_CC.  Some compilers might handle invalid options with something other than an error.

(I'm not sure what _COMPILER_PREFIX is for, though.)

And perhaps this should live somewhere closer to the -Wno-long-long tests?  Or do we just not worry about the order of things inside configure?
Attachment #334283 - Flags: review?(dbaron) → review+
(Assignee)

Comment 3

9 years ago
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/41d9d32ab5a7

I moved the check into a GNU_CXX block... I didn't move it down with long-long because that was part of a block of code that reads configure arguments and this is not pref-controlled.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 4

9 years ago
(In reply to comment #0)
> most of the cases I've actually looked at would be very hard to change
JFTR, try
template<class T> struct offsetof_helper { static T instance; };
#define offsetof(T, M) (\
  reinterpret_cast<char *>(&(offsetof_helper<T>::instance.M) - \
  reinterpret_cast<char *>(&(offsetof_helper<T>::instance))
Unlike casting a random address to a pointer type the compiler really does know exactly which type you're using so by my wooly thinking it should be safe for virtally (!) any class you throw at it.
(Reporter)

Comment 5

9 years ago
... wow, that actually works (with g++ 4.3.1); I was sure it was going to generate external references to the never-actually-defined object offsetof_helper<X>::instance!  (You need two close parens right after "instance.M" and three at the very end, by the way.)

Before doing any such thing I would want to be very sure that it worked correctly with all relevant compilers (in particular I understand MSVC is still not great with templates) and I'd want an opinion from a C++ expert that it wasn't going to blow up in our faces.
(Assignee)

Comment 6

9 years ago
I don't think doing that is necessary or desirable, because compilers may emit symbols to the nonexistent object. If we really wanted to avoid offsetof, couldn't we use something like this:

#define safe_offsetof(T, M) \
  (reinterpret_cast<char*>(&(((T*)0)->M)) -
   reinterpret_cast<char*>(0))
(Reporter)

Comment 7

9 years ago
> &(((T*)0)->M)

That's the construct that triggers the obnoxious warnings from recent g++;  we're already using that or something very like it.  (The precise text of the warning is 

warning: invalid access to non-static data member ‘B::aye’ of NULL object
warning: (perhaps the ‘offsetof’ macro was used incorrectly)

)
(Assignee)

Comment 8

9 years ago
Oh, well then use the following construct, which I've used to good effect in nsISupportsImpl.h:

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#434

#define safe_offsetof(T, M) \
  (reinterpret_cast<char*>(&(((T*)1000)->M)) -
   reinterpret_cast<char*>(1000))
(Reporter)

Comment 9

9 years ago
... g++ doesn't complain about that, but I think it probably *should*.
(In reply to comment #5)
> (You need two close parens right after
> "instance.M" and three at the very end, by the way.)
Sorry, my fault for not cut-n-pasting. (I actually had fewer open parens.)

Comment 11

9 years ago
The patch as checked in (comment 3) seems wrong. Autoconf generates
   ac_compile='${CC-cc} -c $CFLAGS $CPPFLAGS conftest.$ac_ext 1>&5'
   ac_link='${CC-cc} -o conftest${ac_exeext} $CFLAGS $CPPFLAGS $LDFLAGS
            conftest.$ac_ext $LIBS 1>&5'
(without line breaks) so the flags that is temporarily added to CXXFLAGS is never actually tested for. I don't find a copy of the autoconf manual for version 2.13 any more, but I think it would be better to use either CFLAGS (as in the reviewed version of the patch) or CPPFLAGS.

Hmm, or did you mean to use AC_LANG_CPLUSPLUS but forgot to add it?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

9 years ago
Yeah, forgot the AC_LANG_CPLUSPLUS at some point... that is now fixed: http://hg.mozilla.org/mozilla-central/index.cgi/rev/ccf420d6618b
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Blocks: 452504
Duplicate of this bug: 343456
You need to log in before you can comment on or make changes to this bug.