FAQ
Hi everyone,

I've been developing a PHP extension for internal needs.
We're using C++, by using PHP_REQUIRE_CXX() in config.m4.
I'm using debian sid 64bits, with the package php5-dev-5.3.8-2
(against which the patch below has been created).

When declaring an INI entry, like this:
PHP_INI_BEGIN()
STD_PHP_INI_ENTRY("extensionname.variable1", NULL,
PHP_INI_ALL, OnUpdateString, globalsVariable1,
zend_extensionname_globals, extensionname_globals)
STD_PHP_INI_ENTRY("extensionname.variable2", NULL,
PHP_INI_ALL, OnUpdateString, globalsVariable2,
zend_extensionname_globals, extensionname_globals)
PHP_INI_END()
We get the following warning:
warning: deprecated conversion from string constant to 'char*'
[-Wwrite-strings]
I have a bunch of those, one per INI entry, and prefer to get rid of them.

As a usual fix, I changed the line to use char* C-style cast as follows:
STD_PHP_INI_ENTRY((char*)"extensionname.variable1", NULL,
PHP_INI_ALL, OnUpdateString, globalsVariable1,
zend_extensionname_globals, extensionname_globals)
STD_PHP_INI_ENTRY((char*)"extensionname.variable2", NULL,
PHP_INI_ALL, OnUpdateString, globalsVariable2,
zend_extensionname_globals, extensionname_globals)
However, this does not work, I can see through phpinfo(), listing the
extension's INI entries, that only "extensi" is listed.
I'm aware that const char* should be used instead, but even using
strdup("extensionname.variableN") does not work!

It took me a while to find the problem.
STD_PHP_INI_ENTRY resolves to STD_ZEND_INI_ENTRY which resolves to
ZEND_INI_ENTRY2, then ZEND_INI_ENTRY2_EX then in turn
ZEND_INI_ENTRY3_EX which finally uses the following with the first
macro parameter "name":
[...], name, sizeof(name), [...]
Yep, the problem comes from the sizeof(), which works on a string
constant seemingly interpreted as a (const?) char[] in C and C++,
returning sizeof(void*) instead of the length of the string (+ the
trailing \0), when using the classic char* cast.
Fine as this is probably not the right cast (it is really a very
common mistake), but just before we are setting a field of type char*,
which is the one that raises the warning.

My point is that there is a problem if it is that easy to trigger a
bug with some "natural reflex" to get rid of a warning.
I suggest some fixes:
* Use strlen() instead of sizeof().
* Use in-macro cast to char[].
* Use const when the string value won't be modified (I'm not talking
about the pointer, but its content).
In fact, I propose the following changes so that no user (extension
writer) code has to change:
--- zend_ini.h 2011-09-29 12:24:39.012882527 +0200
+++ zend_ini.h 2011-09-29 12:24:32.552577965 +0200
@@ -65,14 +65,14 @@
struct _zend_ini_entry {
int module_number;
int modifiable;
- char *name;
+ const char *name;
uint name_length;
ZEND_INI_MH((*on_modify));
void *mh_arg1;
void *mh_arg2;
void *mh_arg3;

- char *value;
+ const char *value;
uint value_length;

char *orig_value;
@@ -117,7 +117,7 @@
#define ZEND_INI_END() { 0, 0, NULL, 0, NULL, NULL, NULL,
NULL, NULL, 0, NULL, 0, 0, 0, NULL } };

#define ZEND_INI_ENTRY3_EX(name, default_value, modifiable,
on_modify, arg1, arg2, arg3, displayer) \
- { 0, modifiable, name, sizeof(name), on_modify, arg1, arg2,
arg3, default_value, sizeof(default_value)-1, NULL, 0, 0, 0, displayer
},
+ { 0, modifiable, name, strlen(name)+1, on_modify, arg1, arg2,
arg3, default_value, default_value != NULL ? strlen(default_value) :
sizeof(NULL), NULL, 0, 0, 0, displayer },

