-
Notifications
You must be signed in to change notification settings - Fork 730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
btf: Implement support for bitfield relocations #573
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for it!
Your implementation is far better than my draft.
I will implement something to test it on my side and I come back with some "runtime" feedback.
efe0040
to
aefa88e
Compare
6815a83
to
d928ec2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Jussi,
Sorry for the delay and thank you for the PR. Looks nice!
internal/btf/core.go
Outdated
@@ -522,12 +571,31 @@ func adjustOffset(base uint32, t Type, n int) (uint32, error) { | |||
return base + (uint32(n) * uint32(size) * 8), nil | |||
} | |||
|
|||
// calculateBitfieldLoad computes the size and offset for loading a bitfield from | |||
// the target structure. | |||
func calculateBitfieldLoad(bitOffset, bitSize int) (size int, offset int, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add unit tests that test some edge cases wrt to alignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped the function (see discussion below)
internal/btf/core.go
Outdated
// the target structure. | ||
func calculateBitfieldLoad(bitOffset, bitSize int) (size int, offset int, err error) { | ||
// Start with the smallest possible aligned load before the bit offset. | ||
size = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't what libbpf is doing, I think? It starts with the size of the type to preserve the alignment of the load.
I'm thinking about structures like this:
struct foo {
char a;
int b : 9;
}
In my opinion, this example should produce offset = 0, size = 4 for a load of b. Is that the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this isn't doing the same as libbpf and it's not the "simpler" version you had. This version will use the smallest size load possible, so for this example it would do the following:
size=1, offset=8 / 8 / 1 * 1 = 1; 8+9 > (1+1*8) => 17 > 8 => load size too small
size=2, offset=8 / 8 / 2 * 2 = 0; 8+9 > (0+2*8) => 17 > 16 => load size too small
size=4, offset=8 / 8 / 4 * 4 = 0; 8+9 > (0+4*8) => 17 > 32 => load size big enough to capture the field
The calculation always produces an aligned offset:
offset = bitOffset / 8 # offset in bytes, rounded down, e.g. aligned to 1 byte.
offset = offset / size # "number" of "size" blocks to reach the bit offset
offset = offset * size # the offset aligned to "size" that is "just" before the bitOffset.
The benefit of this should be that we wouldn't read past the end of the struct if the bitfield was at the
end and was made smaller: u64 foo:32, bar:32;
=> u32 bar:32
. We'd want a 4 byte load, but the
original load is 8 bytes and will read past the end.
Does that make sense or would you prefer that we'll use the libbpf version or your "simpler" version?
I'm fairly sure this should be an acceptable implementation for this, but please double-check :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it's failing when asked to compute the load for 31 bytes from bit offset 34 (bits 34..65), e.g. it'll try 1 byte from 32, then 2 bytes from 32, then 4 bytes from 32, and then 8 bytes from 0... and no solution. Should've written the exhaustive tests first :)
I'm on vacation now until Tuesday. I'm fix this then.
EDIT: No wait.. there is no solution to this: we can't do such an aligned load. My tests are the issue. Anyway, will continue on Tuesday :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll take a closer look at your version as well. One thing to note before I forget: I think that using bitSize to derive alignment needs some documentation. This is because for something like int b : 8
we'd have alignment of 4 but the bitSize would imply alignment of 1.
Regardless, we should document the following constraints:
- Must not read past the end of the struct
- Must produce a read <= bytes
- Must produce a read that is aligned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought through this once more and I'm now fairly convinced that we can just compute the right load offset with bitOffset / 8 / loadSizeBytes * loadSizeBytes
. From target BTF we know the bitfield offset and the size of the "containing" field (e.g. u32 x:8 => loadSizeBytes = 4
). C standard specifies that the bitfield cannot be larger than the type of the variable (u32 x:33
is not allowed). And since loads are aligned this should work, unless I'm missing some weird corner-case.
internal/btf/core.go
Outdated
// For bitfields we compute the offset from which to load | ||
// the value, and we include the offset of the bitfield and its | ||
// size in 'coreField' so we can later compute the proper bit shift. | ||
size, offset, err := calculateBitfieldLoad(int(targetOffset), int(targetMember.BitfieldSize)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be "safer" to use targetMember.OffsetBits
here and adjust by targetOffset afterwards? Rationale: calculateBitfieldLoad operates on an offset in a type, but targetOffset really is the offset from the start of the "enclosing type".
Might not make a difference ultimately, so no worries if it's too complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm interesting question. I think we do want to use the targetOffset
to make sure we produce a load that's properly aligned to the start of the whole structure. If we just use targetMember.OffsetBits
then we'd produce a load that's aligned to the inner structure's start. But yeah, probably doesn't make a difference.. unless it's a packed struct, in which case it's safer to align to the outer structure.
internal/btf/core.go
Outdated
@@ -15,10 +16,18 @@ import ( | |||
// Code in this file is derived from libbpf, which is available under a BSD | |||
// 2-Clause license. | |||
|
|||
type OptionalLocalValue int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit heavy handed just to opt out of "validation". Some alternatives:
- We skip validation in general and get rid of the local field. Maybe too radical, validation is useful when working on the CO-RE code.
- Skip validation by default, but have a way to enable "strict mode". Would still break on bitfields, so of limited use. Even more code churn.
- Introduce new COREKind for bitfield related relos which disable validation (and assert that local == 0).
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wasn't very happy with it. I thought about adding a COREKind
, but since these are constants coming from BTF I felt icky adding a "magic" non-overlapping kind there, e.g. to me it felt worse than doing it this way.
The validation does make sense as often we can figure out what we're expecting to see, so it's good to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can introduce a separate type likeFixupKind
if we want and unexport current COREKind
. If I remember correctly Fixup.Kind is only used in String()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not quite following what you're proposing. What would this FixupKind
look like and why would we want to unexport COREKind
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think of COREKind
as defined by libbpf, more specifically by the C CORE headers. FixupKind
would be a superset of COREKind, with additional types that are useful to us. I'd unexport COREKind since it's kind of an implementation detail of libbpf, and to avoid confusion between COREKind and FixupKind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at how I structured it and see if it's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you did a good job with a bad idea of mine :D At that point I hadn't realised that it's necessary to mix bitfields and non-bitfields in the same fixup. We should probably chuck the validate bool in the Fixup, but that is something for another day. Unexporting coreKind is definitely the way to go though, we shouldn't expose libbpf ABI if we can avoid it.
303507e
to
b002f00
Compare
@joamaki please ping me when you want another round of review, not sure what the current status is. |
16c2fb0
to
78fb49f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Jussi,
Sorry for taking longer to review, it took me a while to understand how bitfields work again. Kudos for slotting them in so nicely into the existing code.
When reviewing I realised that there are two core things I want to change wrt coreFindField
that aren't necessary for your work, but that would make it easier to fix the things I'd call out. This is why I went ahead and implemented those changes. I tried to keep the refactoring separate from review comments as much as possible, but you'll see that I didn't always succeed.
The first change is that coreFindField is already pretty complex, and suffers from us remembering to clear the variables that make up a coreField. I've refactored it so that the function now deals with coreField from the start of the function. This allows putting helpers on coreField which makes coreFindField itself smaller. This in turn makes it really simple to calculate offsets for local bitfield relocations in addition to the target offsets.
The second change is that the definition of coreField
is inconvenient. offset
is documented to be in bytes, but is actually stored in bytes and bitfieldOffset
includes offset. I've pushed a commit with what I think is a better structure:
/- start of composite (struct, union, ...)
| offset * 8 | bitfieldOffset | bitfieldSize | ... |
\- start of field end of field -/
offset is stored in bytes, as documented. It's implicitly aligned since for normal fields we get that information from the compiler. bitfieldOffset is stored in bits, but starts at the end of offset. This is the form we need for the shift relocations. bitfieldSize is stored in bits and is kept unchanged.
I considered a completely alternate structure as well, where we only ever store the offset in bits and convert to bytes wherever it's needed.
struct {
...
offsetBits uint32
bitfieldSize uint32
}
The problem here is that we lose the knowledge which offset was generated by the compiler, which is important since it guarantees alignment.
Please let me know what you think.
297e8b2
to
2c79916
Compare
@lmb thanks for the cleanups, all of them made sense to me. |
2c79916
to
6dae520
Compare
c0104df
to
5ff6bbc
Compare
This adds support for relocating bitfields, e.g. rewriting of the load offset and bit shifts for accessing the field. The implementation is mostly following libbpf, but does not include the loop for doubling the size of the load as that should not be necessary when one assumes that loads must be aligned and that a bitfield cannot be larger than the variable type. Signed-off-by: Jussi Maki <jussi@isovalent.com> Signed-off-by: Timo Beckers <timo@isovalent.com>
The current test doesn't check all fields of coreField. Fix this by using go-cmp.
The documentation on coreField.offset states that it is in bytes, but the code stores it in bits. Refactor the code to use bytes, since that is more natural for the rest of the code. Change coreFindField to re-initialize coreField variables instead of only creating coreFields when returning. This has the benefit of implicitly zeroing fields of the local and target variables. It also allows us to put helper methods on coreField to slim coreFindField down a little bit.
Make sure that we're at the end of an accessor when dealing with a bitfield. This also allows us to fall through to after the switch instead of returning early.
We currently use the size of the target type to align the load of a bitfield. This is OK for integers and enums, since their size equals their alignment. It's not correct in general however. Add alignof to make this explicit.
coreField.bitfieldOffset is currently defined to count from the start of the enclosing composite type. This is incovenient, since the shift relocations want to know the offset from the start of the load. Redefine coreField.bitfieldOffset to count from the start of the field and add coreField.adjustOffsetBits to handle adjusting offset and bitfieldOffset while respecting alignment. Also compute offsets for the local relocation. We don't use this information at the moment, but it's what we do for other types of relocations. Finally, move the compatibility behaviour for regular fields accessed as bitfield into coreField.sizeBits. Doing the fixup only when encountering a bitfield leads to inconsistent behaviour.
5ff6bbc
to
85daa93
Compare
This adds support for relocating bitfields, e.g. rewriting of
the load offset and bit shifts for accessing the field.
The implementation is mostly following libbpf, but does not
include the loop for doubling the size of the load as that should
not be necessary when one assumes that loads must be aligned and
that a bitfield cannot be larger than the variable type.