Hi Hannes,

Hi Nikita,

Hi,

https://wiki.php.net/rfc/size_t_and_int64

The discussion time has come. The work on the feature branch
continues. The current patch is stable enough to be discussed.

What is the reason behind the renames of IS_LONG to IS_INT (and
Z_LVAL to
Z_IVAL etc), as well as the renames in zpp (s -> S, etc)? Why can't we
keep the old names here? That should reduce the amount of ifndefs
involved a lot, as you'd only have to do it for the type declarations
themselves, not for every single usage. Or is the point here to
intentionally provide a maximum amount of BC breakage, so code doesn't
"accidentally" continue
to run (e.g. I think that without the renames extensions could
continue to run mostly without issue on 32bit.)

Nikita

the renames you mention like IS_LONG -> IS_INT are thought more for
correct semantic, as there is no firm 'long' anymore. The same for
Z_STRLEN -> Z_STRSIZE and others. That kind of thing should be done the
most obvious way. Well, the max BC reason you gave I like too :)

For the same reason zend_parse_parameters() formats was changed, as
it'll issue an error on runtime. However here I still scratch my head as
that's a runtime issue, but it should break the compilation as well.

Without semantic replacements many extensions would just continue to
compile/run on 32 bit, indeed. Though because of the size_t one could
still have some unpleasant surprise in some situation. Besides that,
I'd
really see that more like a side effect with not very clear use.

With the #ifdef's - there shouldn't be any or should be very few. The
compatibility header I've mentioned in one of the previous responses
should do it one for all. It can look like

#if PHP_MAJOR_VERSION < 6

define IS_INT IS_LONG

define php_size_t int

.......
#endif

Once included and given an extension is compiled with an older PHP
version, that defines should cover the compatibility with older
semantics. Some more complicated solution will probably be needed for
zpp to replace "i" with "l" and co. for the older PHP. But generally,
such a header should make the same ext source in 6.x style compatible
with 5.x branch. Of course some exceptions will have to take place, but
i think those will be not more than the current mainstream already
contains to separate the code for minor versions in 5.x branch.

The migration path is the very next thing I have to do.

I am really confused on that. How is the compatibility header useful
if I still need a ifdef else for zpp and things that boil down to printfs,
like error reporting, with 2 different arguments?
with the php_error_docref() - very good point, the header can't help much.
Except to define something like php_error_docref_comp() as a wrapper
function in the compat header, so %pd and alike can be replaced on
runtime. The code might miss some #ifdefs then, so be cleaner.

Wouldn't it be better to not change them, but maybe force extensions
to define a _I_SUPPORT_PHP6, if not defined refuse to build the ext against
PHP6?

Yeah, I'd say an explicit define to indicate PHP6 readiness is good point
as well.

About zpp I've got a click - the compatibility with 5.x can be integrated
into zpp itself in 6.x. I mean look here
http://git.php.net/?p=php-src.git;a=blob;f=Zend/zend_API.c;hb=refs/heads/str_size_and_int64#l326

• 'l' and 'L' was replaced with 'i' and 'I', but nothing prevents to turn
  those formats as aliases in 6.x. That way 'l' and 'i' and another pairs
  will do the same thing, new implementations can use clean semantics, and
  the old formats can be removed after 5.x EOL. This solution however won't
  force the new semantic.

Another way i could suggest for zpp is similarly to docrefs using a
wrapper like zend_parse_parameters_comp(), which would replace new for
old. Still not very nice but would guarantee semantically correct formats.

Then all I have to do in extension is to include the compat header,
change some of my types passed to zpp/printfs and define that macro.. and I
could support PHP5 and PHP6.

And if I try to build against PHP6 it would result in compiler failure...
We'd maybe need to tweak the build system a tiny bit, and phpize, for
that to happen - but that seem to be much easier then supporting PHP5 and 6
for extensions in the same codebase with renamed zpp values?

Sounds plausible to me. Parameters parsing and spprintf are the only
cracky places I can think of now, the other macros/function renames should
be easy covered with the compat header, so can me machinable replaced with
some tool, even sed script.

