Bug 26756

Summary: Work around incorrect warning issued by gcc 4.3 and 4.4
Product: WebKit Reporter: Thiago Macieira <thiago.macieira>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Minor CC: darin, dmaclach, laszlo.gombos
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch silencing the warning
none
Fixes for the warnings generated on gcc 4.3 and above darin: review-

Description Thiago Macieira 2009-06-26 08:57:37 PDT
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
Comment 1 Thiago Macieira 2009-06-26 08:58:52 PDT
Created attachment 31935 [details]
Patch silencing the warning
Comment 2 Dave MacLachlan 2009-10-26 16:38:56 PDT
Created attachment 41914 [details]
Fixes for the warnings generated on gcc 4.3 and above
Comment 3 Dave MacLachlan 2009-10-26 16:49:42 PDT
I'd be happy to have either patch taken, whichever is considered better. I'd just like to get rid of the warnings.
Comment 4 Darin Adler 2009-10-26 17:38:30 PDT
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.
Comment 5 Darin Adler 2009-10-26 17:40:14 PDT
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.
Comment 6 Thiago Macieira 2009-10-27 02:26:50 PDT
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?
Comment 7 Darin Adler 2009-10-27 10:16:22 PDT
(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+.
Comment 8 Dave MacLachlan 2009-10-27 10:34:24 PDT
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.
Comment 9 Thiago Macieira 2009-10-27 10:35:47 PDT
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.
Comment 10 Darin Adler 2009-10-28 18:03:06 PDT
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 ***
Comment 11 Eric Seidel (no email) 2009-10-28 18:05:36 PDT
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.