Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build failed at mbstring_arginfo.h on Windows(Visual C++) #13789

Closed
youkidearitai opened this issue Mar 23, 2024 · 6 comments
Closed

Build failed at mbstring_arginfo.h on Windows(Visual C++) #13789

youkidearitai opened this issue Mar 23, 2024 · 6 comments

Comments

@youkidearitai
Copy link
Contributor

Description

Build failed on Visual C++. mb_trim uses UTF-8 characters.

C:\php-sdk\php-src\ext\mbstring\mbstring_arginfo.h(127): error C2001: 定数が 2 行目に続いています。 (ソース ファイルをコンパイルしています ext\mbstring\mbstring.c)
C:\php-sdk\php-src\ext\mbstring\mbstring_arginfo.h(127): fatal error C1057: マクロ展開中に予期せぬ EOF を検出しました。 (ソース ファイルをコンパイルしています ext\mbstring\mbstring.c)
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64\cl.exe"' : リターン コード '0x2'
Stop.

My environment is below:

  • Visual C++ 2019
  • Windows 11
  • Probably default character is cp932

I confirmed can compile below:

 ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, characters, IS_STRING, 0, "\"\\f\\n\\r\\t\\v\\x00\240\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u2028\u2029\u202f\u205f\u3000\205\u180e\"")

Probably, .stub.php is convert to UTF-8 characters, but Visual C++ can not compile.

I think there are multiple ways to fix it.

  • stub.php is not convert UTF-8 characters
  • As abode, fix mbstring_arginfo.h to escaped character.

Reported from Hirokawa san.

PHP Version

master

Operating System

Windows

youkidearitai added a commit to youkidearitai/php-src that referenced this issue Mar 26, 2024
mb_trim uses Unicode special character.
Some environment(Japanese CP932) VC++ is fail to compile.
I found solution, insert Byte Order Mark.
However, scope of impact is very big. Hence add @insert-bom block
comment.
youkidearitai added a commit to youkidearitai/php-src that referenced this issue Mar 26, 2024
mb_trim uses Unicode special character.
Some environment(Japanese CP932) VC++ is fail to compile.
I found solution that insert Byte Order Mark.
However, scope of impact is very big. Hence add @insert-bom block
comment.
youkidearitai added a commit to youkidearitai/php-src that referenced this issue Mar 26, 2024
mb_trim uses Unicode special character.
Some environment(Japanese CP932) VC++ is fail to compile.
I found solution that insert Byte Order Mark.
However, scope of impact is very big. Hence add @insert-bom block
comment at .stub.php file.
youkidearitai added a commit to youkidearitai/php-src that referenced this issue Mar 26, 2024
mbstring.stub.php uses Unicode special character.
Some environment(Japanese CP932) VC++ is fail to compile.
I found solution that insert Byte Order Mark.
However, scope of impact is very big. Hence add @insert-bom block
comment at .stub.php file.
@youkidearitai
Copy link
Contributor Author

I would tried to #13811 that include Byte Order Mark. That approach is my environment (CP932) is passed, However this CI (CP1252 with /WX flag) is down. I should find other way.

@kocsismate
Copy link
Member

Could these special characters converted to escape sequences? Maybe via using mb_ord()?

@ranvis
Copy link
Contributor

ranvis commented Mar 26, 2024

I'd recommend using the /utf-8 option.

From what I understood, MS cl.exe reads source code (without BOM) using the /source-charset encoding, compiles it, and writes char with the /execution-charset encoding. By default, the codepage of the compiling environment is used for both encodings.

\uXXXX escaping or BOM should solve the first part of the problem. However, /execution-charset:utf-8 is now required, or the string in question will no longer be decodable as UTF-8.

The /utf-8 option, which is a shorthand for the both options mentioned, can be used without source code changes. (I'm not saying escaping invisible/formatting characters are not good.)


I don't know how to integrate the flag, though.
I tried adding /utf-8 to the parameter of EXTENSION() in ext/mbstring/config.w32, but that also caused CFLAGS to be propagated globally if the extension is statically linked:

CFLAGS_MBSTRING=/DHAVE_MBSTRING_H=1 /utf-8 ...

STATIC_EXT_CFLAGS=... $(CFLAGS_MBSTRING) ...

CFLAGS_PHP_OBJ=$(CFLAGS_PHP) $(STATIC_EXT_CFLAGS) 

It should still work if a use of pragma in another extension is removed (#13814), yet it looks not nice.
If CFLAGS_BD_EXT_MBSTRING (BD stands for build dir) is used instead, the propagation will not happen. But I'm not sure if the variable is a public interface. No bundled extensions make use of it.

After all, I don't think the default value for mb_trim functions are appropriate. (see #13815)


... \240 ... \205 ...

It is missed that \ooo and \xXX encode a raw octet, which do not form a valid UTF-8 sequence alone. Like \xc2 must be prefixed for these.
So yeah, these can be converted to sequences of \x... instead of \u....

youkidearitai added a commit to youkidearitai/php-src that referenced this issue Apr 7, 2024
…ndows

Probably CP932 environment can't compile. So add /utf-8 flag.
@kocsismate
Copy link
Member

Thanks for the fix!

@petk
Copy link
Member

petk commented Apr 10, 2024

Just out of curiosity, can't these special characters be encoded with some combination of \ hex codes instead in the ext/mbstring/mbstring_arginfo.h file directly? Also because opening this file has issues in code editors and you get a warning about unusual line terminators.

@nielsdos
Copy link
Member

Just out of curiosity, can't these special characters be encoded with some combination of \ hex codes instead in the ext/mbstring/mbstring_arginfo.h file directly? Also because opening this file has issues in code editors and you get a warning about unusual line terminators.

That will work too, requires modifying the stub generator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants