This is an archive of the discontinued LLVM Phabricator instance.

MC: Add .data. and .rodata. prefixes to MCContext section classification
ClosedPublic

Authored by davemarchevsky on Nov 21 2022, 11:44 PM.

Details

Summary

Commit 463da422f019 ("MC: make section classification a bit more
thorough") changed MCContext::getELFSection section classification logic
to default to SectionKind::getText (previously default was
SectionKind::getReadOnly) and added some matching based on section name
to determine internal section classification.

The BPF runtime implements global variables using 'BPF map'
datastructures, specifically the arraymap BPF map type. Global variables
in a section are placed in a single arraymap value at predictable byte
offsets. Variables in different sections are placed in separate
arraymaps, so in this example:

#define SEC(name) __attribute__((section(name)))
SEC(".data.A") u32 one;
SEC(".data.A") u32 two;
SEC(".data.B") u32 three;
SEC(".data.B") u32 four;

variables one and two would correspond to some byte offsets (probably 0
and 4) in one arraymap, while three and four would be in a separate
arraymap. Variables of a bpf_spin_lock type are considered to protect
next-generation BPF datastructure types in the same arraymap value and
there can only be a single bpf_spin_lock variable per arraymap value -
and thus per section.

As a result it's necessary to keep bpf_spin_locks and the datastructures
they guard in separate data sections. Before the aforementioned commit,
a section whose name starts with ".data." - like ".data.A" - would be
classified as SectionKind::getReadOnly, whereas after it is
SectionKind::getText. If 4-byte padding is required in such a section due to
alignment of some symbol within it, classification of the section as
SectionKind::getText will result in compilation of those variables to
BPF backend failing with an error like "unable to write nop sequence of
4 bytes". This is due to nop instruction emitted in
BPFAsmBackend::writeNopData being 8 bytes, so the function fails since
it cannot emit a 4-byte nop instruction.

Let's follow the pattern of matching section names starting with ".bss."
and ".tbss." prefixes resulting in proper classification of the section
as data by adding similar matches for ".data." and ".rodata." prefixes.
This will bring padding behavior for these sections back to what it was
before that commit and fix the crash.

Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 21 2022, 11:44 PM
davemarchevsky requested review of this revision.Nov 21 2022, 11:44 PM
compnerd requested changes to this revision.Nov 22 2022, 7:57 AM
compnerd added inline comments.
llvm/test/MC/ELF/data-section-prefix.ll
2

Could you please verify the output?

This revision now requires changes to proceed.Nov 22 2022, 7:57 AM

v1 -> v2: Use llvm-readobj and FileCheck to confirm that .data.A section is emitted with correct flags

davemarchevsky marked an inline comment as done.Dec 8 2022, 12:52 PM
davemarchevsky added inline comments.
llvm/test/MC/ELF/data-section-prefix.ll
2

sorry for delay, addressed!

Could you please also add a test case for .rodata.suffix as that is added as a case as well.

davemarchevsky marked an inline comment as done.

Add test data and FileCheck annotations to exercise .rodata.suffix logic

compnerd accepted this revision.Dec 23 2022, 10:35 AM
This revision is now accepted and ready to land.Dec 23 2022, 10:35 AM
chapuni added inline comments.
llvm/test/MC/ELF/data-section-prefix.ll
19

Would it run if bpf is not configured?

yonghong-song added inline comments.Dec 27 2022, 8:04 PM
llvm/test/MC/ELF/data-section-prefix.ll
19

Thanks for pointing this out. Yes, the test will fail if bpf is not configured. I pushed https://github.com/llvm/llvm-project/commit/5922d361229430a373e007a8e82ad69c7186d56d which fixed the issue.

Amir added a subscriber: Amir.Dec 28 2022, 7:14 PM

Hi,

This commit might have caused multi-stage lto+pgo build breakage: https://lab.llvm.org/buildbot/#/builders/246/builds/1358.
There are other commits included in the build but they seem to be unrelated.
Can you please verify?

@Amir

Somehow, CCACHE does not work for me, which has an error message like missing 'malloc/malloc.h'. I used the following scripts without CCACHE.

$ cat build_llvm_debug.sh
cd llvm-project/llvm/build

#    -DLLVM_CCACHE_BUILD=ON \
#    -C llvm-project/clang/cmake/caches/BOLT-PGO.cmake
#    -DBOOTSTRAP_LLVM_CCACHE_BUILD=ON \

cmake .. \
    -DLLVM_APPEND_VC_REV=OFF \
    -DLLVM_ENABLE_LLD=ON \
    -DBOOTSTRAP_LLVM_ENABLE_LLD=ON \
    -DBOOTSTRAP_BOOTSTRAP_LLVM_ENABLE_LLD=ON \
    -DPGO_INSTRUMENT_LTO=Thin \
    -DLLVM_ENABLE_PROJECTS="clang;lld;bolt;llvm" \
    -DCMAKE_BUILD_TYPE=Release \
    -DLLVM_ENABLE_ASSERTIONS=ON \
    -DLLVM_LIT_ARGS="-v -vv" \
    -G Ninja \
    -DCMAKE_INSTALL_PREFIX=$PWD/install

ninja && ninja install

It works fine with me,

...
[4878/4882] Building CXX object tools/obj2yaml/CMakeFiles/obj2yaml.dir/elf2yaml.cpp.o
[4879/4882] Linking CXX executable bin/obj2yaml
[4880/4882] Building CXX object tools/llvm-readobj/CMakeFiles/llvm-readobj.dir/ELFDumper.cpp.o
[4881/4882] Linking CXX executable bin/llvm-readobj
[4882/4882] Generating ../../bin/llvm-readelf
[1/4] Performing build step for 'bolt_rt'
ninja: no work to do.
[2/4] Performing install step for 'bolt_rt'
[0/1] Install the project...
-- Install configuration: "Release"
-- Up-to-date: /home/yhs/work/llvm-project/llvm/build/lib/libbolt_rt_instr.a
-- Up-to-date: /home/yhs/work/llvm-project/llvm/build/lib/libbolt_rt_hugify.a
[3/4] Completed 'bolt_rt'
[3/4] Install the project...
-- Install configuration: "Release"
-- Installing: /home/yhs/work/llvm-project/llvm/build/install/include/llvm
...

Does CCACHE impact the build? If you can help me resolve CCACHE build issue, I am happy to try the build again.

Amir added a comment.Dec 29 2022, 10:06 AM

@Amir

Somehow, CCACHE does not work for me, which has an error message like missing 'malloc/malloc.h'. I used the following scripts without CCACHE.

$ cat build_llvm_debug.sh
cd llvm-project/llvm/build

#    -DLLVM_CCACHE_BUILD=ON \
#    -C llvm-project/clang/cmake/caches/BOLT-PGO.cmake
#    -DBOOTSTRAP_LLVM_CCACHE_BUILD=ON \

cmake .. \
    -DLLVM_APPEND_VC_REV=OFF \
    -DLLVM_ENABLE_LLD=ON \
    -DBOOTSTRAP_LLVM_ENABLE_LLD=ON \
    -DBOOTSTRAP_BOOTSTRAP_LLVM_ENABLE_LLD=ON \
    -DPGO_INSTRUMENT_LTO=Thin \
    -DLLVM_ENABLE_PROJECTS="clang;lld;bolt;llvm" \
    -DCMAKE_BUILD_TYPE=Release \
    -DLLVM_ENABLE_ASSERTIONS=ON \
    -DLLVM_LIT_ARGS="-v -vv" \
    -G Ninja \
    -DCMAKE_INSTALL_PREFIX=$PWD/install

ninja && ninja install

It works fine with me,

...
[4878/4882] Building CXX object tools/obj2yaml/CMakeFiles/obj2yaml.dir/elf2yaml.cpp.o
[4879/4882] Linking CXX executable bin/obj2yaml
[4880/4882] Building CXX object tools/llvm-readobj/CMakeFiles/llvm-readobj.dir/ELFDumper.cpp.o
[4881/4882] Linking CXX executable bin/llvm-readobj
[4882/4882] Generating ../../bin/llvm-readelf
[1/4] Performing build step for 'bolt_rt'
ninja: no work to do.
[2/4] Performing install step for 'bolt_rt'
[0/1] Install the project...
-- Install configuration: "Release"
-- Up-to-date: /home/yhs/work/llvm-project/llvm/build/lib/libbolt_rt_instr.a
-- Up-to-date: /home/yhs/work/llvm-project/llvm/build/lib/libbolt_rt_hugify.a
[3/4] Completed 'bolt_rt'
[3/4] Install the project...
-- Install configuration: "Release"
-- Installing: /home/yhs/work/llvm-project/llvm/build/install/include/llvm
...

Does CCACHE impact the build? If you can help me resolve CCACHE build issue, I am happy to try the build again.

The build succeeded today without any intervention, sorry for bothering you. Might have been a stale ccache issue.