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

Pointer template parameters have the wrong value in some situations #57883

Closed
Supermario1313 opened this issue Sep 21, 2022 · 9 comments
Closed
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer

Comments

@Supermario1313
Copy link

I'm using clang 14.0.6-2 which I've installed with pacman on Arch Linux.

I'm currently trying to cross-compile LunaLua on Linux with clang. This project makes extensive use of casts from integers to pointers in constexpr contexts in order to reference static variables from smbx.exe. Such casts are allowed in Visual C++ 2015 but not in clang. To overcome this restrictions, I used the following workarounds:

  1. Uses the __builtin_constant_p GCC builtin:
#include <cstdio>

template<typename To, typename From>
inline constexpr To constexpr_reinterpret_cast(From from) {
    return __builtin_constant_p( reinterpret_cast<To>(from) ) ? reinterpret_cast<To>(from) : reinterpret_cast<To>(from);
}

constexpr int* test = constexpr_reinterpret_cast<int*>(42UL);

template <int* i>
static void someFunc() {
    std::printf("In templated function: %p\n", i);
}

int main(int argc, char* argv[]) {
    std::printf("In main function: %p\n", test);
    someFunc<test>();
}
  1. Uses a reference to a dereferenced casted unsigned long literal:
#include <cstdio>

static int& test = *(int*)42UL;

template <int* i>
static void someFunc() {
    std::printf("In templated function: %p\n", i);
}

int main(int argc, char* argv[]) {
    std::printf("In main function: %p\n", &test);
    someFunc<&test>();
}

Both codes compile successfully and the compiler doesn't raise warnings when using -fsanitize=undefined,address. Furthermore, test or &test have the correct value when passed to a function or assigned to a variable. The problem arises when passing test or &test as a template parameter. In that case, they inexplicably take the value nullptr. Here's the output of both programs when compiled and executed:

In main function: 0x2a
In templated function: (nil)
@EugeneZelenko
Copy link
Contributor

Could you please try 15 or main branch? https://godbolt.org should be helpful.

@Supermario1313
Copy link
Author

I get the same result for both programs with clang 15.

@Supermario1313
Copy link
Author

Compiling with clang trunk causes the following compilation error (for the first program, the only different things for the second program are line numbers and test being replaced by &test):

<source>:17:5: error: no matching function for call to 'someFunc'
    someFunc<test>();
    ^~~~~~~~~~~~~~