#define ZEND_INI_ENTRY3(name, default_value, modifiable, on_modify,
arg1, arg2, arg3) \
ZEND_INI_ENTRY3_EX(name, default_value, modifiable, on_modify,
arg1, arg2, arg3, NULL)

We use const char* fields not to trigger the C++ deprecation warning,
and we use strlen() to get the size of the string (which is the only
normal way anyway), but test for a non NULL value (useless for "name"
I guess). By the way, I still return sizeof(NULL) for compatibility,
but 0 should probably be a better value.

I only tested that change with building my C++ PHP extension, whole
PHP recompilation.


Best,
---
Olivier Favre
Software engineer
Yakaz
http://www.yakaz.com

Search Discussions

  • Ángel González at Sep 29, 2011 at 3:07 pm

    On 29/09/11 14:14, Olivier Favre wrote:
    Hi everyone,

    I've been developing a PHP extension for internal needs.
    We're using C++, by using PHP_REQUIRE_CXX() in config.m4.
    I'm using debian sid 64bits, with the package php5-dev-5.3.8-2
    (against which the patch below has been created). (...)
    My point is that there is a problem if it is that easy to trigger a
    bug with some "natural reflex" to get rid of a warning.
    I suggest some fixes:
    * Use strlen() instead of sizeof().
    * Use in-macro cast to char[].
    * Use const when the string value won't be modified (I'm not talking
    about the pointer, but its content).
    In fact, I propose the following changes so that no user (extension
    writer) code has to change: (...)
    We use const char* fields not to trigger the C++ deprecation warning,
    and we use strlen() to get the size of the string (which is the only
    normal way anyway), but test for a non NULL value (useless for "name"
    I guess). By the way, I still return sizeof(NULL) for compatibility,
    but 0 should probably be a better value.

    I only tested that change with building my C++ PHP extension, whole
    PHP recompilation.

    Best,
    Using strlen() there forces a runtime call to figure out the string
    length*, the
    sizeof is preferable there.
    I find the change from char* to const char* acceptable, though. Or at least
    char const*, I'm not sure if those values are changed at runtime.
    I have found in the past some places where a const keyword would be
    preferable,
    but was't used. I don't know if there's a rationale for that or is it just
    "old code". There are functions using the const keyword, so it is not a
    case of
    compatibility...


    * GCC may actually be smart enough to resolve it at compilation time,
    since it
    implements strlen() as a builtin. But you obviously can't count on that.
    I'm not
    even sure if that's legal C (or from which version). g++ doesn't
    complain, but with
    your patch of adding strlen(), I think gcc gives (on C files) the
    following warning:
    warning: initializer element is not a constant expression
  • Olivier Favre at Sep 29, 2011 at 3:42 pm
    I checked with a tiny test program, you're right about GCC complaining.
    The right fix is to make the field const (I don't know about const keyword).
    G++ won't give warnings, no error would be triggered by a broken fix.

    By the way, const char* and char const* are the same, you probably
    meant char * const.
    But that wouldn't prevent by a compilation error changing the
    referenced chars, and may lead to segfaults if that happens.
    As no segfault happens (fortunately!) I we can infer that the string
    constants aren't modified (hopefully!).
    Therefore we should use const char*, or even const char* const.
    (Finally maybe keywords are more suited for the task, depending on how
    they work ;-)

    Another thing, g++ raises another warning with the last field that
    STANDARD_MODULE_PROPERTIES_EX sets in the zend_module_entry structure
    (declared in Zend/zend_modules.h:101).
    Guess what, it is a char* too. Other fields of the structure are set
    to const char* though.

    Conclusion: I thing const char * should be used, for consistency.

    By compiling my extension, I didn't see any other warnings.


    Thanks,
    ---
    Olivier Favre
    Software engineer
    Yakaz
    http://www.yakaz.com

    2011/9/29 Ángel González <keisial@gmail.com>:
    On 29/09/11 14:14, Olivier Favre wrote:

    Hi everyone,

    I've been developing a PHP extension for internal needs.
    We're using C++, by using PHP_REQUIRE_CXX() in config.m4.
    I'm using debian sid 64bits, with the package php5-dev-5.3.8-2
    (against which the patch below has been created).

    (...)

    My point is that there is a problem if it is that easy to trigger a
    bug with some "natural reflex" to get rid of a warning.
    I suggest some fixes:
    * Use strlen() instead of sizeof().
    * Use in-macro cast to char[].
    * Use const when the string value won't be modified (I'm not talking
    about the pointer, but its content).
    In fact, I propose the following changes so that no user (extension
    writer) code has to change:

    (...)

    We use const char* fields not to trigger the C++ deprecation warning,
    and we use strlen() to get the size of the string (which is the only
    normal way anyway), but test for a non NULL value (useless for "name"
    I guess). By the way, I still return sizeof(NULL) for compatibility,
    but 0 should probably be a better value.

    I only tested that change with building my C++ PHP extension, whole
    PHP recompilation.

    Best,

    Using strlen() there forces a runtime call to figure out the string length*,
    the
    sizeof is preferable there.
    I find the change from char* to const char* acceptable, though. Or at least
    char const*, I'm not sure if those values are changed at runtime.
    I have found in the past some places where a const keyword would be
    preferable,
    but was't used. I don't know if there's a rationale for that or is it just
    "old code". There are functions using the const keyword, so it is not a case
    of
    compatibility...


    * GCC may actually be smart enough to resolve it at compilation time, since
    it
    implements strlen() as a builtin. But you obviously can't count on that. I'm
    not
    even sure if that's legal C (or from which version). g++ doesn't complain,
    but with
    your patch of adding strlen(), I think gcc gives (on C files) the following
    warning:
    warning: initializer element is not a constant expression
  • Ángel González at Sep 29, 2011 at 5:07 pm

    On 29/09/11 17:42, Olivier Favre wrote:
    I checked with a tiny test program, you're right about GCC complaining.
    The right fix is to make the field const (I don't know about const keyword).
    G++ won't give warnings, no error would be triggered by a broken fix.

    By the way, const char* and char const* are the same, you probably
    meant char * const.
    Is it?
    I never remember which one is a mutable pointer of const chars and which
    the const pointer of mutable chars so I relied on the description at
    http://stackoverflow.com/questions/162480/const-in-c#answer-162504
    before sending the mail.
  • Gustavo Lopes at Sep 29, 2011 at 6:07 pm

    On Thu, 29 Sep 2011 18:13:34 +0100, Ángel González wrote:
    On 29/09/11 17:42, Olivier Favre wrote:
    I checked with a tiny test program, you're right about GCC complaining.
    The right fix is to make the field const (I don't know about const
    keyword).
    G++ won't give warnings, no error would be triggered by a broken fix.

    By the way, const char* and char const* are the same, you probably
    meant char * const.
    Is it?
    I never remember which one is a mutable pointer of const chars and which
    the const pointer of mutable chars so I relied on the description at
    http://stackoverflow.com/questions/162480/const-in-c#answer-162504
    before sending the mail.
    const char * and char const * are the same (just like const int and int
    const are the same); what's not the same is char *const.

    --
    Gustavo Lopes
  • Andrey Hristov at Sep 29, 2011 at 6:11 pm

    On 09/29/2011 07:13 PM, Ángel González wrote:
    On 29/09/11 17:42, Olivier Favre wrote:
    I checked with a tiny test program, you're right about GCC complaining.
    The right fix is to make the field const (I don't know about const
    keyword).
    G++ won't give warnings, no error would be triggered by a broken fix.

    By the way, const char* and char const* are the same, you probably
    meant char * const.
    Is it?
    I never remember which one is a mutable pointer of const chars and which
    the const pointer of mutable chars so I relied on the description at
    http://stackoverflow.com/questions/162480/const-in-c#answer-162504
    before sending the mail.
    it's easy, whatever const is closer to is immutable
    const char * is a pointer to a const char, because the const is closer
    to the char than to the pointer.
    char * const is a const pointer to a char, because the const is closer
    to the pointer than to the char.
    Anyway, never used char const *, which will conflict with my explanation :)

    Best,
    Andrey
  • Ángel González at Sep 29, 2011 at 8:18 pm

    Gustavo Lopes wrote:
    const char * and char const * are the same (just like const int and
    int const are the same); what's not the same is char *const.
    Andrey Hristov wrote:
    it's easy, whatever const is closer to is immutable
    const char * is a pointer to a const char, because the const is closer
    to the char than to the pointer.
    char * const is a const pointer to a char, because the const is closer
    to the pointer than to the char.
    Anyway, never used char const *, which will conflict with my
    explanation :)

    Best,
    Andrey

    I stand corrected, char const * is the same as const char* [1], so any
    of them would
    work for the ini struct.
    The C++ FAQ Lite recommends reading it right to left [2], but then the
    leftmost const
    case is left undefined by the mnemonic, so it isn't valid for all cases,
    either.

    I'm also attaching a small program showing the different ways of
    accessing each of those
    and the expected warnings so that your favorite compiler can yell at you :)


    [1] http://www.parashift.com/c++-faq/const-correctness.html#faq-18.9
    [2] http://www.parashift.com/c++-faq/const-correctness.html#faq-18.5
  • Olivier Favre at Sep 30, 2011 at 9:17 am
    I keep on having the following error:
    ERROR:
    ⋅ Incorrect Captcha
    while trying to file a bug on the following page:
    https://bugs.php.net/report.php
    (Debian sid, Google Chrome 14.0.835.186)

    I tried flushing my cookies.
    There are two opened bug reports about that:
    - https://bugs.php.net/bug.php?id=54380
    - https://bugs.php.net/bug.php?id=53255
    And I would have liked to file another... if only I could!


    Anyway, if someone else is luckier, here what I'd have liked to file:

    PHP version: 5.3.8
    Package affected: Compile issues/Compilation warning
    Bug type: Feature/Change request
    OS: All (seen under Linux)
    Summary: char* field should be const char* to avoid C++ warning
    Description:
    http://news.php.net/php.internals/55662

    I'm writing a C++ extension to PHP.
    When declaring a INI entry I get the following warning, multiple times:
    warning: deprecated conversion from string constant to 'char*' [-Wwrite-strings]

    This only arises when compiling with a C++ compiler.
    The right and easy fix seems to set some fields to const char *.
    Some may even be set to const char * const (but this alternative seems
    to be used
    nowhere).

    The proposed patch is against the php5-dev-5.3.8-2 package of debian sid:
    PHP 5.3.8-2 with Suhosin-Patch (cli) (built: Sep 12 2011 07:28:26)
    - - -
    Test script:
    Write a C++ extension:
    config.m4 should contain PHP_REQUIRE_CXX().

    Declare your module:
    zend_module_entry quezako_module_entry = {
    STANDARD_MODULE_HEADER,
    "YourExtensionName", // (1 warning here)
    [...],
    "0.42", // (1 warning here)
    [...],
    STANDARD_MODULE_PROPERTIES_EX
    };

    Declare an INI entry:
    PHP_INI_BEGIN()
    STD_PHP_INI_ENTRY(
    "extensionName.variable", // (1 warning here)
    "default value", // (1 warning here)
    [...]
    )
    PHP_INI_END()
    - - -
    Patch name: field_constness_cpp_compilation_warning_fix.patch
    Patch file: (see attached file)
    Expected result:
    No compilation warning.
    - - -
    Actual result:
    Multiple of the following warning:
    warning: deprecated conversion from string constant to 'char*' [-Wwrite-strings]

    Using the very common fix of prepending (char*) to the string constant is
    especially harmful here, because of ZEND_INI_ENTRY3_EX using sizeof() on in: it
    returns 4/8 (32/64bits platform).
    Using a cast to char[] solves the problem, but the above fix is a very very
    common mistake.

    If the target fields were const char*, no compilation warning would be rose.
    - - -
    Solve the problem 17 + 23 = ?: 40 (I even checked the answer using a
    calculator!!!!)
    Submit: Send bug report (I'm going mad, really...)

    --
    Olivier Favre
    Software engineer
    Yakaz
    www.yakaz.com
  • Olivier Favre at Oct 3, 2011 at 8:02 am
    I finally managed to submit the patch... I used iceweasel 6.0.2
    instead of Google Chrome 14.0.835.186... or maybe the captcha was just
    in a better mood!

    The link to the bug report: https://bugs.php.net/bug.php?id=55835


    Yours,
    --
    Olivier Favre
    Software engineer
    Yakaz
    www.yakaz.com

    2011/9/30 Olivier Favre <olivier@yakaz.com>
    I keep on having the following error:
    ERROR:
    ⋅ Incorrect Captcha
    while trying to file a bug on the following page:
    https://bugs.php.net/report.php
    (Debian sid, Google Chrome 14.0.835.186)

    I tried flushing my cookies.
    There are two opened bug reports about that:
    - https://bugs.php.net/bug.php?id=54380
    - https://bugs.php.net/bug.php?id=53255
    And I would have liked to file another... if only I could!


    Anyway, if someone else is luckier, here what I'd have liked to file:

    PHP version: 5.3.8
    Package affected: Compile issues/Compilation warning
    Bug type: Feature/Change request
    OS: All (seen under Linux)
    Summary: char* field should be const char* to avoid C++ warning
    Description:
    http://news.php.net/php.internals/55662

    I'm writing a C++ extension to PHP.
    When declaring a INI entry I get the following warning, multiple times:
    warning: deprecated conversion from string constant to 'char*' [-Wwrite-strings]

    This only arises when compiling with a C++ compiler.
    The right and easy fix seems to set some fields to const char *.
    Some may even be set to const char * const (but this alternative seems
    to be used
    nowhere).

    The proposed patch is against the php5-dev-5.3.8-2 package of debian sid:
    PHP 5.3.8-2 with Suhosin-Patch (cli) (built: Sep 12 2011 07:28:26)
    - - -
    Test script:
    Write a C++ extension:
    config.m4 should contain PHP_REQUIRE_CXX().

    Declare your module:
    zend_module_entry quezako_module_entry = {
    STANDARD_MODULE_HEADER,
    "YourExtensionName", // (1 warning here)
    [...],
    "0.42", // (1 warning here)
    [...],
    STANDARD_MODULE_PROPERTIES_EX
    };

    Declare an INI entry:
    PHP_INI_BEGIN()
    STD_PHP_INI_ENTRY(
    "extensionName.variable", // (1 warning here)
    "default value", // (1 warning here)
    [...]
    )
    PHP_INI_END()
    - - -
    Patch name: field_constness_cpp_compilation_warning_fix.patch
    Patch file: (see attached file)
    Expected result:
    No compilation warning.
    - - -
    Actual result:
    Multiple of the following warning:
    warning: deprecated conversion from string constant to 'char*' [-Wwrite-strings]

    Using the very common fix of prepending (char*) to the string constant is
    especially harmful here, because of ZEND_INI_ENTRY3_EX using sizeof() on in: it
    returns 4/8 (32/64bits platform).
    Using a cast to char[] solves the problem, but the above fix is a very very
    common mistake.

    If the target fields were const char*, no compilation warning would be rose.
    - - -
    Solve the problem 17 + 23 = ?: 40 (I even checked the answer using a
    calculator!!!!)
    Submit: Send bug report (I'm going mad, really...)

    --
    Olivier Favre
    Software engineer
    Yakaz
    www.yakaz.com
  • Ferenc Kovacs at Oct 3, 2011 at 8:04 am
    there was a problem with bugs.php.net, so the captcha error was on our end.
    thanks for the report
    On Mon, Oct 3, 2011 at 10:01 AM, Olivier Favre wrote:
    I finally managed to submit the patch... I used iceweasel 6.0.2
    instead of Google Chrome 14.0.835.186... or maybe the captcha was just
    in a better mood!
    --
    Ferenc Kovács
    @Tyr43l - http://tyrael.hu

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedSep 29, '11 at 12:14p
activeOct 3, '11 at 8:04a
posts10
users5
websitephp.net

People

Translate

site design / logo © 2022 Grokbase