implement safer align_ function

[dodsonmg]

Splits the align_ function and overloads it avoids an unnecessary cast from size_t to void *.

Avoids passing the casted pointer by reference. In the existing align_ function, the pointer argument is both manipulated via reference and returned, which is both confusing and potentially dangerous, since it's behaviour is not clear from the function prototype.

[rotu]

This code needs an entire overhaul, so I don't think it's really worth refactoring to make it nicer. It also seems like fundamentally unsafe code to start with (reinterpret_casting an arbitrary std::vector<T> * to a std::vector<unsigned char> * is SO wrong..., and casting a pointer to a uint_t, aligning, then casting back is not guaranteed to align the pointer), so I think this is somewhat a misdirected effort.

Ignoring the pre-existing problems with this code, introducing a new template with the same name as an existing one (align_) is a recipe for confusion, especially since their type signatures can overlap - go with either templates or function overloading - not both.

That said, the PR looks otherwise correct.

[jrtc27]

For what it's worth, Clang has a __builtin_align_up these days that is similarly overloaded to work with both pointers and integers. The whole point of overloading here is that they are semantically the same, there are just some extra casts needed for pointers. But if that really concerns you still then would separate templated align_ptr and align_int functions (ie just renaming what this PR does) be suitable?

[rotu]

Exactly. Making these templates align_ptr and align_int would resolve my concern here. So would using a single template and SFINAE. It allows you to reason more easily about which implementation is called (and makes it obvious that align_ is not recursive, which it may appear at first).

[jrtc27]

Oh if you're happy with SFINAE and C++11 then how about:

template<typename T>
static inline
std::enable_if_t<std::is_integral<T>::value>
align_(size_t __align, T __int) noexcept
{
  return (__int - 1u + __align) & ~(__align - 1);
}

template<typename T>
static inline
std::enable_if_t<std::is_pointer<T>::value>
align_(size_t __align, T __ptr) noexcept
{
  const auto __intptr = reinterpret_cast<uintptr_t>(__ptr);
  const auto __aligned = align_(__align, __intptr);
  return reinterpret_cast<T>(__aligned);
}

[rotu]

I didn't say I'm happy with SFINAE, just that it resolves this concern :-). I think SFINAE here is overkill, and the issue here is easily fixed without it.

[jrtc27]

Well, personally I think having two different names for basically the same thing is more confusing and less elegant, but if that's what you'd prefer then sure.

[rotu]

It is more confusing and less elegant to have two implementations. Either way, you have two implementations. SFINAE versus two different names might be the right call in better-designed code. If you think it's how you want to do it, then go for it! Still, it's an internal API call, and hopefully one that will be removed soon that a larger rewrite is in order, so don't worry too much either way.

[dodsonmg]

implemented split align_int_ and align_ptr_ functions.

Thanks for the discussion.

For some background on why I'm trying to refactor this tiny bit of code: I'm trying to build ROS2 on a system which, amongst other things, has very specific rules for pointer provenance. This particular piece of code was problematic and we tried to find a generic solution that maintainers might be happy to merge upstream, rather than just holding on to our own fork with specific work-arounds.

[rotu]

Thanks for the background!

Do note that there may be some issues with pointer alignment today. The deserialization code can already access doubles and uint64s through unaligned pointers today (which is UB, though I haven't seen it be an actual problem yet). If your system is picky about pointers, it might be picky about that too.

[jrtc27]

It's only picky about unaligned loads/stores of pointers themselves. Normal integers/floats adhere to the base architecture's restrictions (there are version based on MIPS, RISC-V and AArch64 which all differ there, and some make it implementation-defined).

[rotu]

I'm sorry it took so long. I assumed @eboasson would merge this. LGETM!

[eboasson]

@rotu @jrtc27 @dodsonmg: my apologies, I assumed @rotu was taking care of it and that he would interpret my absence from the discussion as a silent support of his approval. Assumptions ... anyway here it ends well :)

[rotu]

:-p I don't know what I'm doing and I am scared to break everything

[dodsonmg]

No worries. Thanks very much for considering and working with us.