<source>:11:13: note: candidate template ignored: invalid explicitly-specified argument for template parameter 'i'
static void someFunc() {
            ^
1 error generated.
ASM generation compiler returned: 1
<source>:17:5: error: no matching function for call to 'someFunc'
    someFunc<test>();
    ^~~~~~~~~~~~~~
<source>:11:13: note: candidate template ignored: invalid explicitly-specified argument for template parameter 'i'
static void someFunc() {
            ^
1 error generated.
Execution build compiler returned: 1

Which is weird since the template argument is a compile-time constant.

@efriedma-quic
Copy link
Collaborator

reinterpret_cast<int*>(42UL) isn't a constant expression according to C++ rules, so clang correctly rejects all the variants with -std=c++17 or later (and so does gcc).

Not sure why the compiler accepts the code with -std=c++14 or earlier. Probably the check for a null template argument is wrong somehow.

@Supermario1313
Copy link
Author

Supermario1313 commented Sep 22, 2022

reinterpret_cast not being a constant expression by standard C++ rules is the reason why I was using the __builtin_constant_p hack in the first place. I assumed that it would've been enough to make the compiler consider it as a constant expression since using it with something that isn't known at compile-time (like the address of a local variable) causes a compilation error:

hello-world-ptr.cpp:19:20: error: constexpr variable 'test2' must be initialized by a constant expression
    constexpr int* test2 = __builtin_constant_p( reinterpret_cast<int*>(&n) ) ? reinterpret_cast<int*>(&n) : reinterpret_cast<int*>(&n);
                   ^       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hello-world-ptr.cpp:19:20: note: pointer to 'n' is not a constant expression
hello-world-ptr.cpp:16:9: note: declared here
    int n;

My point is that the program wouldn't have compiled if the initializer of test wasn't a compile-time constant. Furthermore, the disassembly of the main function of the first program looks like this:

0000000000001140 <main>:
    1140:       55                      push   rbp
    1141:       48 89 e5                mov    rbp,rsp
    1144:       48 83 ec 10             sub    rsp,0x10
    1148:       89 7d fc                mov    DWORD PTR [rbp-0x4],edi
    114b:       48 89 75 f0             mov    QWORD PTR [rbp-0x10],rsi
    114f:       48 8d 3d ae 0e 00 00    lea    rdi,[rip+0xeae]        # 2004 <_IO_stdin_used+0x4>
    1156:       be 2a 00 00 00          mov    esi,0x2a
    115b:       b0 00                   mov    al,0x0
    115d:       e8 ce fe ff ff          call   1030 <printf@plt>
    1162:       e8 09 00 00 00          call   1170 <_ZL8someFuncILPi0EEvv>
    1167:       31 c0                   xor    eax,eax
    1169:       48 83 c4 10             add    rsp,0x10
    116d:       5d                      pop    rbp
    116e:       c3                      ret
    116f:       90                      nop

The mov esi, 0x2a instruction at offset 1156 shows that test was successfully interpreted as a compile-time constant by the compiler.

@Supermario1313
Copy link
Author

Supermario1313 commented Sep 22, 2022

I've done additional tests with clang trunk and test acts like a constant expression in any other context. For example, the following code works as intended:

#include <cstdio>

template<typename To, typename From>
inline constexpr To constexpr_reinterpret_cast(From from) {
    return __builtin_constant_p( reinterpret_cast<To>(from) ) ? reinterpret_cast<To>(from) : reinterpret_cast<To>(from);
}

constexpr int* test = constexpr_reinterpret_cast<int*>(42UL);
constexpr int* test2(int* param) {
    return param + 1;
}
constexpr int* test3 = test2(test);

int main(int argc, char* argv[]) {
    std::printf("In main function: %p\n", test3);
}

Output:

In main function: 0x2e

Likewise, the assembly code for this program contains mov esi, 46. The problem only happens with template arguments.

@efriedma-quic
Copy link
Collaborator

Oh, right, __builtin_constant_p lets you violate the normal constexpr rules a bit. Template arguments in particular have extra restrictions according to the standard, though; __builtin_constant_p can't be used to get around those.

We have to ensure standard-compliant code works correctly, and we print the correct diagnostics for invalid code, before we can consider any non-standard extensions. (I'm not sure a non-standard extension is really warranted here; it's a bit obscure.)

@Supermario1313
Copy link
Author

Thank you for your answer. In that case, I'll try to program a compiler plugin to support this (or just replace pointer template arguments with std::uintptr_t if that's not possible or too complicated). Side question though, do you think that requesting this feature for clang-cl would be warranted for compatibility with Visual C++ 2015?

@efriedma-quic
Copy link
Collaborator

Posted https://reviews.llvm.org/D134928 .

Side question though, do you think that requesting this feature for clang-cl would be warranted for compatibility with Visual C++ 2015?

We could consider it. We mostly weigh this sort of thing by practical compatibility impact (and to some extent, how difficult it is to implement). Please file a new bug report for this; please describe as much as you can how widespread the breakage is, and how hard it is to fix the code to avoid this construct. (Let's keep this bug report just about the miscompile.)

@EugeneZelenko EugeneZelenko added clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer and removed new issue labels Oct 19, 2022
sid8123 pushed a commit to sid8123/llvm-project that referenced this issue Oct 25, 2022
The way this code checks whether a pointer is null is wrong for other
reasons; it doesn't actually check whether a null pointer constant is a
"constant" in the C++ standard sense.  But this fix at least makes sure
we don't treat a non-null pointer as if it were null.

Fixes llvm#57883

Differential Revision: https://reviews.llvm.org/D134928
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer
Projects
None yet
Development

No branches or pull requests

3 participants