-Wabi warnings with gcc-3.3 ABI-compliant

RESOLVED WORKSFORME

Status

defect
--
major
RESOLVED WORKSFORME
17 years ago
2 years ago

People

(Reporter: bbaetz, Unassigned)

Tracking

(Blocks 1 bug, {topembed+})

Trunk
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: CLOSEME? Comment #7, )

brad got upgraded to gcc-3.3, and is now spewing 20000 or so warnings due to
-Wabi. Note that the ABI has not changed; this is just a warning that when it
changes in the future, this class' layout will change. This was done for GCC
PR8024 (http://gcc.gnu.org/cgi-bin/gnatsweb.pl)

The cases its complaining about here is when a class doesn't have an explicit
virtual destructor; only an implicit one which needs to actually do something
(ie call a destructor in a parent class). The ABI says that the implicit
destructor should be an the end of the vtable, but gcc puts it at the start in
the current ABI implementation

After some testing, this appears to only a real issue if a class has an implicit
destructor and:

a) Uses 'regular' inheritance (single or multiple) where the first class in the
list does not have an explicit destructor and any of the other ancestors do AND
the class has no virtual methods which are not overloading stuff in the first
parent class; or 
b) Uses virtual inheritance; or
c) Inherits from a class where any of these items occurs

There may be other cases where this occurs.

Since moz doesn't use virtual inheritance, or use MI much, these are mostly
bogus, I think. However, our string classes do trigger them in a few intances
(making up most of the 20000 warnings, given how many times we include those
header files), and those are the ones with the concrete impls frozen, I believe.

You can play arround with this by usng -fdump-class-hierarchy, which will create
a foo.cpp.class file with the layout for all the classes in foo.cpp.

The fix is to add

#if __GNUC__ > 3 || \
    (__GNUC__ == 3 && __GNUC_MINOR__ >= 2)
virtual ~foo() {};
#endif

to the top of any class where this warning triggers. We ned the #if so that we
don't change the ABI where not required (eg on windows), and for simplicitly
this should be a macro which people can use instead of the #if stuff, defined to
empty on nongcc=>3.2 platforms. This will not affect the 3.2 ABI, since it
already places the destructor there, but it will affect the 'next' ABI
(-fabi-version=2), since the destructor is then given an explicit position.

However, there are some bogus warnings. Specifically, g++ gives a warning even
w/o the last AND in (a) but I don't think that that is valid, since the vtable
entries then go into the parent class, leaving the current class with no extra
table entries, and so adding it to the start is the same as ading it to the end
of the list.

Thats an impl detail which gcc probably figures isn't worth determining for this
warning, although it would be worth filing a gcc bug, I guess. Thats a very rare
case, although some of our string clases may hit it.

The additional destructor shouldn't be a perf issue, since there would have been
a destructor anyway - we're just making its location explicit.

Since there are 20000 odd warnings, its really hard to see real ones, so we
should do this now rather than wait until the abi version does become the
default. Any patch should be tested by comparing every single .o and .so in the
build and confirming that there are no changes in 3.2, or 3.3 with the default ABI.
Warnings is over 40k now adding ABI-compliant to summary as I almost logged a
dupe and will hopefully stop others.
Summary: -Wabi warnings with gcc-3.3 → -Wabi warnings with gcc-3.3 ABI-compliant
> We need the #if so that we don't change the ABI where not required (eg on
> windows).

Is this really true? I don't think that doing compiler specific warning fixes is
the natural way to go. Either it's a relevant warning which should be addressed 
or it's bogus stuff and should be disabled if it hinders daily development.
Its a relvent warning, but only on gcc >=3.2 builds. We 'fix' it by changing the
ABI of the class in a way where the result is the same under gcc>=3.2, and is
still the same under the new ABI. Call it working arround a compiler bug.
Blocks: buildwarning
Mass reassign of Build/Config bugs to Leaf.
Assignee: mozbugs-build → leaf
ping? This affects the nsAString ABI, unless we checked in a fix that I missed.
I think we should at least add the necessary #ifdef for that class on the 1.7
branch, because presumably that will be being built with gcc3.4 who will wonder
why the string classes don't work right.
Flags: blocking1.7?
Keywords: topembed+
nsAString (see nsTObsoleteAString.h) has an explicit destructor, no?  so, this
is a non-issue for nsAString, right?
oh, was this bug only about our concrete classes? It should be a non-issue now,
right?
Flags: blocking1.7? → blocking1.7-
Assignee: leaf → cmp
Product: Browser → Seamonkey
Mass reassign of open bugs for chase@mozilla.org to build@mozilla-org.bugs.
Assignee: chase → build
Mass re-assign of bugs that aren't on the build team radar, so bugs assigned to build@mozilla-org.bugs reflects reality.

If there is a bug you really think we need to be looking at, please *email* build@mozilla.org with a bug number and explanation.
Assignee: build → nobody
Product: SeaMonkey → Core
QA Contact: granrosebugs → build-config
Whiteboard: CLOSEME? Comment #7
The relevant string code doesn't use vtables any more, and this is not part of the public API.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.