Bug 26756 – Work around incorrect warning issued by gcc 4.3 and 4.4

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebCore Misc.

[Thiago Macieira]

gcc 4.3 and 4.4 keep giving us a warning like:
 'function()::type' declared with greater visibility than the type of
 its field 'function()::type::subtype'

This seems to be a gcc bug. See the bug report for more information:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40145

However, in the mean time, the patch (to be attached) silences the warning with no loss of functionality

[Thiago Macieira]

Created attachment 31935 [details]
Patch silencing the warning

[Dave MacLachlan]

Created attachment 41914 [details]
Fixes for the warnings generated on gcc 4.3 and above

[Dave MacLachlan]

I'd be happy to have either patch taken, whichever is considered better. I'd just like to get rid of the warnings.

[Darin Adler]

Comment on attachment 41914 [details]
Fixes for the warnings generated on gcc 4.3 and above

Fix looks OK to me.

> +        Test: none

No need to have this in ChangeLog.

>  #if COMPILER(MSVC)
>  #define SKIP_STATIC_CONSTRUCTORS_ON_MSVC 1
> +// Due to issue in gcc 4.3/4.4
> +// http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40145
> +// We get warnings about some structs defined in functions based on the
> +// visibility of their member types.
> +// Using FUNCTION_LEVEL_STRUCT_VISIBILITY cleans up the warnings.
> +#define FUNCTION_LEVEL_STRUCT_VISIBILITY
>  #else
>  #define SKIP_STATIC_CONSTRUCTORS_ON_GCC 1
> +#define FUNCTION_LEVEL_STRUCT_VISIBILITY __attribute__((visibility ("hidden")))
>  #endif

This sort of thing probably does not belong in config.h.

A great place for it would be a new file named JavaScriptCore/wtf/CompilerWorkarounds.h. Or I think you could put it with WARN_UNUSED_RETURN in JavaScriptCore/wtf/Platform.h or somewhere else in the WTF code. It would also probably be fine to have CompilerWorkarounds.h be in WebCore/platform, I guess.

It would be clearer to state that we are using this to work around a compiler bug. You call it "issue in", which in some circles might mean this is a workaround for a problem, but that wasn't clear to me.

review- because config.h is the wrong place for this but otherwise the patch looks fine.

[Darin Adler]

I guess sidestepping the compiler bug by moving the structs out of the functions would be cleaner than adding an entire header file for this, so we should probably go with that although we need a patch with a change log. But I'm annoyed that a bug in a new version of gcc is forcing us to change our code like this.

[Thiago Macieira]

The bug has been reported to the GCC team. However, that doesn't change already-released versions that do produce this warning.

It's a harmless warning, though. The structs in question have no constructors, so they generate no external symbols.

Is a patch with a proper changelog all that is missing?

[Darin Adler]

(In reply to comment #6)
> The bug has been reported to the GCC team.

Yes, Dave included the URL of the bug report in his patch, which was good!

> However, that doesn't change already-released versions that do produce this warning.

Sure. I am still annoyed that we have to do this. We don't have any buildbots building with this version of gcc so we’re likely to introduce more cases like this.

> It's a harmless warning, though.

Can the build scripts be changed to ignore this harmless warning so we can avoid changing the source code?

> Is a patch with a proper changelog all that is missing?

Roughly speaking, yes.

I think your patch is probably a better approach than Dave’s, although I would have preferred blank lines after the struct definitions, and all it lacked was a change log. So if you posted it with a change log I’d probably say review+.

[Dave MacLachlan]

Thiago,

I'm happy with either patch. If Darin prefers yours, please go forward with it. The warning is showing up in Chromium builds and we'd like to get rid of it.

[Thiago Macieira]

We do have Qt building with WebKit using gcc 4.3 and 4.4. So even though this isn't the WebKit buildbot, we do get the warning report internally. If this appears again, we can send new patches.

So I'll update the patch and add a suitable ChangeLog.

[Darin Adler]

Looks like some Chrome folks fixed this in bug 30716 along with other compiler-related changes.

*** This bug has been marked as a duplicate of bug 30716 ***

[Eric Seidel (no email)]

Comment on attachment 41914 [details]
Fixes for the warnings generated on gcc 4.3 and above

Thank you very much for reporting this to GCC.