Bug 77422 - Fix type punning warning in HashTable.h debug builds
Summary: Fix type punning warning in HashTable.h debug builds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Wingo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-31 03:48 PST by Andy Wingo
Modified: 2012-02-02 11:13 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.58 KB, patch)
2012-01-31 03:50 PST, Andy Wingo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Wingo 2012-01-31 03:48:31 PST
The patch to be attached fixes warnings like this one:

../../Source/JavaScriptCore/API/JSObjectRef.cpp:548:1:   instantiated from here
../../Source/JavaScriptCore/wtf/HashTable.h:481:90: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

Why does it work, you ask?  That, dear sir or madam, I do not know.
Comment 1 Andy Wingo 2012-01-31 03:50:10 PST
Created attachment 124704 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-31 03:51:53 PST
Attachment 124704 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Andy Wingo 2012-01-31 04:01:35 PST
(In reply to comment #2)
> Falling back to patching base and 3-way merge...
> Auto-merging LayoutTests/ChangeLog

My patch does not touch this file.  What's up with that?

Andy
Comment 4 Andy Wingo 2012-01-31 04:04:22 PST
Adding Darin and Adam for review.
Comment 5 Darin Adler 2012-01-31 08:45:00 PST
(In reply to comment #0)
> Why does it work, you ask? That, dear sir or madam, I do not know.

Oof. That’s a problem. We should figure out why. If possible, this fix should be done in the reinterpret_cast_ptr template rather than at the call site.
Comment 6 Andy Wingo 2012-01-31 12:30:21 PST
(In reply to comment #5)
> (In reply to comment #0)
> > Why does it work, you ask? That, dear sir or madam, I do not know.
> 
> Oof. That’s a problem. We should figure out why. If possible, this fix should be done in the reinterpret_cast_ptr template rather than at the call site.

I did some investigations before making this patch. The reinterpret_cast_ptr looks correct to my knowledge.  The case that is taken for the aligned buffer is the __may_alias__ case.  This is x86-64, so reinterpret_cast_ptr is reinterpret_cast.

Every time I think I find a gcc bug I have been wrong.  Still, it sounds possible in this case.  This is gcc 4.6.2 from Debian unstable, with the default compilation options for the GTK+ port.

FWIW, I have seen this warning for months, but only got bothered enough to look at it today.
Comment 7 Andy Wingo 2012-02-01 08:52:00 PST
WDYT, Darin?  It is a bit inscrutable, but it is a correct change, and it makes --debug more useful for the gtk port.  As it is, this error typically occurs for every file that is compiled, and it is preceded by hundreds of lines of template gobbledygook each time.
Comment 8 Darin Adler 2012-02-01 10:33:01 PST
(In reply to comment #7)
> but it is a correct change

I’m not sure what you mean by “correct change”. It’s probably OK to change this so the compiler stops complaining, but there is no reason this change should have any effect.
Comment 9 Andy Wingo 2012-02-01 13:33:22 PST
(In reply to comment #8)
> (In reply to comment #7)
> > but it is a correct change
> 
> I’m not sure what you mean by “correct change”. It’s probably OK to change this so the compiler stops complaining, but there is no reason this change should have any effect.

I didn't express myself well. I meant exactly what you said here.
Comment 10 Darin Adler 2012-02-01 14:04:41 PST
Anders, Gavin, do you have any alternative to landing this, and/or do you think it’s OK to just do this to sidestep the warning Andy is running into?
Comment 11 Gavin Barraclough 2012-02-01 17:00:27 PST
I've got no better suggestions, this is fine with me unless Anders has any better ideas.
G.
Comment 12 Anders Carlsson 2012-02-01 17:51:50 PST
I don't have any better ideas, this looks good.
Comment 13 WebKit Review Bot 2012-02-02 11:13:03 PST
Comment on attachment 124704 [details]
Patch

Clearing flags on attachment: 124704

Committed r106574: <http://trac.webkit.org/changeset/106574>
Comment 14 WebKit Review Bot 2012-02-02 11:13:08 PST
All reviewed patches have been landed.  Closing bug.