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

[mono] Fix typo in detecting sys/auxv.h #66894

Merged
merged 4 commits into from
Mar 21, 2022

Conversation

akoeplinger
Copy link
Member

We were checking sts/auxv.h instead of sys/auxv.h so we were never defining HAVE_SYS_AUXV_H.

Instead of fixing the typo let's switch to checking for HAVE_GETAUXVAL which is what coreclr/libs.native are using.

We were checking `sts/auxv.h` instead of `sys/auxv.h` so we were never defining `HAVE_SYS_AUXV_H`.

Instead of fixing the typo let's switch to checking for `HAVE_GETAUXVAL` which is what coreclr/libs.native are using.
@@ -22,7 +22,7 @@

#include "mono/utils/mono-hwcap.h"

#if defined(HAVE_SYS_AUXV_H) && !defined(HOST_ANDROID)
#if defined(HAVE_GETAUXVAL) && !defined(HOST_ANDROID)
Copy link
Member

Choose a reason for hiding this comment

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

#if HAVE_GETAUXVAL since it is using cmakedefine01 instead of cmakedefine.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah thanks, I've changed it to cmakedefine since that is what we mostly use in mono.

Copy link
Member

Choose a reason for hiding this comment

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

Then it will break src/native/minipal case. Please use cmakedefine01 since this is a shared code.

Copy link
Member Author

@akoeplinger akoeplinger Mar 20, 2022

Choose a reason for hiding this comment

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

Mono has its own config.h.in where we set this, there should be no effect to the minipal.

Copy link
Member Author

Choose a reason for hiding this comment

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

btw. this looks wrong in the existing native/libs code since we use cmakedefine01 for it too:

#if defined(HAVE_STAT_FLAGS) && defined(UF_HIDDEN)

#cmakedefine01 HAVE_STAT_FLAGS

Copy link
Member

@am11 am11 Mar 20, 2022

Choose a reason for hiding this comment

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

Since I added that cmakedefine01 c88c88a, naturally I know what mono is using. :)

Your change is wrong and will suffer from same thing what I have fixed yesterday #66874.

Copy link
Member

Choose a reason for hiding this comment

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

btw. this looks wrong in the existing native/libs code since we use cmakedefine01 for it too:

Yes, we should fix that too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't see why this wouldn't work, would you mind explaining why you think it'd break (on my phone right now so can't play with it)?

Copy link
Member

@am11 am11 Mar 20, 2022

Choose a reason for hiding this comment

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

minipal/getexepath is used by all: coreclr, mono, libs and corehost partitions. When HAVE_GETAUXVAL is detected on a system (which is a case with most modern linux systems), we wanted to use getauxval to get the exe path (and avoid the fallback, procfs, path since procfs is usually not reliable).

So there are three ways we can define a macro in cmake's config file (.in):

// 1. if detected, defines HAVE_GETAUXVAL, else skips the definition
#cmakedefine HAVE_GETAUXVAL

// 2. if detected, defines HAVE_GETAUXVAL as 1, else defines HAVE_GETAUXVAL as 0
#cmakedefine01 HAVE_GETAUXVAL

// 3. if detected defines HAVE_GETAUXVAL as 1, else skips the definition
#define HAVE_GETAUXVAL 1  // or #cmakedefine HAVE_GETAUXVAL 1

Since minipal (and almost everything under src/native) is shared code, we should use the same semantics everywhere. Otherwise, we get Wundef with 3. from minipal that is using #elif HAVE_GETAUXVAL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok so it's more a consistency thing rather than something that wouldn't work, got it. I'll change it.

@akoeplinger akoeplinger merged commit baffb71 into dotnet:main Mar 21, 2022
@akoeplinger akoeplinger deleted the fix-getauxval branch March 21, 2022 15:48
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
We were checking `sts/auxv.h` instead of `sys/auxv.h` so we were never defining `HAVE_SYS_AUXV_H`.

Instead of fixing the typo let's switch to checking for `HAVE_GETAUXVAL` which is what coreclr/libs.native are using.
@ghost ghost locked as resolved and limited conversation to collaborators Apr 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants