Page MenuHomePhabricator

[libcxx] Rename PS() macro to avoid clashing with Xtensa register name
ClosedPublic

Authored by gustavonihei on Mar 25 2022, 6:44 AM.

Details

Summary

This patch addresses a clash with the PS register from Xtensa
defined in the <specreg.h> header file, which is commonly included in
OS implementation.

Issue identified while building libc++ port for Apache NuttX, targeting
Xtensa-based chips (e.g. Espressif's ESP32).

Signed-off-by: Gustavo Henrique Nihei <gustavo.nihei@espressif.com>

Diff Detail

Event Timeline

gustavonihei created this revision.Mar 25 2022, 6:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2022, 6:44 AM

[libcxx] Add UNICODE definition to be used by <windows.h>

[libcxx] Defined UNICODE to 1 to prevent macro redefinition warning

gustavonihei published this revision for review.Mar 25 2022, 11:18 AM
gustavonihei edited the summary of this revision. (Show Details)
gustavonihei added reviewers: ldionne, philnik.
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2022, 11:19 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I think the reasoning/argument here is backwards. I wouldn't want to bring in the TEXT() macro here; what we do here is unrelated to the unicode switch for the Windows APIs.

So the issue is that the PS() macro we use right now can clash with some header on some OS? If that's the case, I don't mind changing it to a slightly longer macro, e.g. PATHSTR() or something like that, but please keep it untangled from the Win32 TEXT()/UNICODE macros.

I think the reasoning/argument here is backwards. I wouldn't want to bring in the TEXT() macro here; what we do here is unrelated to the unicode switch for the Windows APIs.

So the issue is that the PS() macro we use right now can clash with some header on some OS? If that's the case, I don't mind changing it to a slightly longer macro, e.g. PATHSTR() or something like that, but please keep it untangled from the Win32 TEXT()/UNICODE macros.

Yes, it is clashing with another one defined for Xtensa CPUs, commonly included in several OSes:
https://android.googlesource.com/kernel/common/+/fd43fe19b830d6cd0eba08a6c6a5f71a6bd9c1b0/include/asm-xtensa/xtensa/config-linux_be/specreg.h#71
https://github.com/espressif/esp-idf/blob/master/components/xtensa/include/xtensa/specreg.h#L96

I'll update the patch according to your suggestion.
Thanks for the feedback!

Revert previous patch and rename PS() macro according to suggestion from review

Thanks, this looks ok to me. A libc++ approver still will need to give the final verdict though.

philnik accepted this revision as: philnik.Mar 25 2022, 4:28 PM

Looks OK to me. Please update the title and provide "Your name" <your.email@example.com> for attribution if you don't have commit access. Do you want libc++ to officially support some other OS? If not we might break your platform at any time. If you want support you have to provide some CI bots of course.

gustavonihei retitled this revision from [libcxx] Reuse TEXT macro from WIN32 for mapping strings to UNICODE to [libcxx] Rename PS() macro to avoid clashing with Xtensa register name.Mar 25 2022, 4:56 PM
gustavonihei edited the summary of this revision. (Show Details)
gustavonihei edited the summary of this revision. (Show Details)Mar 25 2022, 5:01 PM

Looks OK to me. Please update the title and provide "Your name" <your.email@example.com> for attribution if you don't have commit access. Do you want libc++ to officially support some other OS? If not we might break your platform at any time. If you want support you have to provide some CI bots of course.

I've update the title and description with information about me for attribution.
Thanks, @philnik!

ldionne accepted this revision.Apr 8 2022, 1:58 PM

LGTM, but I want to mention that this setup is broken for defining a non-namespaced macro with such a short name. It is bound to conflict with tons of other software, and it would be better to fix it in the OS header instead.

This revision is now accepted and ready to land.Apr 8 2022, 1:58 PM