Skip to content

is EarlyCSEPass folding icmp of global to null => false correct? #133621

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

Open
workingjubilee opened this issue Mar 30, 2025 · 3 comments
Open

is EarlyCSEPass folding icmp of global to null => false correct? #133621

workingjubilee opened this issue Mar 30, 2025 · 3 comments
Labels
question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead!

Comments

@workingjubilee
Copy link
Contributor

We recently discovered that EarlyCSEPass applies something like the following transformation:

@GLIBC_PRIVATE = external global {}

define noundef zeroext i1 @example_foo_h402e223ff693409d() unnamed_addr {
start:
-  %_0 = icmp eq i64 ptrtoint (ptr @GLIBC_PRIVATE to i64), 0
-  ret i1 %_0
+  ret i1 false
}

Unfortunately, this symbol is actually "linked" at 0x0.

The full(?) LLVMIR with no prepopulated passes, though the externally linked item declaration seems to have gone missing...? It should be as above

source_filename = "example.22da6be311ca0e3f-cgu.0"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define noundef zeroext i1 @_ZN7example3foo17h402e223ff693409dE() unnamed_addr #0 {
  %_0 = icmp eq i64 ptrtoint (ptr @GLIBC_PRIVATE to i64), 0
  ret i1 %_0
}

attributes #0 = { noinline nonlazybind uwtable "probe-stack"="inline-asm" "target-cpu"="x86-64" }

!llvm.module.flags = !{!0, !1}
!llvm.ident = !{!2}

!0 = !{i32 8, !"PIC Level", i32 2}
!1 = !{i32 2, !"RtLibUseGOT", i32 1}
!2 = !{!"rustc version 1.85.0 (4d91de4e4 2025-02-17)"}

As an external global of zero bytes can seemingly be given the address of 0x0 without needing to be initialized, and doing so might make sense, it seems that this folding is incorrect. Or, at least, the LangRef needs a considerable amount of updating to explain this optimization's justification.

@pinskia
Copy link

pinskia commented Mar 30, 2025

I think only weak symbols can be resolved to null pointer and be valid.

@dtcxzyw
Copy link
Member

dtcxzyw commented Mar 30, 2025

@GLIBC_PRIVATE should be marked as extern_weak.
See also https://llvm.org/docs/LangRef.html#linkage-types and

} else if (const GlobalValue *GV = dyn_cast<GlobalValue>(V1)) {
// Now we know that the RHS is a GlobalValue, BlockAddress or simple
// constant.
if (const GlobalValue *GV2 = dyn_cast<GlobalValue>(V2)) {
return areGlobalsPotentiallyEqual(GV, GV2);
} else if (isa<BlockAddress>(V2)) {
return ICmpInst::ICMP_NE; // Globals never equal labels.
} else if (isa<ConstantPointerNull>(V2)) {
// GlobalVals can never be null unless they have external weak linkage.
// We don't try to evaluate aliases here.
// NOTE: We should not be doing this constant folding if null pointer
// is considered valid for the function. But currently there is no way to
// query it from the Constant type.
if (!GV->hasExternalWeakLinkage() && !isa<GlobalAlias>(GV) &&
!NullPointerIsDefined(nullptr /* F */,
GV->getType()->getAddressSpace()))
return ICmpInst::ICMP_UGT;
}

@dtcxzyw dtcxzyw added question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead! and removed new issue labels Mar 30, 2025
@workingjubilee
Copy link
Contributor Author

Hmm. It seems like it should be explicitly noted in the LangRef that declaring something with non-extern_weak linkage, if it may "resolve" to the address 0x0, is UB, even if it is zero-sized. It is reasonable to assume it does but does not seem to follow directly from the other rules about addressable objects and such.

And apparently Alive2 does not believe that this transformation is incorrect, even with extern_weak linkage: https://alive2.llvm.org/ce/z/YX8gMP

I guess I should file that against Alive2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead!
Projects
None yet
Development

No branches or pull requests

4 participants