The _I_SUPPORT_PHP6 def (should be defined before includes) can be checked
directly in php.h, where an #error can be thrown. That sounds like a sane
migration workflow. Like if I'm not aware of 6.x and try to compile just
right on, it'll reject. If I explicitly say, that i'm ready for 6.x, means
i've done the porting work before.

So for the migration it'd be like

• run the replacement tool on the sources for simple semantic fixes
• include compat header
• #define PHP6_SUPPORTED
• replace appropriate things with *_compat() wrappers or do some #ifdefs
• fix datatypes for function args
• fix the extension code where appropriate (that might be big or not, as
  Jan meant some exts he's tried was easy)
• check if it plays good with some lib i link against, do some range
  checks eventually

Sounds like a plan.

Thanks for the ideas!

Anatol

re,

I completely agree with Nikita.
Why to rename LONG->INT STRLEN->STRSIZE in thousands places?
Why not just define zend_long and zend_ulong to be 64-bit on 64-bit
platforms and use them instead of int, ulint, zend_int, zend_uint, long,
ulong where it's necessary.

Anatol, I understood your point about catching incompatibility code at
compile-time, but I'm not sure if the new features cost such huge code base
changes.

To catch 64bit issues at compile is immensely valuable. We had so many
issues in the past, some of them leading to security issues. It is
also a one time job, with a little extra effort for two years.
Besides the portability improvements, code review and correctness is
one of the goal of this RFC. I can't remember to know any widely known
and sane project relying on on other type for buffer length. PHP does
not have to be different :)

1. 64-bit integers on Windows (they are already 64-bit on other systems)

Almost all :)

2. 64-bit string length. I don't think many people are interested in that.
   Fortunately, the patch doesn't change the zval size, so it shouldn't make a
   lot of harm. However, usage of "zend_size_t" instead of "int" is a bit
   annoying. I would change it into the same "zend_long" or "zend_ulong".

It is not about being interested but preventing many security issues
as well, by default. The large buffer availability is a good side
effect. The implementation correctness is also drastically improved in
this case.

After a short chat with Dmitry, it seems that the only open questions are:

• renaming
  . pros
  . 100% compile time checks, unvaluable
  . clarity, no magic as we have now
  . code correctness, any 64bit port guides cover things the way we
  implemented it

  . cons
  . renaming will create extra work for the next 2 years
  . annoying (while I can live with it given the benefits in the long run)

• SAPIs
  We have discussed many times the removal of the dead SAPIs. I think
  it is time to remove them and keep only those maintained and actually
  working (some sapis are not even possible to use anymore)

I think we can clear these questions during the vote phases, as options.

If anyone has an idea how to keep the advantage brought by the
renaming (compile checks, clarity, code correctness) without actually
doing the renaming in extensions, please fire it :)

Cheers,

Pierre

@pierrejoye | http://www.libgd.org

Hi Anatol,

Hi Dmitry,

greateful thanks for taking a look at this patch.

I completely agree with Nikita.
Why to rename LONG->INT STRLEN->STRSIZE in thousands places?
Why not just define zend_long and zend_ulong to be 64-bit on 64-bit
platforms and use them instead of int, ulint, zend_int, zend_uint, long,
ulong where it's necessary.

Anatol, I understood your point about catching incompatibility code at
compile-time, but I'm not sure if the new features cost such huge code
base changes.
Firstly it's the historical reason as while porting it "had" to break so
one could easy see the relevant places. After that - it's also a
transformation in the mind, as that int placeholders do not depend on a
fixed datatype anymore.

historical reason of the patch itself doesn't matter :)
If there's no significant reason to introduce new names lets use old ones.

Even with new names anyone is able to change macros but not types.
It's better to use some smarter compile-time protection.
I would try to play with "compile time assert" (in GCC it must be even
possible to use __builtin_types_compatible_b()).
For example:

#define CONCAT_TOKENS(a, b) a ## b
#define EXPAND_THEN_CONCAT(a, b) CONCAT_TOKENS(a, b)
#define COMPILE_TIME_ASSERT(expr) do {
enum { EXPAND_THEN_CONCAT(ASSERT_line_,LINE) = 1 /
!!(expr) };
} while(0)

