Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:43827
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 208.43.138.18 as permitted sender)
Message-ID: <49FE9AE7.4000008@php.net>
Date: Mon, 04 May 2009 00:36:07 -0700
Reply-To: shire@php.net
User-Agent: Postbox 1.0b11 (Macintosh/2009041623)
MIME-Version: 1.0
To: Matt Wilmas <php_lists@realplain.com>
CC: internals@lists.php.net, Nuno Lopes <nlopess@php.net>, 
 Lukas Kahwe Smith <mls@pooteeweet.org>,
 Dmitry Stogov <dmitry@zend.com>
References: <6604D94D40FD465F992144110B075BB5@pc1> <E17F0EFAC0F847C994EBDA6D020E52BF@PC3EE1F19287> <9D5D4CBF-5CB1-47EC-81F4-59E3C48EEEEF@pooteeweet.org> <C8E7CB69316B42D1B8BB5ED78408088D@PC3EE1F19287> <B06CC8E0A66F4959B3BFC4AA0698223D@pc1>
In-Reply-To: <B06CC8E0A66F4959B3BFC4AA0698223D@pc1>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [PATCH] Scanner "diet" with fixes, etc.
From: shire@php.net (shire)



Hey Matt,

Thanks for posting, sorry for not having a chance to reply to this sooner.  Maybe couple things from the patch,

>> +/* To save initial string length after scanning to first variable, CG(doc_comment_len) can be reused */
>> +#define double_quotes_scanned_len CG(doc_comment_len)
>> +

(minor) Maybe we should rename this var if we're going to use it for other purposes, this doesn't really save any typing.  Also if we do want the define maybe we should upper case it so it's more obvious?

>> +	while (YYCURSOR < YYLIMIT) {
>> +		switch (*YYCURSOR++) {

In the example above, which we have a couple examples of here, we don't obey the YYFILL macro to detect if we have exceeded our EOF.  This *might* be a problem, but only really depends on if we intend to use the YYFILL as a solution for exceeding our mmap bounds.

Regarding the ZEND_MMAP_AHEAD issue and the temp. fix that Dmitry put in we need to find a solution to that, perhaps I can play with that this week too as I think I'm seeing some related issues in my testing of 5.3.  Essentially we abuse ZEND_MMAP_AHEAD by adding it to the file size and passing it to the mmap call which isn't at all valid and only really works up to PAGESIZE.  We could possibly use YYFILL to re-allocate more space as necessary past the end of file to fix this.

I don't see anything glaring in the patch that's a major issue, I can probably test more on a larger code base in the next 2-3 days.  As I've said before this seems to be crossing the line of us writing a scanner by hand rather than letting re2c do the heavy lifting, but without a modification to re2c to handle EOF I don't have an alternative solution currently.  (If we had some way to detect which regex we where matching against in the YYFILL that would likely be able to handle these bugs, but I didn't see a way to do that easily).

-shire