[PATCH] D156175: [clang][ConstExprEmitter] handle NullToPointer ImplicitCastExpr

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1131-1132
+    case CK_NullToPointer: {
+      if (llvm::Constant *C = Visit(subExpr, destType))
+        if (C->isNullValue())
+          return CGM.EmitNullConstant(destType);
----------------
nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > FWIW, I would have thought these might be unnecessary, but we do trip 2 tests without these guards IIRC.
> > The fact that the Visit can fail doesn't surprise me; you can write arbitrary expressions of type nullptr_t (for example, `nullptr_t f(); void* g = f();`.  I can't think of any reason you'd need to check C->isNullValue(), though; do you have a testcase for that?
> Failed Tests (2):
>   Clang :: CodeGenCXX/const-init-cxx11.cpp
>   Clang :: CodeGenCXX/cxx11-thread-local-instantiated.cpp
> 
> Looking at the first:
> 
> ```
> decltype(nullptr) null();
> int *p = null();
> ```
> and the corresponding AST:
> ```
> |-FunctionDecl 0x5650ebbb44a8 <x.cpp:2:3, col:26> col:21 used null 'decltype(nullptr) ()'
> `-VarDecl 0x5650ebbb45e0 <line:3:3, col:17> col:8 p 'int *' cinit
>   `-ImplicitCastExpr 0x5650ebbb4740 <col:12, col:17> 'int *' <NullToPointer>
>     `-CallExpr 0x5650ebbb4720 <col:12, col:17> 'decltype(nullptr)':'std::nullptr_t'
>       `-ImplicitCastExpr 0x5650ebbb4708 <col:12> 'decltype(nullptr) (*)()' <FunctionToPointerDecay>
>         `-DeclRefExpr 0x5650ebbb4690 <col:12> 'decltype(nullptr) ()' lvalue Function 0x5650ebbb44a8 'null' 'decltype(nullptr) ()'
> ```
> 
> 
> we change the existing test case from having a `__cxx_global_var_init` routine, i.e.:
> ```
> @p = dso_local global ptr null, align 8
> @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__sub_I_x.cpp, ptr null }]
> 
> ; Function Attrs: noinline uwtable
> define internal void @__cxx_global_var_init() #0 section ".text.startup" {
> entry:
>   %call = call ptr @_Z4nullv()
>   store ptr null, ptr @p, align 8
>   ret void
> }
> 
> declare ptr @_Z4nullv() #1
> 
> ; Function Attrs: noinline uwtable
> define internal void @_GLOBAL__sub_I_x.cpp() #0 section ".text.startup" {
> entry:
>   call void @__cxx_global_var_init()
>   ret void
> }
> ```
> to simply:
> ```
> @p = dso_local global ptr null, align 8
> ```
> So it seems nice to skip having the static constructors, but then `@_Z4nullv` is never invoked.  That seems problematic? (I expect the fast path to fail once it recurses down to the DeclRefExpr, but that requires the subexpr of the ImplicitCastExpr be visited).
> 
> ---
> 
> So I think the code as written is good to go. WDYT?
I think you need to check that Visit() succeeds (returns a non-null Constant*), but like I said before, I can't see why you need to check `C->isNullValue()`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156175/new/

https://reviews.llvm.org/D156175