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

a persistent bug caused by name conflicts between barrier() macro and function name #5292

Closed
Transpeptidase opened this issue Sep 1, 2021 · 3 comments · Fixed by #5293
Closed
Labels
Type: Bug A previously unknown bug in PMDK

Comments

@Transpeptidase
Copy link

ISSUE:

Environment Information

  • PMDK package version(s): PMDK Version 1.11.0
  • OS(es) version(s): N/A
  • ndctl version(s): N/A
  • kernel version(s): N/A
  • compiler, libraries, packaging and other related tools version(s): N/A

Please provide a reproduction of the bug:

static force_inline void
memmove_movnt_avx512f(char *dest, const char *src, size_t len, flush_fn flush,
barrier_fn barrier)
{
if ((uintptr_t)dest - (uintptr_t)src >= len)
memmove_movnt_avx512f_fw(dest, src, len, flush);
else
memmove_movnt_avx512f_bw(dest, src, len, flush);
barrier();
VALGRIND_DO_FLUSH(dest, len);
}

In this function, the function pointer called barrier is overridden by macro barrier(), which causes function memmove_movnt_avx512f_clflush not issuing sfence instructions.

pmdk/src/core/util.h

Lines 134 to 142 in 46d4fc9

#ifdef _MSC_VER
#define force_inline inline __forceinline
#define NORETURN __declspec(noreturn)
#define barrier() _ReadWriteBarrier()
#else
#define force_inline __attribute__((always_inline)) inline
#define NORETURN __attribute__((noreturn))
#define barrier() asm volatile("" ::: "memory")
#endif

How often bug is revealed: (always, often, rare):

Actual behavior:

Use the barrier macro

Expected behavior:

Call the barrier function pointer

Details

Additional information about Priority and Help Requested:

Are you willing to submit a pull request with a proposed change? (Yes, No) Yes

Requested priority: (Showstopper, High, Medium, Low) High

@Transpeptidase Transpeptidase added the Type: Bug A previously unknown bug in PMDK label Sep 1, 2021
pbalcer added a commit to pbalcer/nvml that referenced this issue Sep 1, 2021
The implementation of hardware fencing for non-temporal
memcpy variants is done using a function pointer. Some
of those pointers were called "barrier" which unfortunately
overlaps with a function-like macro that's used for compiler
barriers. This meant that a compiler barrier was being used
instead of a hardware store barrier.

This patch changes the compiler barrier to a static inline
function called "compiler_barrier" to avoid name conflicts.

Fixes pmem#5292
Reported-by: @Transpeptidase
@pbalcer
Copy link
Member

pbalcer commented Sep 1, 2021

Thank you for finding this. Such an unfortunate thing to overlook... Macros are evil after all ;)

If you don't mind me asking, how did you find the problem? Just by analyzing the code or through the use of some tool?

pbalcer added a commit to pbalcer/nvml that referenced this issue Sep 1, 2021
The implementation of hardware fencing for non-temporal
memcpy variants is done using a function pointer. Some
of those pointers are called "barrier" which unfortunately
overlaps with a function-like macro that's used for compiler
barriers. This meant that a compiler barrier was being used
instead of a hardware store barrier.

This patch changes the compiler barrier to a static inline
function called "compiler_barrier" to avoid name conflicts.

Fixes pmem#5292
Reported-by: @Transpeptidase
@Transpeptidase
Copy link
Author

I just used memmove_movnt_avx512f_clwb to test PM bandwidth, but found no matter how granular the write is (64B or 128B),the single-core bandwidth is the same and is high :)

@pbalcer
Copy link
Member

pbalcer commented Sep 1, 2021

That makes sense. Thanks.

pbalcer added a commit to pbalcer/nvml that referenced this issue Sep 1, 2021
The implementation of hardware fencing for non-temporal
memcpy variants is done using a function pointer. Some
of those pointers are called "barrier" which unfortunately
overlaps with a function-like macro that's used for compiler
barriers. This meant that a compiler barrier was being used
instead of a hardware store barrier.

This patch changes the compiler barrier to a static inline
function called "compiler_barrier" to avoid name conflicts.

Fixes pmem#5292
Reported-by: @Transpeptidase
@pbalcer pbalcer closed this as completed in 55ec1b2 Sep 2, 2021
kilobyte pushed a commit to kilobyte/pmdk that referenced this issue Oct 9, 2021
The implementation of hardware fencing for non-temporal
memcpy variants is done using a function pointer. Some
of those pointers are called "barrier" which unfortunately
overlaps with a function-like macro that's used for compiler
barriers. This meant that a compiler barrier was being used
instead of a hardware store barrier.

This patch changes the compiler barrier to a static inline
function called "compiler_barrier" to avoid name conflicts.

Fixes pmem#5292
Reported-by: @Transpeptidase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug A previously unknown bug in PMDK
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants