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.

Sign up for GitHub

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

Jump to bottom

[libpas] pas_utils.c fails to build due to missing parameter names in pas_crash_with_info_impl() #1878

Merged
merged 1 commit into from Jun 30, 2022
Merged

[libpas] pas_utils.c fails to build due to missing parameter names in pas_crash_with_info_impl() #1878

merged 1 commit into from Jun 30, 2022

Conversation

aperezdc
Copy link
Contributor

@aperezdc aperezdc commented Jun 28, 2022 •
edited by webkit-commit-queue

ff4150d

[libpas] pas_utils.c fails to build due to missing parameter names in pas_crash_with_info_impl()
https://bugs.webkit.org/show_bug.cgi?id=242090

Reviewed by Yusuke Suzuki.

* Source/bmalloc/libpas/src/libpas/pas_utils.c:
(pas_crash_with_info_impl): Add missing parameter names, and use
PAS_IGNORE_WARNINGS_{BEGIN,END} to avoid compiler warnings about
unused variables.

Canonical link: https://commits.webkit.org/251986@main

@aperezdc aperezdc self-assigned this Jun 28, 2022
@aperezdc aperezdc added Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases WebKit Local Build labels Jun 28, 2022
@Constellation
Copy link
Member

cdd4151

[libpas] pas_utils.c fails to build due to missing parameter names in pas_crash_with_info_impl()
https://bugs.webkit.org/show_bug.cgi?id=242090[](https://bugs.webkit.org/show_bug.cgi?id=242090)

Reviewed by NOBODY (OOPS!).

  • Source/bmalloc/libpas/src/libpas/pas_utils.c:
    (pas_crash_with_info_impl): Add missing parameter names, and use them
    in void statements to avoid compiler warnings about unused variables.

Why does it cause warnings? They are not used, and they do not have parameter names. I think this code should be fine.

Constellation
Constellation requested changes Jun 29, 2022
View reviewed changes
Source/bmalloc/libpas/src/libpas/pas_utils.c Outdated
@@ -147,7 +147,18 @@ PAS_NEVER_INLINE PAS_NO_RETURN static void pas_crash_with_info_impl(uint64_t rea

#else

PAS_NEVER_INLINE PAS_NO_RETURN static void pas_crash_with_info_impl(uint64_t, uint64_t, uint64_t, uint64_t, uint64_t, uint64_t, uint64_t) { __builtin_trap(); }
PAS_NEVER_INLINE PAS_NO_RETURN static void pas_crash_with_info_impl(uint64_t reason, uint64_t misc1, uint64_t misc2, uint64_t misc3, uint64_t misc4, uint64_t misc5, uint64_t misc6)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's suppress warnings instead. We strongly want to keep this function having only __builtin_trap.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's use PAS_IGNORE_GCC_WARNINGS_{BEGIN,END}

@aperezdc
Copy link
Contributor Author

cdd4151

[libpas] pas_utils.c fails to build due to missing parameter names in pas_crash_with_info_impl()
[https://bugs.webkit.org/show_bug.cgi?id=242090](https://bugs.webkit.org/show_bug.cgi?id=242090%5B%5D(https://bugs.webkit.org/show_bug.cgi?id=242090))
Reviewed by NOBODY (OOPS!).

  • Source/bmalloc/libpas/src/libpas/pas_utils.c:
    (pas_crash_with_info_impl): Add missing parameter names, and use them
    in void statements to avoid compiler warnings about unused variables.

Why does it cause warnings? They are not used, and they do not have parameter names. I think this code should be fine.

This source file is C (not C++), and parameter names in function declarations are mandatory. At least until the C2x spec is published and used in WebKit.

I remember seeing warnings about “C source being compiled in C++ mode” shown during compilation in the past so that's why we haven't seen the errors before.

I can change the void statements to PAS_UNUSED_PARAM, or wrap the function declaration between PAS_IGNORE_GCC_WARNINGS_{BEGIN,END}. Which approach would we prefer here?

@Constellation
Copy link
Member

I can change the void statements to PAS_UNUSED_PARAM, or wrap the function declaration between PAS_IGNORE_GCC_WARNINGS_{BEGIN,END}. Which approach would we prefer here?

PAS_IGNORE_GCC_WARNINGS_{BEGIN,END} is better.

Constellation
Constellation requested changes Jun 29, 2022
View reviewed changes
Source/bmalloc/libpas/src/libpas/pas_utils.c Outdated
@@ -147,7 +147,18 @@ PAS_NEVER_INLINE PAS_NO_RETURN static void pas_crash_with_info_impl(uint64_t rea

#else

PAS_NEVER_INLINE PAS_NO_RETURN static void pas_crash_with_info_impl(uint64_t, uint64_t, uint64_t, uint64_t, uint64_t, uint64_t, uint64_t) { __builtin_trap(); }
PAS_NEVER_INLINE PAS_NO_RETURN static void pas_crash_with_info_impl(uint64_t reason, uint64_t misc1, uint64_t misc2, uint64_t misc3, uint64_t misc4, uint64_t misc5, uint64_t misc6)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's use PAS_IGNORE_GCC_WARNINGS_{BEGIN,END}

@aperezdc aperezdc force-pushed the eng/libpas-pas_utils-c-fails-to-build-due-to-missing-parameter-names-in-pas_crash_with_info_impl branch from cdd4151 to b08103e Compare June 29, 2022 22:56
Constellation
Constellation approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@aperezdc aperezdc added the merge-queue Applied to send a pull request to merge-queue label Jun 30, 2022
@aperezdc
Copy link
Contributor Author

r=me

Thanks @Constellation!

@aperezdc
[libpas] pas_utils.c fails to build due to missing parameter names in…
ff4150d 
… pas_crash_with_info_impl()

https://bugs.webkit.org/show_bug.cgi?id=242090

Reviewed by Yusuke Suzuki.

* Source/bmalloc/libpas/src/libpas/pas_utils.c:
(pas_crash_with_info_impl): Add missing parameter names, and use
PAS_IGNORE_WARNINGS_{BEGIN,END} to avoid compiler warnings about
unused variables.

Canonical link: https://commits.webkit.org/251986@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/libpas-pas_utils-c-fails-to-build-due-to-missing-parameter-names-in-pas_crash_with_info_impl branch from b08103e to ff4150d Compare June 30, 2022 10:56
@webkit-commit-queue
Copy link
Collaborator

Committed 251986@main (ff4150d): https://commits.webkit.org/251986@main

Reviewed commits have been landed. Closing PR #1878 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit ff4150d into WebKit:main Jun 30, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 30, 2022
@aperezdc aperezdc deleted the eng/libpas-pas_utils-c-fails-to-build-due-to-missing-parameter-names-in-pas_crash_with_info_impl branch June 30, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Constellation Constellation Constellation approved these changes

Assignees

@aperezdc aperezdc

Labels
Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases
Projects
None yet
Milestone
No milestone
4 participants
@aperezdc @Constellation @webkit-commit-queue @webkit-early-warning-system