search

trema / trema-edge

Transient repository for Trema OF1.3 branch
27 stars 14 forks source link

Returning pointers to stack allocated variables has undefined behavior #38

Closed rakshasa closed 11 years ago

rakshasa commented 11 years ago

The 'mac_addr_to_cstr' function causes mac addresses to zero out due to compiler optimization since pointers to stack allocated memory is not valid outside the scope of the function.

Testing is limited to running trema due to issues with compiling the unit tests.

coveralls commented 11 years ago

Coverage Status

Coverage remained the same when pulling df1dad95628b25ffa1f428d72ae2744e680b5129 on axsh:fix-invalid-array-return into ed31a9131289e27155915198ad1ae694957777c4 on trema:master.

nickkaranatsios commented 11 years ago

Thanks fixed in https://github.com/trema/trema-edge/commit/c2273333cc1ced96545b1b8d5a64fe3d4b914e02. I ignored the __thread for portability reasons.

rakshasa commented 11 years ago

Ok, thanks.

However it now gets allocated on the data segment, not the stack, and might as such share a cache line with variables modified by other threads. Since attribute 'packed' is used already, there should be no portability issue with using "attribute((aligned(128)))" instead of __thread.

nickkaranatsios commented 11 years ago

Thanks for the comment. One question is every linker capable of aligning variables to a maximum specified value of 128?

rakshasa commented 11 years ago

Note that the effectiveness of aligned attributes may be limited by inherent limitations in your linker. On many systems, the linker is only able to arrange for variables to be aligned up to a certain maximum alignment. (For some linkers, the maximum supported alignment may be very very small.) If your linker is only able to align variables up to a maximum of 8-byte alignment, then specifying aligned(16) in an attribute still only provides you with 8-byte alignment. See your linker documentation for further information.

http://gcc.gnu.org/onlinedocs/gcc/Variable-Attributes.html

If the linker does not support 128 byte alignment then it should silently fall back to the biggest supported alignment, which should not cause any concurrency issues assuming the linker has this restriction due to a smaller cache line or different handling of memory access.

In the case where the linker does not support aligned large enough to take up a whole cache line you're going to have to use somewhat ugly hacks. However this seems like it would be a rather esoteric combination of linker and architecture.