btf: Implement support for bitfield relocations

[joamaki]

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.

[eiffel-fl]

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.

[lmb]

Hi Jussi,

Sorry for the delay and thank you for the PR. Looks nice!

[lmb]

Please add unit tests that test some edge cases wrt to alignment.

[joamaki]

Dropped the function (see discussion below)

[lmb]

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?

[joamaki]

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

[joamaki]

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 :)

[lmb]

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

[joamaki]

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.

[lmb]

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.

[joamaki]

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.

[lmb]

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?

[joamaki]

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.

[lmb]

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().

[joamaki]

Hm, not quite following what you're proposing. What would this FixupKind look like and why would we want to unexport COREKind?

[lmb]

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.

[joamaki]

Please take a look at how I structured it and see if it's ok.

[lmb]

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.

[lmb]

@joamaki please ping me when you want another round of review, not sure what the current status is.

[lmb]

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.

[joamaki]

@lmb thanks for the cleanups, all of them made sense to me.