`__builtin_constant_p` incorrect with undefined variables

[kees]

Given this situation:

// -O2 -Wall
extern void knowable(void);
extern void unknowable(void);

void test(void)
{
    char buf[8];

    // The end of "buf" is undefined. That shouldn't make
    // __builtin_constant_p() undefined nor true, though.
    // It should just be false.
    if (__builtin_constant_p(buf[sizeof(buf)-1])) {
        knowable();
    } else {
    	unknowable();
    }
}

Clang is incorrectly resolving __builtin_constant_p() to true (or undefined) instead of false. GCC correctly resolves it to false (and emits a warning about using buf uninitialized).

Isolated from ClangBuiltLinux: ClangBuiltLinux/linux#2073

Providing an initializer to buf makes this go away (in the sense that now it is correctly "knowable"). GCC flips over to knowable():

    char buf[8] = "";

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Kees Cook (kees)

Given this situation:

// -O2 -Wall
extern void knowable(void);
extern void unknowable(void);

void test(void)
{
    char buf[8];

    // The end of "buf" is undefined. That shouldn't make
    // __builtin_constant_p() undefined nor true, though.
    // It should just be false.
    if (__builtin_constant_p(buf[sizeof(buf)-1])) {
        knowable();
    } else {
    	unknowable();
    }
}

Clang is incorrectly resolving __builtin_constant_p() to true (or undefined) instead of false. GCC correctly resolves it to false (and emits a warning about using buf uninitialized).

Isolated from ClangBuiltLinux: ClangBuiltLinux/linux#2073

Providing an initializer to buf makes this go away (in the sense that now it is correctly "knowable"). GCC flips over to knowable():

    char buf[8] = "";

[efriedma-quic]

Even if we did change constant folding to fold llvm.is.constant.i32(i32 undef) to fold to false, I don't think that reliably helps you. We don't, in general, preserve the undef-ness of a value in optimizations, so any optimization could flip it to true. Given that, I don't see any way to reliably fix this.

Is there any way you can write your code so it doesn't involve reading uninitialized memory?

[kees]

Well, the code isn't trying to directly read a value, it's trying to check the "known to be a constant value" state of the variable so it can guide the code-gen (generally via DCE) toward either a compile-time constant codegen vs needing runtime calculation codegen. And I don't think it's too unreasonable that undef is analyzed here given that __buitin_constant_p() doesn't allow "side effects". I would count "reading the value" to be a side effect here (e.g. the value is not accessible nor used by __builtin_constant_p() nor its caller).

So I guess I'm saying, yes, let's do that constant folding since the value isn't needed (i.e. undef-ness preservation doesn't seem like something that would be a problem here, but perhaps I lack imagination).

[kees]

Hm, is it really as simple as this?

diff --git a/llvm/lib/IR/Constants.cpp b/llvm/lib/IR/Constants.cpp
index 9e3e739fae3d..36f413645761 100644
--- a/llvm/lib/IR/Constants.cpp
+++ b/llvm/lib/IR/Constants.cpp
@@ -841,6 +841,8 @@ Constant *Constant::mergeUndefsWith(Constant *C, Constant *Other) {
 }
 
 bool Constant::isManifestConstant() const {
+  if (isa<UndefValue>(this))
+    return false;
   if (isa<ConstantData>(this))
     return true;
   if (isa<ConstantAggregate>(this) || isa<ConstantExpr>(this)) {

A simple test works ... Attempting a full kernel build now.

[kees]

Yes, that fixes the ClangBuiltLinux bug and everything else appears to continue working correctly. I will write some tests and get a PR posted. Thank you for the hints for what the name to track down was! (llvm.is.constant led me ultimately to Constant::isManifestConstant())