Page MenuHomePhabricator

[BPF] Support external globals
AbandonedPublic

Authored by yonghong-song on Oct 10 2019, 11:45 AM.

Details

Reviewers
ast
anakryiko
Summary

[BPF] Support external globals

Previously, only types for external variables and globals without
section names are omitted.
This patch emitted types for all non-common-linkage globals.
From https://llvm.org/docs/LangRef.html,

common
  “common” linkage is most similar to “weak” linkage,
  but they are used for tentative definitions in C,
  such as “int X;” at global scope. ...

It is not clear what are "common"/"weak" semantics for bpf programs,
so omit them for now.

For non-extern global variables without section names,

  • a BTF_KIND_VAR will be generated.
  • the BTF_KIND_VAR will be put into .bss/.data DataSec based on whether they are zero initialized or not.

For extern globals:

  • A BTF_KIND_VAR will be generated.
  • if with section name, a DataSec with that section name will be created containing that global variable.
  • external globals do not have types in BTF_KIND_VAR since clang does not generate debug info type for external variables.
  • the in-section offset for extern variables will be all 0 since these variables are not allocated in current ELF.

Diff Detail

Event Timeline

yonghong-song created this revision.Oct 10 2019, 11:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 11:45 AM
yonghong-song edited the summary of this revision. (Show Details)
anakryiko added inline comments.Oct 11 2019, 3:31 PM
llvm/lib/Target/BPF/BTF.h
183–193

what's the difference between EXTERNAL and TENTATIVE?

llvm/lib/Target/BPF/BTFDebug.cpp
1064

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?

1121

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.

yonghong-song marked 3 inline comments as done.Oct 11 2019, 4:06 PM
yonghong-song added inline comments.
llvm/lib/Target/BPF/BTF.h
183–193

TENTATIVE is for .common symbols. Just do "int g;" and compile, you will find out.

llvm/lib/Target/BPF/BTFDebug.cpp
1064

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.

1121

No, all externs will emitted with their special section name.
CommonLinkage is not an extern.

yonghong-song retitled this revision from [WIP][BPF] Support external globals to [BPF] Support external globals.
yonghong-song edited the summary of this revision. (Show Details)

do not recognize section names for extern variables in llvm and leave it to bpf loader
upgrade and add test cases

if there is no type information available for extern variables, there is little benefit in having them as DATASEC+VAR. No size, no type. We can get the same information from ELF itself without having to teach verifier and libbpf how to handle these new types. Let's drop this for now? The only potential benefit is having a section name associated with extern, but we can either add that later, or even add it in a way that will require no verifier changes - BTF.ext.

Yes, we can drop it now. There is no rush if there is no real need. The extern variables are not used yet. Let us revisit this once we have some concrete use cases.

yonghong-song added a comment.EditedOct 21 2019, 8:27 PM

Actually, there is 'size' but no 'type'. And 'size' can be used to do cross checking. But anyway, we can delay this until there is a concrete thing we need to implement.

Also, another thing. we COULD emit debug info type for externs for BPF in clang. We just have to think whether this is really necessary or not.

hm... size is useful, actually. Ok, let me play with it, it's hard to get a good idea of BTF from LLVM tests. Verifier will definitely reject such variables, though, so we'll need to do sanitization for sure.

Also, another thing. we COULD emit debug info type for externs for BPF in clang. We just have to think whether this is really necessary or not.

I think it's extremely useful. Think about exposing some internal kernel stuff. This is going to be a requirement for BPF verifier, I think. I'll let @ast confirm, though.

yonghong-song abandoned this revision.Nov 22 2019, 11:35 AM

This patch is not needed any more.