On 11/26/19 10:01 PM, Andrii Nakryiko wrote:
> Similarly to a0d7da26ce86 ("libbpf: Fix call relocation offset calculation
> bug"), relocations against global variables need to take into account
> referenced symbol's st_value, which holds offset into a corresponding data
> section (and, subsequently, offset into internal backing map). For static
> variables this offset is always zero and data offset is completely described
> by respective instruction's imm field.
>
> Convert a bunch of selftests to global variables. Previously they were relying
> on `static volatile` trick to ensure Clang doesn't inline static variables,
> which with global variables is not necessary anymore.
>
> Fixes: 393cdfbee809 ("libbpf: Support initialized global variables")
> Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
LGTM with a few nits below.
Acked-by: Yonghong Song <yhs@xxxxxx>
> ---
> tools/lib/bpf/libbpf.c | 40 +++++++++----------
> .../testing/selftests/bpf/progs/fentry_test.c | 12 +++---
> .../selftests/bpf/progs/fexit_bpf2bpf.c | 6 +--
> .../testing/selftests/bpf/progs/fexit_test.c | 12 +++---
> tools/testing/selftests/bpf/progs/test_mmap.c | 4 +-
> 5 files changed, 35 insertions(+), 39 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b20f82e58989..4209b5a23a53 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -171,10 +171,8 @@ struct bpf_program {
> RELO_DATA,
> } type;
> int insn_idx;
> - union {
> - int map_idx;
> - int text_off;
> - };
> + int map_idx;
> + int sym_off;
> } *reloc_desc;
> int nr_reloc;
> int log_level;
[...]
> @@ -3622,31 +3622,27 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj)
> return 0;
>
> for (i = 0; i < prog->nr_reloc; i++) {
> + struct reloc_desc *relo = &prog->reloc_desc[i];
> +
> if (prog->reloc_desc[i].type == RELO_LD64 ||
> prog->reloc_desc[i].type == RELO_DATA) {
Using relo->type == RELO_LD64 or RELO_DATA?
> - bool relo_data = prog->reloc_desc[i].type == RELO_DATA;
> - struct bpf_insn *insns = prog->insns;
> - int insn_idx, map_idx;
> -
> - insn_idx = prog->reloc_desc[i].insn_idx;
> - map_idx = prog->reloc_desc[i].map_idx;
> + struct bpf_insn *insn = &prog->insns[relo->insn_idx];
>
> - if (insn_idx + 1 >= (int)prog->insns_cnt) {
> + if (relo->insn_idx + 1 >= (int)prog->insns_cnt) {
> pr_warn("relocation out of range: '%s'\n",
> prog->section_name);
> return -LIBBPF_ERRNO__RELOC;
> }
>
> - if (!relo_data) {
> - insns[insn_idx].src_reg = BPF_PSEUDO_MAP_FD;
> + if (relo->type != RELO_DATA) {
> + insn[0].src_reg = BPF_PSEUDO_MAP_FD;
> } else {
> - insns[insn_idx].src_reg = BPF_PSEUDO_MAP_VALUE;
> - insns[insn_idx + 1].imm = insns[insn_idx].imm;
> + insn[0].src_reg = BPF_PSEUDO_MAP_VALUE;
> + insn[1].imm = insn[0].imm + relo->sym_off;
> }
> - insns[insn_idx].imm = obj->maps[map_idx].fd;
> + insn[0].imm = obj->maps[relo->map_idx].fd;
> } else if (prog->reloc_desc[i].type == RELO_CALL) {
Using relo->type == RELO_CALL?
> - err = bpf_program__reloc_text(prog, obj,
> - &prog->reloc_desc[i]);
> + err = bpf_program__reloc_text(prog, obj, relo);
> if (err)
> return err;
> }
> diff --git a/tools/testing/selftests/bpf/progs/fentry_test.c b/tools/testing/selftests/bpf/progs/fentry_test.c
> index d2af9f039df5..615f7c6bca77 100644
> --- a/tools/testing/selftests/bpf/progs/fentry_test.c
> +++ b/tools/testing/selftests/bpf/progs/fentry_test.c
> @@ -6,28 +6,28 @@
[...]
