#undef fails to undefine commandline preprocessor directives

[ryao]

In OpenZFS, we define preprocessor directives to provide API depreciation warnings. These are set in an automake file like this:

AM_CPPFLAGS_NOCHECK += -D"strncpy(...)=__attribute__((deprecated(\"strncpy(3) is deprecated. Use strlcpy(3) instead!\"))) strncpy(__VA_ARGS__)"

That is actually the latest (proposed) deprecation warning. Due to portability issues, we initially wanted to provide an exception to the proposed deprecation policy in one of our test cases by doing #undef. This worked on GCC, but failed to work with Clang, where the #undef was ignored.

I made a minimal test case based on the depreciation warning:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#undef strncpy

int main()
{
        char buf[256];
        char hello[] = "hello world\n";

        strncpy(buf, hello, sizeof(buf));

        printf("%s", buf);

        return 0;
}

gcc -D"strncpy(...)=__attribute__((deprecated(\"strncpy(3) is deprecated. Use strlcpy(3) instead\!\"))) strncpy(__VA_ARGS__)" hello.c

works fine.

clang -D"strncpy(...)=__attribute__((deprecated(\"strncpy(3) is deprecated. Use strlcpy(3) instead\!\"))) strncpy(__VA_ARGS__)" hello.c

breaks.

This was discovered in openzfs/zfs#13876. We have since changed the code to avoid the need to bypass the deprecation warning, but I am filing this because I feel it is a bug.

This issue was detected by the FreeBSD system clang on FreeBSD stable/13 amd64 in our continuous integration infrastructure. It was reproduced on my local Gentoo Linux machine that uses Clang version 14.0.6.

[kiwixz]

Clang is working as expected here, you need to move the undef above function declaration (the <string.h> include) to hide the warning.

[ryao]

Why does the CPP understand C function declarations? I thought that the CPP should not have any concept of those.

[AaronBallman]

Why does the CPP understand C function declarations? I thought that the CPP should not have any concept of those.

It doesn't.

The reason Clang behaves the way it does is because you're redefining the macro before including the header, which means the declaration you get out of the header is:

extern char *__attribute__((deprecated("strncpy(3) is deprecated. Use strlcpy(3) instead!"))) strncpy(char *__restrict __dest, const char *__restrict __src, size_t __n) __attribute__ ((__nothrow__ )) __attribute__ ((__nonnull__ (1, 2)));

Undefining the macro after including the header doesn't help because the declaration has still been changed. So from the compiler's perspective, the function is still marked as being deprecated (though I'm shocked that this code compiles given how deeply weird that declaration ends up being).

It's worth noting that what you're doing is undefined behavior (see C2x 6.2.4p8 and 7.1.3p1). Your code could run into problems for C standard libraries which define their function interfaces as macros. The #undef isn't an issue (you're explicitly allowed to undefine library interfaces to get to an actual function), but the #define is.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[ryao]

It's worth noting that what you're doing is undefined behavior (see C2x 6.2.4p8 and 7.1.3p1).

It was not my idea. My idea was to use a perl script to catch it. Other people had the idea when I was not looking, put it into place and wanted me to use it too.

That said, while I do not have access to the standard, the standard seems to be overzealous in declaring things to be undefined. It declares making your own headers with the same names as system headers as undefined too, but the people building the system need to do that to make the system library headers in the first place (and we have #include_next to handle the various needs associated with that).

Your code could run into problems for C standard libraries which define their function interfaces as macros.

In that case, wouldn't there be a compiler error from the macro definitions colliding? I assume -Werror is in use.

The original goal is to make builds fail if someone tries to write a patch using strncpy() because it is prone to being used incorrectly. As far as I can tell, this will achieve that.

Undefining the macro after including the header doesn't help because the declaration has still been changed.

I am now confused how GCC does not have a build failure here. :/

That said, I have since switched the code to use snprintf() instead of strncpy(), so the software builds without a problem. As long as no one uses strncpy(), it should continue to build.

[AaronBallman]

That said, while I do not have access to the standard,

FWIW, you can get the latest draft at: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf (and this draft should be rather close to what is finally published). You can find other drafts at: https://www.open-std.org/jtc1/sc22/wg14/www/projects

the standard seems to be overzealous in declaring things to be undefined.

This is not one such case IMO. Presuming you can reach into the C standard library and change its declarations is a pretty dangerous thing to expect to work in general.

In that case, wouldn't there be a compiler error from the macro definitions colliding? I assume -Werror is in use.

If you have -Wmacro-redefined enabled (which it is by default), then yes.

The original goal is to make builds fail if someone tries to write a patch using strncpy() because it is prone to being used incorrectly. As far as I can tell, this will achieve that.

It does something useful for you in GCC and doesn't work quite as well as you'd want in Clang; who knows what it will do with any other C compiler (because it's UB) or another C standard library (like musl).

Another approach you could take that would work the way you want in Clang (and isn't UB) is to redeclare the C Standard Library function but add the attribute to it: https://godbolt.org/z/rW797sfTK

I am now confused how GCC does not have a build failure here. :/

FWIW, I was also pretty surprised by the GCC behavior. Passing -E shows that they do munge the declaration in string.h the same way Clang is doing.

[ryao]

FWIW, you can get the latest draft at: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf (and this draft should be rather close to what is finally published). You can find other drafts at: https://www.open-std.org/jtc1/sc22/wg14/www/projects

I do not see how 6.2.4p8 applies. That refers to runtime modification of temporary data structures. What is happening in the example code is build time modification of a function definition to ensure that runtime is impossible if it is used. Since runtime is impossible, this does not apply.

I also do not see a conflict with 7.1.3p1 since we are not attempting to use it to mean something else at runtime but instead want it to never be used period.

This is not one such case IMO. Presuming you can reach into the C standard library and change its declarations is a pretty dangerous thing to expect to work in general.

Not if the purpose is to cause a build failure.

That said, this is not a case where the standard claims undefined behavior.

It does something useful for you in GCC and doesn't work quite as well as you'd want in Clang; who knows what it will do with any other C compiler (because it's UB) or another C standard library (like musl).

This works fine with musl. It also works fine with FreeBSD’s libc and MacOS’ libc. At the moment, we only test GCC and Clang. We already rely on a number of things that would be unlikely to work with other compilers, but there is no desire to use other compilers at the moment. However, I am not convinced this particular case would not work with other compilers.

That said, undefined behavior refers to runtime behavior, not compile time behavior. The way that this is designed is intended to ensure that runtime never happens when that function is used and thus it is not undefined behavior.

[ryao]

I am closing this as there is nothing left to do here.