[PATCH] D68822: [WIP][BPF] Support external globals

[Yonghong Song via Phabricator via llvm-commits]

yonghong-song marked 3 inline comments as done.
yonghong-song added inline comments.


================
Comment at: llvm/lib/Target/BPF/BTF.h:183-184
   VAR_GLOBAL_ALLOCATED = 1, ///< Linkage: ExternalLinkage
-  VAR_GLOBAL_TENTATIVE = 2, ///< Linkage: CommonLinkage
-  VAR_GLOBAL_EXTERNAL = 3,  ///< Linkage: ExternalLinkage
+  VAR_GLOBAL_EXTERNAL = 2,  ///< Linkage: ExternalLinkage
+  VAR_GLOBAL_TENTATIVE = 3, ///< Linkage: CommonLinkage
 };
----------------
anakryiko wrote:
> what's the difference between EXTERNAL and TENTATIVE?
TENTATIVE is for .common symbols. Just do "int g;" and compile, you will find out.


================
Comment at: llvm/lib/Target/BPF/BTFDebug.cpp:1064
+    // Whether DataSec is readonly or not can be found from the
+    // corresponding ELF section flags.
 
----------------
anakryiko wrote:
> I remember there were problems figuring out datasec size when emitting BTF. Is this still the case? Does the same problem prevent readonly flag in DATASEC or you are trying to not put redundant information into BTF that can be derived from ELF?
Yes this is still the case. The debugging info is emitted earlier and we do not know the data section until very end of compilation which is too late. 

The section readonly thing is marked by ELF object writer. The BPF backend can only detect readonly variables through type inspection, so it MAY mark the container DataSec as readonly section. Let us do it now with ELF checking since the ELF section inspection is required to get the readonly data.


================
Comment at: llvm/lib/Target/BPF/BTFDebug.cpp:1112
 
+    if (SecName.empty())
+      continue;
----------------
anakryiko wrote:
> we'll just ignore externs with GlobalValue::CommonLinkage, right? Should we just assign a special section name to them instead? feels bad to only have a partial list of externs.
No, all externs will emitted with their special section name.
CommonLinkage is not an extern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68822