[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

Mon Apr 26 13:22:41 PDT 2021

dblaikie added inline comments.

================
Comment at: clang/lib/CodeGen/CGExpr.cpp:2843-2844
+      CGDebugInfo *DI = CGM.getModuleDebugInfo();
+      auto *Fn = dyn_cast<llvm::Function>(LV.getPointer(*this));
+      if (DI && Fn && !Fn->getSubprogram())
+        DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);
----------------
yonghong-song wrote:
> dblaikie wrote:
> > It looks like there isn't any test coverage for the case where Fn is null here (I added an assertion that Fn is non-null and it didn't fire when running check-clang) - please add some to this patch. (I'd like to then look at those cases to better understand when they come up and whether there's a different/better way to phrase this code to handle those cases)
> Sorry. It is my fault. Tested with a slight different test case than this one in my actual test setup. Now the new test should reflect null Fn. Thanks!
Thanks for the updated test case!

I think for generality, either this code shouldn't use the `LV` value - instead calling `GetOrCreateLLVMFunction` (though that's probably not great - since that would involve things like going through ConvertType and other shenanigans in `GetAddrOfFunction` - I guess if pieces of `GetAddrToFunction`, `EmitFunctionDeclPointer`, and `EmitFunctionDeclPointer` were split up maybe it could be done - or changet he return value to include the raw llvm::Function result in addition to the LValue (& keep the existing function name returning just the LValue for other callers)) - or doing more work to unwrap the Constant - maybe it would be enough to use "stripPointerCasts" & that'd get you something you can cast to llvm::Function?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567