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

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

Merged
merged 4 commits into from
Mar 21, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5d16bc8
[mono] Fix typo in detecting sys/auxv.h
akoeplinger Mar 20, 2022
97396ae
Use cmakedefine instead of cmakedefine01
akoeplinger Mar 20, 2022
58cf3fe
Revert "Use cmakedefine instead of cmakedefine01"
akoeplinger Mar 20, 2022
c04e81b
Don't use defined()
akoeplinger Mar 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/mono/cmake/config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,9 +351,6 @@
/* Define to 1 if you have the <execinfo.h> header file. */
#cmakedefine HAVE_EXECINFO_H 1

/* Define to 1 if you have the <sys/auxv.h> header file. */
#cmakedefine HAVE_SYS_AUXV_H 1

/* Define to 1 if you have the <sys/resource.h> header file. */
#cmakedefine HAVE_SYS_RESOURCE_H 1

Expand Down
2 changes: 1 addition & 1 deletion src/mono/cmake/configure.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ endfunction()

ac_check_headers (
sys/types.h sys/stat.h sys/filio.h sys/sockio.h sys/utime.h sys/un.h sys/syscall.h sys/uio.h sys/param.h
sys/prctl.h sys/socket.h sys/utsname.h sys/select.h sys/poll.h sys/wait.h sts/auxv.h sys/resource.h
sys/prctl.h sys/socket.h sys/utsname.h sys/select.h sys/poll.h sys/wait.h sys/resource.h
sys/ioctl.h sys/errno.h sys/sendfile.h sys/statvfs.h sys/statfs.h sys/mman.h sys/mount.h sys/time.h sys/random.h
strings.h stdint.h unistd.h signal.h setjmp.h syslog.h netdb.h utime.h semaphore.h alloca.h ucontext.h pwd.h elf.h
gnu/lib-names.h netinet/tcp.h netinet/in.h link.h arpa/inet.h unwind.h poll.h wchar.h linux/magic.h
Expand Down
4 changes: 2 additions & 2 deletions src/mono/mono/utils/mono-hwcap-arm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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:

runtime/src/native/libs/System.Native/pal_io.c

Line 214 in fa1f283

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

runtime/src/native/libs/Common/pal_config.h.in

Line 25 in fa1f283

#cmakedefine01 HAVE_STAT_FLAGS

Copy link
Member

@am11 am11 Mar 20, 2022 •
edited
Loading

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 •
edited
Loading

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.

#include <sys/auxv.h>
#elif defined(__APPLE__)
#include <mach/machine.h>
Expand All @@ -38,7 +38,7 @@
void
mono_hwcap_arch_init (void)
{
#if defined(HAVE_SYS_AUXV_H) && !defined(HOST_ANDROID)
#if defined(HAVE_GETAUXVAL) && !defined(HOST_ANDROID)
unsigned long hwcap;
unsigned long platform;

Expand Down
4 changes: 2 additions & 2 deletions src/mono/mono/utils/mono-hwcap-ppc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

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

#if defined(__linux__) && defined(HAVE_SYS_AUXV_H)
#if defined(__linux__) && defined(HAVE_GETAUXVAL)
#include <string.h>
#include <sys/auxv.h>
#elif defined(_AIX)
Expand All @@ -46,7 +46,7 @@
void
mono_hwcap_arch_init (void)
{
#if defined(__linux__) && defined(HAVE_SYS_AUXV_H)
#if defined(__linux__) && defined(HAVE_GETAUXVAL)
unsigned long hwcap;
unsigned long platform;

Expand Down