Bug 37877 - Autogenerate yarr character tables
Summary: Autogenerate yarr character tables
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-20 11:38 PDT by Oliver Hunt
Modified: 2010-04-20 16:02 PDT (History)
8 users (show)

See Also:


Attachments
Patch (22.21 KB, patch)
2010-04-20 11:42 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (22.88 KB, patch)
2010-04-20 12:10 PDT, Oliver Hunt
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2010-04-20 11:38:31 PDT
Autogenerate yarr character tables
Comment 1 Oliver Hunt 2010-04-20 11:42:50 PDT
Created attachment 53858 [details]
Patch
Comment 2 WebKit Review Bot 2010-04-20 11:48:10 PDT
Attachment 53858 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/assembler/X86Assembler.h:780:  cmpb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/X86Assembler.h:911:  testb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Early Warning System Bot 2010-04-20 11:52:34 PDT
Attachment 53858 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1821004
Comment 4 Oliver Hunt 2010-04-20 12:10:33 PDT
Created attachment 53862 [details]
Patch
Comment 5 WebKit Review Bot 2010-04-20 12:16:11 PDT
Attachment 53862 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/assembler/X86Assembler.h:780:  cmpb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/X86Assembler.h:911:  testb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Gavin Barraclough 2010-04-20 12:47:00 PDT
Comment on attachment 53862 [details]
Patch

Please comment the CharacterClass implementation to explain that all classes have ranges; the table is optional.
Ideally I guess we should land the assembler changes first & get the arm port before relying on them – but you should at least give the arm jit devs a heads up.
Comment 7 Zoltan Herczeg 2010-04-20 13:45:39 PDT
Thank you Gavin letting us know! You are really kind! As far as I see the normal Address and ExtendedAddress will be the same on ARM since both intptr_t and int are four bytes long. Hence we can simply reuse the 32 bit versions.
Comment 8 Oliver Hunt 2010-04-20 14:36:12 PDT
(In reply to comment #7)
> Thank you Gavin letting us know! You are really kind! As far as I see the
> normal Address and ExtendedAddress will be the same on ARM since both intptr_t
> and int are four bytes long. Hence we can simply reuse the 32 bit versions.

Can you come on irc? we do need additional instructions -- cmpb and testb for BaseIndex addresses
Comment 9 Peter Varga 2010-04-20 14:40:45 PDT
I noticed that you didn't updated Yarr Interpreter for this feature.
Thus the JavaScriptCore can't be compiled with Yarr Interpreter, because
the linker can't link the functions of builtin CharacterClasses.

./libjscore.a(RegexInterpreter.o): In function `JSC::Yarr::ByteCompiler::compile()':
RegexInterpreter.cpp:(.text._ZN3JSC4Yarr12ByteCompiler7compileEv[JSC::Yarr::ByteCompiler::compile()]+0x5e8): undefined reference to `JSC::Yarr::newlineCreate()'
RegexInterpreter.cpp:(.text._ZN3JSC4Yarr12ByteCompiler7compileEv[JSC::Yarr::ByteCompiler::compile()]+0x633): undefined reference to `JSC::Yarr::wordcharCreate()'
...
Comment 10 Oliver Hunt 2010-04-20 14:51:57 PDT
(In reply to comment #9)
> I noticed that you didn't updated Yarr Interpreter for this feature.
> Thus the JavaScriptCore can't be compiled with Yarr Interpreter, because
> the linker can't link the functions of builtin CharacterClasses.
> 
> ./libjscore.a(RegexInterpreter.o): In function
> `JSC::Yarr::ByteCompiler::compile()':
> RegexInterpreter.cpp:(.text._ZN3JSC4Yarr12ByteCompiler7compileEv[JSC::Yarr::ByteCompiler::compile()]+0x5e8):
> undefined reference to `JSC::Yarr::newlineCreate()'
> RegexInterpreter.cpp:(.text._ZN3JSC4Yarr12ByteCompiler7compileEv[JSC::Yarr::ByteCompiler::compile()]+0x633):
> undefined reference to `JSC::Yarr::wordcharCreate()'
> ...

Fixed locally, but still not sure about what to do about the ARM backend as i'm not sure how i can avoid build breakage without someone else doing the code :-(
Comment 11 Csaba Osztrogonác 2010-04-20 15:04:38 PDT
(In reply to comment #10)
> Fixed locally, but still not sure about what to do about the ARM backend as i'm
> not sure how i can avoid build breakage without someone else doing the code :-(

I think Gábor Zoltán or Gábor will submit the ARM 
patch asap, they will start their day at 11pm in PST.
Comment 12 Oliver Hunt 2010-04-20 15:05:58 PDT
Committed r57925: <http://trac.webkit.org/changeset/57925>
Comment 13 Oliver Hunt 2010-04-20 15:06:58 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > Fixed locally, but still not sure about what to do about the ARM backend as i'm
> > not sure how i can avoid build breakage without someone else doing the code :-(
> 
> I think Gábor Zoltán or Gábor will submit the ARM 
> patch asap, they will start their day at 11pm in PST.

Gabor tells me everything builds on arm fine so have landed.  *crosses fingers*
Comment 14 WebKit Review Bot 2010-04-20 16:02:34 PDT
http://trac.webkit.org/changeset/57925 might have broken Leopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/57924
http://trac.webkit.org/changeset/57925