Bug 59927 - [SH4] AssemblerLabel does not name a type
Summary: [SH4] AssemblerLabel does not name a type
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-02 04:52 PDT by thouraya
Modified: 2011-05-26 14:59 PDT (History)
8 users (show)

See Also:


Attachments
fix issue (1.48 KB, patch)
2011-05-02 05:16 PDT, thouraya
no flags Details | Formatted Diff | Diff
fix issue (1.40 KB, patch)
2011-05-23 08:05 PDT, thouraya
oliver: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description thouraya 2011-05-02 04:52:43 PDT
Hello,

Updating webkit, I have got the following error : error: AssemblerLabel does not name a type.

The issue is caused by the work done to Use AssemblerLabel throughout Assembler classes.

To fix the problem, I have to change the define order inf MacroAssemblerSH4.h as follow

#include "SH4Assembler.h"
#include "AbstractMacroAssembler.h"

But I'll get a webkit style problem :Alphabetical sorting problem.  [build/include_order] [4]


I have another solution which is to add:

# inculde AbstractMacroAssembler.h in AbstractMacroAssembler.h

What do you suggest as a solution? I'm preparing  a patch.

I think there is the same issue in MIPS platforms.

Regards,
Thouraya
Comment 1 thouraya 2011-05-02 05:16:24 PDT
Created attachment 91912 [details]
fix issue
Comment 2 WebKit Review Bot 2011-05-02 05:17:30 PDT
Attachment 91912 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 thouraya 2011-05-03 09:04:04 PDT
Hello,

Any update ?

Regards.
Comment 4 Eric Seidel 2011-05-11 20:32:50 PDT
There is a similar fix to this posted for MIPS somewher ein bugzilla.  Why do these headers have an ordering dependency?  That seems like the real bug!
Comment 5 thouraya 2011-05-12 00:47:46 PDT
(In reply to comment #4)
> There is a similar fix to this posted for MIPS somewher ein bugzilla.  Why do these headers have an ordering dependency?  That seems like the real bug!


Hello,

because AbstractMacroAssembler class needs AssemblerLabel struct which is defined in AssemblerBuffer.h file.

Or AssemblerBuffer.h is included in "SH4Assembler.h" and not in AbstractMacroAssembler.h.


Regards,
Thouraya.
Comment 6 Gavin Barraclough 2011-05-16 23:10:08 PDT
Comment on attachment 91912 [details]
fix issue

sorry! - tried to keep everything working :-)
Comment 7 thouraya 2011-05-23 06:40:50 PDT
Hello

The patch was reviewed more than one week ago but it's not yet added to commit queue?


Rgards,
Thouraya.
Comment 8 Eric Seidel 2011-05-23 07:31:31 PDT
Comment on attachment 91912 [details]
fix issue

You may want to set cq? To convey your desire.  Why do we need to missort the headers like this? Can't We include the needed header in the other header?
Comment 9 thouraya 2011-05-23 08:05:13 PDT
Created attachment 94428 [details]
fix issue
Comment 10 Oliver Hunt 2011-05-23 09:09:53 PDT
Comment on attachment 94428 [details]
fix issue

Why do you need this change?  no other platforms require AssemblerBuffer to be included in this way.
Comment 11 Oliver Hunt 2011-05-23 09:11:53 PDT
(In reply to comment #10)
> (From update of attachment 94428 [details])
> Why do you need this change?  no other platforms require AssemblerBuffer to be included in this way.

Oh i see, eric is making a bad suggestion for the world of jsc.
Comment 12 thouraya 2011-05-23 09:13:40 PDT
Hello,


(In reply to comment #10)
> (From update of attachment 94428 [details])
> Why do you need this change?  no other platforms require AssemblerBuffer to be included in this way.

MIPS requires changes: they suggested the same solution which is to swap the 2 include files and it was accepted in Bug 60283



Regards,
Thouraya.
Comment 13 Oliver Hunt 2011-05-23 09:14:09 PDT
Comment on attachment 91912 [details]
fix issue

Just set cq? on patches f you are unable to commit yourself.   Otherwise reviewers have no indication as to whether they should be using the commit bot or not.
Comment 14 WebKit Commit Bot 2011-05-23 09:40:20 PDT
Comment on attachment 91912 [details]
fix issue

Clearing flags on attachment: 91912

Committed r87076: <http://trac.webkit.org/changeset/87076>
Comment 15 WebKit Commit Bot 2011-05-23 09:40:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Darin Adler 2011-05-23 09:41:30 PDT
This is not good. In our project we strive to not create includes that have to be included in a certain order. Each header file is supposed to include everything it depends on. Check-ins like this one go against that basic WebKit design principle.
Comment 17 Ademar Reis 2011-05-26 07:21:46 PDT
(In reply to comment #16)
> This is not good. In our project we strive to not create includes that have to be included in a certain order. Each header file is supposed to include everything it depends on. Check-ins like this one go against that basic WebKit design principle.

Any plans on fixing it any other way? (I don't have access to a SH4 environment to test)
Comment 18 Darin Adler 2011-05-26 12:54:46 PDT
This patch was already landed, so it should be fixed.

I do think someone should fix it another way.
Comment 19 Eric Seidel 2011-05-26 14:59:49 PDT
There were a flurry of these patches for all the various ports.  This was a recent regression caused by some other Assembler refactor.  I never tracked down the change, but I would suspect that Gavin or Oliver or Geoff would know what change it might be.