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

Fix a few #ifdef vs. #if issues #66898

Merged
merged 3 commits into from
Mar 22, 2022
Merged

Conversation

am11
Copy link
Member

@am11 am11 commented Mar 20, 2022

Also upstreamed a fix for libunwind libunwind/libunwind#342 (but we are not using ptrace so it can wait until the next libunwind update).

cc @akoeplinger, @jkotas

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 20, 2022
@ghost
Copy link

ghost commented Mar 20, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Also upstreamed a fix for libunwind libunwind/libunwind#342 (but we are not using ptrace so it can wait until the next libunwind update).

cc @akoeplinger, @jkotas

Author: am11
Assignees: -
Labels:

area-Diagnostics-coreclr

Milestone: -

@am11
Copy link
Member Author

am11 commented Mar 20, 2022

Ah, actually we are compiling libunwind_ptrace_la_SOURCES. Applied the patch.

@tommcdon
Copy link
Member

@hoyosjs @josalem

@@ -42,9 +42,9 @@ SET_DEFAULT_DEBUG_CHANNEL(DEBUG); // some headers have code with asserts, so do
#include <unistd.h>
#if HAVE_PROCFS_CTL
#include <unistd.h>
#elif HAVE_TTRACE // HAVE_PROCFS_CTL
#elif defined(HAVE_TTRACE) // HAVE_PROCFS_CTL
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we use cmakedefine/defined here over the existing check? The other cases out of libunwind make sense as they were always defined and they are "HAVE" remarks. Is this because libunwind uses this pattern more often?

Copy link
Member Author

@am11 am11 Mar 21, 2022

Choose a reason for hiding this comment

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

Yup, correct.

It is actually related to config.h.in change, we are using same define (pr: #cmakedefine HAVE_TTRACE, main: #cmakedefine01 HAVE_TTRACE) for libunwind and debug.cpp. So I converged it to libunwind's way.

When we use #cmakedefine01 FOO, it generates #define FOO 1 (when feature is detected) or #define FOO 0 (when detection fails), so defined(FOO) is always true and it is a logical error if we use that. With #cmakedefine FOO, it generates #define FOO or nothing when detection fails. In that case #if FOO issues a warning Wundef.

Copy link
Member

Choose a reason for hiding this comment

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

I just wanted to know why use the define divergent when everything uses the logical one. If it's for libunwind consistency that's all good.

@akoeplinger akoeplinger merged commit a484644 into dotnet:main Mar 22, 2022
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
Also upstreamed a fix for libunwind libunwind/libunwind#342 (but we are not using ptrace so it can wait until the next libunwind update).
@ghost ghost locked as resolved and limited conversation to collaborators Apr 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Diagnostics-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants