allow using binary.NativeEndian #1384
Conversation
lmb
force-pushed
the
native-endian
branch
4 times, most recently
from
485a6c5
to
f528c20
Compare
Remove the internal implementation of NativeEndian in favour of binary.NativeEndian. There is one hiccup: it's not possible to use a simple comparison to figure out whether NativeEndian is the same as for example LittleEndian. The IsNativeEndian helper takes care of this. See golang/go#57237 (comment) Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
|
Discussed with Timo: seems like a lot of pain for little gain. We'll discuss next week. |
| @@ -935,14 +935,19 @@ func (iter *InstructionIterator) Next() bool { | |||
| type bpfRegisters uint8 | |||
|
|
|||
| func newBPFRegisters(dst, src Register, bo binary.ByteOrder) (bpfRegisters, error) { | |||
| buf := make([]byte, 2) | |||
| val := uint16(dst&0x0F)<<8 | uint16(src&0x0F) | |||
| switch bo { | |||
| case binary.LittleEndian: | |||
There was a problem hiding this comment.
why not replacing this whole switch with bo.PutUint16 directly ?
There was a problem hiding this comment.
That's the annoying bit: calling an interface method cannot be inlined, plus passing a pointer to an interface method causes it to escape. This function is called a ton, so it starts adding up.
We have some amount of discovery to do, because now binary.NativeEndian is a thing, we can probably eliminate these kind of switches/branches and replace them with binary.NativeEndian.PutUint16, since marshaling bpf insns is usually done before loading into the host kernel, and we'd almost always do that in native endianness.
Instruction.Marshal() is exported API, though, so we should have a look at public callers on Sourcegraph to see if anyone depends on marshaling asm.Instructions in arbitrary endiannesses (e.g. for converting ELFs from LE to BE).
There was a problem hiding this comment.
ah yeah, that's too bad
There was a problem hiding this comment.
Instruction.Marshal()is exported API, though, so we should have a look at public callers on Sourcegraph to see if anyone depends on marshaling asm.Instructions in arbitrary endiannesses
I'd rather not get into the habit of hardcoding NativeEndian, it leads to not thinking about endianness at callsites and that tends to cause subtle bugs. Compared to that the type switch isn't bad at all imo.
|
Coming back to this, I find our |
|
yeah makes sense. i think the real solution is to stop using |
use binary.NativeEndian