BCC cannot handle alignment in struct used in HashMap

[janrueth]

Hi,

I had the following struct as an item in a hash map:

struct Leaf {
  uint16_t len;
  unsigned char p[32];
};

I needed to copy the contents of p to the stack and I though uint64_t* copies would be faster and less instructions so I did:

char dst[32];
((uint64_t*)dst)[0] = ((uint64_t*)leaf->p)[0];
((uint64_t*)dst)[1] = ((uint64_t*)leaf->p)[1];
((uint64_t*)dst)[2] = ((uint64_t*)leaf->p)[2];
((uint64_t*)dst)[3] = ((uint64_t*)leaf->p)[3];

However, this caused the compiler to restore a register like this r1 = (uint64_t*)(r5 + 2) (I don't remember the actual error line) which of cause caused a misalignment error. I came to the conclusion ( even though I think the compiler should not have done that uint64_t* restoring) that this does not work as the struct is misaligned... So I changed the struct to:

struct Leaf {
  uint16_t len;
    unsigned char p[32] __attribute__ ((aligned (8)));
};

which works as expected... however, now when I fill the map from userspace... the data that I put into p is off by 6 byte, it seems that the userspace is unable to detect the actual alignment of the members in the struct and starts to put data for p right after len. The alignment should cause the compiler to add 6 bytes of padding between len and p.

Of course I can change everything in a way that it works however I find this rather difficult to debug...

Using __builtin_memcpy does not produce this behavior, there I can leave the alignment out or I can also switch p and len in the struct to make everything work...

I was just wondering is this a conceptual limitation or just a bug in BCC?

[4ast]

@drzaeus77 I think attribute ((aligned (8))) is not recognized by python side, so it's a bug.
so it's working fine with __builtin_memcpy and no extra align directives, right?
Changing order of p and len makes sense to avoid the hole regardless.
Also memcpy generated code will be more efficient. Same rules as in normal C applies:
llvm will use the largest load/store quantities depending on the alignment it can detect.

[janrueth]

Yep, it works with the __builtin_memcpy without extra alignment directives.
Well changing the order of p and len makes sense from a "I know what a C compiler does" perspective from code structure I would always prefer to have a length field in front of the actual data field. Nevertheless, I just wanted to post this as others might run in the same mistake and still as you said, not acknowledging the alignment is a bug :)

I was actually going to fix this, but I could not figure out where I would start looking, is there a document that describes the structure and the internal components of bcc?

[goldshtn]

@4ast: It's not actually on the Python side. I checked the JSON type description we're getting in BPF.get_table in both cases. They are identical with and without the aligned attribute. It's the Clang side that generates the structure layout information that needs to recognize alignment properly. BMapDeclVisitor::VisitRecordDecl specifically.

[goldshtn]

@4ast @drzaeus77: Seems to me that we would have to call ASTRecordLayout::getFieldOffset for each field and get their offsets in the structure to account for this.

[drzaeus77]

Getting the offset out clang is pretty easy, but I can't find the way in ctypes to enforce the alignment of a particular field or the struct as a whole. The pack attribute is for the opposite case.

>>> cls = type("Leaf", (ctypes.Structure,), dict(_fields_=[("a", ctypes.c_char), ("b", ctypes.c_longlong)], _pack_=2))
>>> cls.b.offset
2
>>> cls = type("Leaf", (ctypes.Structure,), dict(_fields_=[("a", ctypes.c_char), ("b", ctypes.c_char*32)], _pack_=8))
>>> cls.b.offset
1

[goldshtn]

Maybe you could insert artificial padding fields?

[oliviertilmans]

Bringing the discussion from #1131:

I recently hit a related bug when trying to access key/values from a complex C structure. This would cause random segfaults or absurd interpreter corruption (i.e. objects would 'lose' some of their attributes over time ...).

My take on it was that as the C structs are 8bytes aligned, while ctypes assumes only 4, the buffers it was allocating to call the various lib_bpf functions were too small (i.e. the lib_bpf code was overwriting cpython memory ...).

I looked into how re-using the field offset info that is available in the clang frontend in order to align the ctypes structures properly. It does not seem feasible as: (i) the pack directive for ctypes Structure cannot grow larger than 4 on my system (it is ultimately defined by libffi in cpython) (ii) The offset variable of fields in a ctype Structure is read-only for the python VM (i.e. I cannot alter a class definition to change the auto-computed offsets). I guess that all of this could be solved by generating struct format strings instead of ctypes objects, but that would needed some extensive work to make it backward compatible with current code.

Adding artificial padding fields would work as long as the ctypes structure pack parameter is set to 1 (which will make padding computation straightforward as this will disable ctypes own padding ...).

At the very least, the ctypes-generated layout could be checked and a warning/error could be reported if offset values are inconsistent, i.e. exporting field offset alongside their type/names and checking that the offset value computed by ctypes is consistent (as given by Class.field.offset).

My current workaround ensures that all 'parsed' structs (i.e. contained in maps accessed by python) are packed, and have their fields 4bytes aligned (possibly over-sizing some of them or adding unused padding). This seems sufficient for now (ctypes over-allocates in the worst case), but is certainly optimal wrt. performance.