Make BTF for data sections optional #675
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The default behaviour of podman is to drop all stdout/err it generates into the journal, which gets rather noisy. Signed-off-by: Timo Beckers <timo@isovalent.com>
|
I'll try to review this Wednesday afternoon after #647. |
lmb
reviewed
May 18, 2022
| volatile int ctx = 0; | ||
|
|
||
| // 32 bytes wide results in a .rodata.cst32 section. | ||
| #define values \ |
There was a problem hiding this comment.
Could we just fake a .rodata.foo via explicit section annotations? Might be more robust on newer versions of LLVM as well.
There was a problem hiding this comment.
I've tried all possible ways I could think of to force this to be emitted to a particular section, but haven't managed. Since it's not a real symbol, clang doesn't support setting the section attribute on it.
| @@ -131,6 +131,12 @@ func TestLoadCollectionSpec(t *testing.T) { | |||
| SectionName: "static", | |||
| License: "MIT", | |||
| }, | |||
| "anon_const": { | |||
There was a problem hiding this comment.
Seems like there is no fix for the problem here, this means that the tests will fail on this commit? If yes, that's problematic when trying to bisect problems.
There was a problem hiding this comment.
Okay, didn't know we really cared about that, will respin the set to take this into account.
|
|
||
| copy(cpy[v.Offset:v.Offset+v.Size], b) | ||
|
|
||
| replaced[vname] = true |
There was a problem hiding this comment.
Should we allow replacing the same variable in multiple rodata sections? Seems wonky.
There was a problem hiding this comment.
What's the reasoning behind not allowing it? Detecting compiler bugs? Technically, the same var being present in multiple rodata sections would also require having 2 equally-named ELF symbols with global vis, which is wonky indeed. So, we error here instead?
There was a problem hiding this comment.
Done, added a check that makes sure we don't replace the same var twice.
There was a problem hiding this comment.
I guess this could also happen when the caller copies .rodata from a CollectionSpec and adds it under a different key in Maps.
LoadCollectionSpecFromReader marks all .rodata* sections as data sections, but loadDataSections() only freezes '.rodata'. Signed-off-by: Timo Beckers <timo@isovalent.com>
https://lore.kernel.org/bpf/CAK3+h2wcDceeGyFVDU3n7kPm=zgp7r1q4WK0=abxBsj9pyFN-g@mail.gmail.com If an anonymous value or constant (e.g. array declared in a macro) ends up in a data section, it will not be accompanied by debug info, resulting in no BTF being emitted for the value. If the section consists of only anonymous values, there could be no BTF for the section as a whole. A workaround was added to LLVM to always emit an empty BTF Datasec for .rodata specifically, but this doesn't work for special sections like .rodata.cst32 and friends. Signed-off-by: Timo Beckers <timo@isovalent.com>
Symbols for anonymous constants are somewhat irregular on older versions
of LLVM. This commit relaxes the constraints on symbols those compilers
emit map relocations against.
LLVM 7:
1: 0000000000000000 0 NOTYPE LOCAL DEFAULT 16 .Lconstinit.1
LLVM 9:
1: 0000000000000000 32 OBJECT LOCAL DEFAULT 22 .Lconstinit.1
LLVM 14 has these symbols correctly sanitized.
Signed-off-by: Timo Beckers <timo@isovalent.com>
https://lore.kernel.org/bpf/CAK3+h2wcDceeGyFVDU3n7kPm=zgp7r1q4WK0=abxBsj9pyFN-g@mail.gmail.com Signed-off-by: Timo Beckers <timo@isovalent.com>
Instead of only looking for constants in .rodata, iterate over all maps with an .rodata prefix. Moved the datasec bytes and BTF info extraction into a method on MapSpec. Lifted patchValue back into RewriteConstants. Signed-off-by: Timo Beckers <timo@isovalent.com>
This exercises support for constants in custom .rodata* sections. Signed-off-by: Timo Beckers <timo@isovalent.com>
ti-mo
force-pushed
the
tb/datasec-optional-btf
branch
from
f960522
to
352d2b3
Compare
dylandreimerink
added a commit
to dylandreimerink/ebpf
that referenced
this pull request
Jul 20, 2022
In the past we had the limitation that string sections like `.rodata.str1.1` could not not be loaded since clang/LLVM doesn't provide BTF information for such sections. In cilium#675 we lifted the requirement to have BTF information for global data sections. This means that we can now also allow string sections to be loaded as global data. All facilities to do so are already in place, this commit just removes the check for references to sections with the `SHF_STRINGS` flag set. Fixes: cilium#741 Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
dylandreimerink
added a commit
to dylandreimerink/ebpf
that referenced
this pull request
Jul 21, 2022
In the past we had the limitation that string sections like `.rodata.str1.1` could not not be loaded since clang/LLVM doesn't provide BTF information for such sections. In cilium#675 we lifted the requirement to have BTF information for global data sections. This means that we can now also allow string sections to be loaded as global data. All facilities to do so are already in place, this commit just removes the check for references to sections with the `SHF_STRINGS` flag set. Fixes: cilium#741 Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
dylandreimerink
added a commit
to dylandreimerink/ebpf
that referenced
this pull request
Jul 22, 2022
In the past we had the limitation that string sections like `.rodata.str1.1` could not not be loaded since clang/LLVM doesn't provide BTF information for such sections. In cilium#675 we lifted the requirement to have BTF information for global data sections. This means that we can now also allow string sections to be loaded as global data. All facilities to do so are already in place, this commit just removes the check for references to sections with the `SHF_STRINGS` flag set. Fixes: cilium#741 Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
lmb
pushed a commit
that referenced
this pull request
Jul 22, 2022
In the past we had the limitation that string sections like `.rodata.str1.1` could not not be loaded since clang/LLVM doesn't provide BTF information for such sections. In #675 we lifted the requirement to have BTF information for global data sections. This means that we can now also allow string sections to be loaded as global data. All facilities to do so are already in place, this commit just removes the check for references to sections with the `SHF_STRINGS` flag set. Fixes: #741 Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a workaround for a compiler bug/limitation when working with arrays declared as preprocessor macros.
This bit us on two occasions:
Discussion on the list:
https://lore.kernel.org/bpf/CAK3+h2wcDceeGyFVDU3n7kPm=zgp7r1q4WK0=abxBsj9pyFN-g@mail.gmail.com
Best reviewed per commit.