int main()
{
int a;

    COMPILE_TIME_ASSERT(__builtin_types_compatible_p(typeof(a), long));
    COMPILE_TIME_ASSERT(sizeof(a) == 8);
    return 0;

}

1. 64-bit integers on Windows (they are already 64-bit on other systems)
2. 64-bit string length. I don't think many people are interested in
   that.
   Fortunately, the patch doesn't change the zval size, so it shouldn't
   make a lot of harm. However, usage of "zend_size_t" instead of "int" is a
   bit annoying. I would change it into the same "zend_long" or
   "zend_ulong".

The original patch was for size_t only. With only that it were Linux/Unix
only improvement, as size_t is 64 bit on Windows so it'd have to stay int
or become just unsigned. Omitting the size_t change and doing int64 were
only improvement on Windows. Adding both is the three-way improvement -
Linux and Windows with biggest possible strings, Windows with 64 bit
integers. So in fact, doing only one of those wouldn't IMHO justify all
the effort.

After some thoughts I think that usage of "size_t" is a good thing for the
future support of X32 ABI.

X86: sizeof(int)=4 sizeof(long)=4 sizeof(sizet_t)=4
X86-64 (Linux): sizeof(int)=4 sizeof(long)=8 sizeof(sizet_t)=8
X86-64 (Win64): sizeof(int)=4 sizeof(long)=4 sizeof(sizet_t)=8
X32: sizeof(int)=4 sizeof(long)=8 sizeof(sizet_t)=4

Additional Windows improvement "for free" is the whole file API
exhausting, so large file objects and offsets.

That's true, the possibility to process gigabytes of data in memory will
not be needed every day, however the presence of it is something else.
Like, why should I be interested on something not available anyway?

Keeping size_t separated semantically is good for several reasons. It's
clean with the specification. Should it come to 64 bit integer on 32 bit
platform, it's easier to continue (merging size_t and ulong would break
this option). And, one day it can come to 128 bit integers (for what
reasons ever). I know, it's science fiction now, but wasn't a RAM size of
1Gb so 10 years ago? You never know. So size_t separated from unsigned int
is a good thing imho and keeps some interesting options open for the
future.

Agreed.

At the end line, the new vs. old names is the last thing I personally
would ultimately hang on, given the essential modification is in place.
However the code clearly expressing what happens is something I'd call
more appropriate with such a big substantial change.

It's questionable. I' mot sure what names are better but big changes are
always annoying.
May be it makes sense to extend votiing with options ("with renaming" and
"without renaming")

Thanks. Dmitry.

Best regards

Anatol

On 24 January 2014 01:26, Christopher Jones
christopher.jones@oracle.com wrote:

Overall

• I'm fine with changes going into, say, PHP 5.7
• I'm fine with the type renaming
• I'd prefer ZPP compatibility was kept
• I'm fine with dropping support for old SAPIs, but I think warning
  is needed no matter how unusable they currently are e.g. this means
  merging the feature to PHP 5.7

This is pretty much how I feel about this too. As a user, it's
something I'd like to have, but as a maintainer or developer on a few
extensions (some PECL, some third party), this isn't something I'm
terribly excited about having on a few months notice for 5.6 — it's
going to be an annoying, subtle change for code that has to support
older versions, as worthwhile as it is in the long term.

The same amount of effort is required for 5.6+int64 changes than with
5.4 or 5.5. This is really easy to migrate. Yes, the zpp question has
to be solved but it should not stop this patch to get in, it is ready,
tested (as much as we can), etc. Delaying its integration will not
help anyone and the same problem will happen in 5.7 or 6. Let alone
the unnecessary wait for a long due change, cleanup and correctness
change, this wait will be even insanely long if we go with 6, both
from an availability (2 years+) and adoption point of view.

I really ask everyone to at least read the migration doc and try it,
with an extension. I did it with my exts (and I have many), and it is
really straightforward and the efforts pay off given the benefits of
this patch.

Cheers,

Pierre

@pierrejoye | http://www.libgd.org