This is an archive of the discontinued LLVM Phabricator instance.

[BPF] Use SectionForGlobal() for section names computation in BTF
ClosedPublic

Authored by eddyz87 on Dec 21 2022, 2:52 PM.

Details

Summary

Use function TargetLoweringObjectFile::SectionForGlobal() to compute
section names for globals described in BTF_KIND_DATASEC records.

This fixes a discrepancy in section name computation between
BTFDebug::processGlobals and the rest of the LLVM pipeline.

Specifically, the following example illustrates the discrepancy
before this commit:

struct Foo {
  int i;
} __attribute__((aligned(16)));
struct Foo foo = { 0 };

The initializer for 'foo' looks as follows:

%struct.Foo { i32 0, [12 x i8] undef }

TargetLoweringObjectFile::SectionForGlobal() classifies 'foo' as
a part of '.bss' section, while BTFDebug::processGlobals
classified it as a part of '.data' section because of the
following expression:

SecName = Global.getInitializer()->isZeroValue() ? ".bss" : ".data"

The isZeroValue() returns false because of the undef tail of the
initializer, while SectionForGlobal() allows such patterns in '.bss'.

Diff Detail

Event Timeline

eddyz87 created this revision.Dec 21 2022, 2:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2022, 2:52 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
eddyz87 published this revision for review.Dec 21 2022, 2:59 PM
eddyz87 added a reviewer: yonghong-song.
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2022, 2:59 PM

Otherwise, the patch LGTM.

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

Could you change the above 'auto' to actual types? Typically we use 'auto' for some *obvious* times. The above two are not obvious.

yonghong-song added inline comments.Dec 28 2022, 9:22 AM
llvm/test/CodeGen/BPF/BTF/global-var-bss-and-data.ll
2

The default check-prefix is 'CHECK' so '-check-prefixes=CHECK' can be rmoved.

15

Let us remove the above line 12-15. The 'llc' command line already has -march=<> flag. Also, the above target datalayout is only for little endian. The big endian datalayout is 'E-m:e-p:...'.

eddyz87 updated this revision to Diff 485619.Dec 29 2022, 8:23 AM

Styling updates as requested by Yonghong.

eddyz87 marked 3 inline comments as done.Dec 29 2022, 8:25 AM
This revision was not accepted when it landed; it landed in state Needs Review.Dec 29 2022, 11:27 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.