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

prov/verbs: Replace __BITS_PER_LONG with LONG_WIDTH #10223

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

raffenet
Copy link
Contributor

@raffenet raffenet commented Jul 26, 2024

The checks for the verbs provider can pass on a FreeBSD system, but will fail to compile because asm/types.h is not available.

Updated description: 2024-08-01
The verbs provider uses asm/types.h for __BITS_PER_LONG, but that header
is limited to Linux systems. LONG_WIDTH is defined in C23 in
limits.h. Use it instead, and add logic to define it if not
available. This will allow the verbs provider to compile on FreeBSD
systems.

@j-xiong
Copy link
Contributor

j-xiong commented Jul 26, 2024

The header is included in prov/verbs/src/ofi_verbs.h. Can verbs be built on BSD by commenting out the line? If so, a better fix would be using a conditional to guard the #include line.

@raffenet
Copy link
Contributor Author

It does compile if I comment out that header, but gives this warning about a missing define.

--- prov/verbs/src/src_libfabric_la-verbs_ep.lo ---
prov/verbs/src/verbs_ep.c: In function 'vrb_query_atomic':
prov/verbs/src/verbs_ep.c:2002:5: warning: "__BITS_PER_LONG" is not defined, evaluates to 0 [-Wundef]
 2002 | #if __BITS_PER_LONG == 64

@j-xiong
Copy link
Contributor

j-xiong commented Jul 29, 2024

Something like this should work:

#if HAVE_ASM_TYPES_H
#include <asm/types.h>
#else
#if defined(__x86_64__) && !defined(__ILP32__)
#define __BITS_PER_LONG 64
#else
#define __BITS_PER_LONG 32
#endif
#endif

where HAVE_ASM_TYPES_H needs to be defined by configure.

@raffenet
Copy link
Contributor Author

Something like this should work:

#if HAVE_ASM_TYPES_H
#include <asm/types.h>
#else
#if defined(__x86_64__) && !defined(__ILP32__)
#define __BITS_PER_LONG 64
#else
#define __BITS_PER_LONG 32
#endif
#endif

where HAVE_ASM_TYPES_H needs to be defined by configure.

OK. I will work on it later this week.

@raffenet
Copy link
Contributor Author

OK. I will work on it later this week.

BTW, it looks like C23 adds LONG_WIDTH in limits.h which we could also test for before using the fallback code? See https://en.cppreference.com/w/c/types/limits

@j-xiong
Copy link
Contributor

j-xiong commented Jul 29, 2024

@raffenet I think that's something worth experimenting with (looks like it's already supported by gcc). Just need to keep the logic simple.

@raffenet
Copy link
Contributor Author

@raffenet I think that's something worth experimenting with (looks like it's already supported by gcc). Just need to keep the logic simple.

What do you think about this? Looks like LONG_BIT is a non-standard define and LONG_WIDTH is the new standard. I also added support for __aarch64__ so that it sets the right value on my laptop. It's kind of verbose, so maybe it belongs in an osd.h header rather than the provider?

#include <limits.h>
#if !defined(LONG_WIDTH)
#ifdef LONG_BIT
#define LONG_WIDTH LONG_BIT
#elif defined(HAVE_ASM_TYPES_H)
#include <asm/types.h>
#define LONG_WIDTH __BITS_PER_LONG
#elif defined(__x86_64__) || defined(__aarch64__)
#ifdef !defined(__ILP32__)
#define LONG_WIDTH 64
#else
#define LONG_WIDTH 32
#endif
#endif
#endif

@j-xiong
Copy link
Contributor

j-xiong commented Jul 31, 2024

@raffenet Thanks! This looks good to me. Let's keep it in the verbs provider for now since it's only used here.

The verbs provider uses asm/types.h for __BITS_PER_LONG, but that header
is limited to Linux systems. LONG_WIDTH is defined in C23 in
limits.h. Use it instead, and add logic to define it if not
available. This will allow the verbs provider to compile on FreeBSD
systems.

Signed-off-by: Ken Raffenetti <[email protected]>
@raffenet raffenet changed the title prov/verbs: Check for required header prov/verbs: Replace __BITS_PER_LONG with LONG_WIDTH Aug 1, 2024
@raffenet
Copy link
Contributor Author

raffenet commented Aug 6, 2024

Is the AWS CI failure real or no?

@wenduwan
Copy link
Contributor

wenduwan commented Aug 6, 2024

@raffenet Likely noise. I'm restarting it.

@wenduwan
Copy link
Contributor

wenduwan commented Aug 6, 2024

bot:aws:retest

@j-xiong j-xiong merged commit c459970 into ofiwg:main Aug 6, 2024
13 checks passed
raffenet added a commit to raffenet/mpich that referenced this pull request Aug 7, 2024
Include ofiwg/libfabric#10223. Fixes verbs
provider compilation on FreeBSD systems.
@raffenet raffenet deleted the verbs-config branch August 7, 2024 18:51
raffenet added a commit to raffenet/mpich that referenced this pull request Aug 16, 2024
Include ofiwg/libfabric#10223. Fixes verbs
provider compilation on FreeBSD systems.
raffenet added a commit to raffenet/mpich that referenced this pull request Aug 16, 2024
Include ofiwg/libfabric#10223. Fixes verbs
provider compilation on FreeBSD systems.
raffenet added a commit to raffenet/mpich that referenced this pull request Aug 20, 2024
Include ofiwg/libfabric#10223. Fixes verbs
provider compilation on FreeBSD systems.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants