4 ms·

The code overall is pretty clever, but at the core of all this they completely ignore strict aliasing to dump stuff into a char* buffer indiscriminately... Look

The code overall is pretty clever, but at the core of all this they completely ignore strict aliasing to dump stuff into a char* buffer indiscriminately... Look at this function: https://github.com/PlatformLab/NanoLog/blob/master/runtime/NanoLogCpp17.h#L742 https://github.com/PlatformLab/NanoLog/blob/master/runtime/N... T argument = *reinterpret_cast<T*>(*in); ... uint32_t stringBytes = *reinterpret_cast<uint32_t*>(*in); A total of 6 reinterpret_casts in that file alone. I didn't see any indication that you have to turn off strict aliasing to use this library (and would you really want to make all your remaining code slower just so your logging has better benchmark scores?). Which makes all this code UB as far as the C++ standard is concerned (no, their allocator does not magically placement-new the correct object types into the right places). They could just inspect their T values as char* and/or memcpy them around instead of all this pointer aliasing (well, as long as their values are trivial types - I couldn't figure out whether you can log non-trivial stuff here?) at virtually no additional cost. Edit: And this right here convinces me that I definitely wouldn't want to use this library in production... Adding a volatile to fix a threading bug is just a big no-no. https://github.com/PlatformLab/NanoLog/commit/e9691246ede6dacbc4be2a56409f9f982856e3eb#diff-7d79f738e30bcbecebcf9183a9c00ed1R749 https://github.com/PlatformLab/NanoLog/commit/e9691246ede6da...

Leszek 7y ago
You'd think so, but in fact reinterpret_cast from char* (and unsigned char, and std::byte) is explicitly allowed by the type aliasing rules.
- MauranKilom 7y ago
  To the best of my knowledge, casting to char* is totally fine (inspecting an object as bytes) under certain constraints. What is not fine is pretending that an object lives at a position in memory where it does not (i.e. treating bytes of memory as some object through a reinterpret_cast away from char* ). Edit: To clarify, casting away from char* is of course allowed if you cast to whatever object type actually lives at the pointed-to location.
  - detaro 7y ago
    If I remember correctly, it is fine if there actually was an object of that type (or a "similar" type) in that location. So casting to char* and back is fine, casting to char and then to int-type to inspect multiple bytes is fine as long as alignment plays correctly, ... It seems like the function only cast to T* if the input pointer hasn't been changed (the paths that do modify it return early), so there's a value there?
    - gpderetta 7y ago
      > casting to char and then to int-type to inspect multiple bytes is fine as long as alignment plays correctly It is only fine if there were the same exact int types at that address. Remember, UB usually isn't the reinterpret_cast but the dereference.
      - detaro 7y ago
        I thought the int-types had a special exception making that always possible (if there's a bunch of bytes, you can turn them into an int), but I could very well be wrong on that detail.
        gpderetta 7y ago
        Char is the only exception. And now std::byte IIRC
    - MauranKilom 7y ago
      > it is fine if there actually was an object of that type (or a "similar" type) in that location. That is correct. See my other nearby reply. > So casting to char* and back is fine Indeed. > casting to char and then to int-type to inspect multiple bytes is fine as long as alignment plays correctly, ... Only if the place in memory you are pointing to actually has a live int object. In particular, inspecting the byte representation of an object is fine (including copying the bytes elsewhere, which is why they could just use memcpy instead of reinterpret_cast). See also http://eel.is/c++draft/basic.types#2 http://eel.is/c++draft/basic.types#2 and surroundings.
Iwan-Zotow 7y ago
char* is universal memory format, you could specialize from that into anything you want
- MauranKilom 7y ago
  That's not how the C++ object model works. Objects in C++ have clearly defined lifetimes. If you reinterpret_cast<int* >(someCharBuffer) and then dereference that, you have Undefined Behavior unless an int object is alive at that exact location. You can do int value[2] = {0, 0}; char* ptr = reinterpret_cast<char*>(value) + sizeof(int); (*reinterpret_cast<int*>(ptr))++; or (given knowledge about compiler padding): struct XY { double x; int y; }; XY xy = {}; char* ptr = reinterpret_cast<char*>(&xy) + sizeof(double); (*reinterpret_cast<int*>(ptr))++; or even (placement new): unsigned char* buf = new unsigned char[12]; buf += 4; new (buf) int(0); // Create an int object in the buffer. (*reinterpret_cast<int*>(buf))++; delete[] buf; But in each case an int object has had its lifetime begin before you can treat the bytes in memory like an int. Everything else is UB.
  - bonzini 7y ago
    You cannot do the last unless you know that the argument of placement new is suitably aligned to receive an int object, however.
gpderetta 7y ago
> Adding a volatile to fix a threading bug is just a big no-no. The fence instructions also don't make much sense. Not sure why one would not use std::atomic in a c++17 only library.
jnordwick 7y ago
What's your problem with the volatile? Using it to prevent register caching of or a value or prevent optimizing out repeated reads is perfectly fine. That is how it is supposed to be used. The bug was that one thread was reading the gcc thought it wouldn't be update between reads. It is even used properly by grabbing a copy of the volatile value instead of reading it repeatedly. edit: I also don't understand your aliasing issues. A `char*` can point anything and you are allowed to cast back out of it to the correct type. It is casting to int because the string is prepended by its length (the var names are "size" and "nibble").
- MauranKilom 7y ago
  > Using it to prevent register caching of or a value or prevent optimizing out repeated reads is perfectly fine. That is how it is supposed to be used. The code has a race condition, plain and simple. volatile does not lead to thread synchronization, and that function is (as gcc correctly identified) nonsensical in a single-threaded world. The code expects that a different thread writes to the variable at the same time that this thread might read from it - that's the definition of a race condition, which is straight up UB (volatile or not). You are trying to reason about compiler optimizations from the perspective of the machine and memory model you know ("prevent register caching of a value" etc.). That's wrong though - compiler optimizations operate on the abstract machine that C++ is defined on. In the abstract machine, this code has a race condition (the absence of which is guaranteed to compilers by the C++ standard) and so optimizations could easily violate whatever mental model you are using to conclude the code is fine. The code will likely break again some time in the future, just like it broke this time for a newer compiler version. Or maybe it compiles to subtly broken code right now, who knows! > I also don't understand your aliasing issues. A `char* ` can point anything and you are allowed to cast back out of it to the correct type. int is not the "correct type" because no int object lives at the pointed-to location after the cast. They could just memcpy and everything would be fine, but writing to/reading from type aliased pointers (other than char* and friends, in specific circumstances) is UB. Here is a good description (in the context of C, not C++) of strict aliasing: https://stackoverflow.com/questions/98650/what-is-the-strict-aliasing-rule https://stackoverflow.com/questions/98650/what-is-the-strict...

Leszek 7y ago

You'd think so, but in fact reinterpret_cast from char* (and unsigned char, and std::byte) is explicitly allowed by the type aliasing rules.

MauranKilom 7y ago
To the best of my knowledge, casting to char* is totally fine (inspecting an object as bytes) under certain constraints. What is not fine is pretending that an object lives at a position in memory where it does not (i.e. treating bytes of memory as some object through a reinterpret_cast away from char* ). Edit: To clarify, casting away from char* is of course allowed if you cast to whatever object type actually lives at the pointed-to location.
- detaro 7y ago
  If I remember correctly, it is fine if there actually was an object of that type (or a "similar" type) in that location. So casting to char* and back is fine, casting to char and then to int-type to inspect multiple bytes is fine as long as alignment plays correctly, ... It seems like the function only cast to T* if the input pointer hasn't been changed (the paths that do modify it return early), so there's a value there?
  - gpderetta 7y ago
    > casting to char and then to int-type to inspect multiple bytes is fine as long as alignment plays correctly It is only fine if there were the same exact int types at that address. Remember, UB usually isn't the reinterpret_cast but the dereference.
    - detaro 7y ago
      I thought the int-types had a special exception making that always possible (if there's a bunch of bytes, you can turn them into an int), but I could very well be wrong on that detail.
      - gpderetta 7y ago
        Char is the only exception. And now std::byte IIRC
  - MauranKilom 7y ago
    > it is fine if there actually was an object of that type (or a "similar" type) in that location. That is correct. See my other nearby reply. > So casting to char* and back is fine Indeed. > casting to char and then to int-type to inspect multiple bytes is fine as long as alignment plays correctly, ... Only if the place in memory you are pointing to actually has a live int object. In particular, inspecting the byte representation of an object is fine (including copying the bytes elsewhere, which is why they could just use memcpy instead of reinterpret_cast). See also http://eel.is/c++draft/basic.types#2 http://eel.is/c++draft/basic.types#2 and surroundings.

Iwan-Zotow 7y ago

char* is universal memory format, you could specialize from that into anything you want

MauranKilom 7y ago
That's not how the C++ object model works. Objects in C++ have clearly defined lifetimes. If you reinterpret_cast<int* >(someCharBuffer) and then dereference that, you have Undefined Behavior unless an int object is alive at that exact location. You can do int value[2] = {0, 0}; char* ptr = reinterpret_cast<char*>(value) + sizeof(int); (*reinterpret_cast<int*>(ptr))++; or (given knowledge about compiler padding): struct XY { double x; int y; }; XY xy = {}; char* ptr = reinterpret_cast<char*>(&xy) + sizeof(double); (*reinterpret_cast<int*>(ptr))++; or even (placement new): unsigned char* buf = new unsigned char[12]; buf += 4; new (buf) int(0); // Create an int object in the buffer. (*reinterpret_cast<int*>(buf))++; delete[] buf; But in each case an int object has had its lifetime begin before you can treat the bytes in memory like an int. Everything else is UB.
- bonzini 7y ago
  You cannot do the last unless you know that the argument of placement new is suitably aligned to receive an int object, however.

gpderetta 7y ago

> Adding a volatile to fix a threading bug is just a big no-no. The fence instructions also don't make much sense. Not sure why one would not use std::atomic in a c++17 only library.

jnordwick 7y ago

What's your problem with the volatile? Using it to prevent register caching of or a value or prevent optimizing out repeated reads is perfectly fine. That is how it is supposed to be used. The bug was that one thread was reading the gcc thought it wouldn't be update between reads. It is even used properly by grabbing a copy of the volatile value instead of reading it repeatedly. edit: I also don't understand your aliasing issues. A `char*` can point anything and you are allowed to cast back out of it to the correct type. It is casting to int because the string is prepended by its length (the var names are "size" and "nibble").

MauranKilom 7y ago
> Using it to prevent register caching of or a value or prevent optimizing out repeated reads is perfectly fine. That is how it is supposed to be used. The code has a race condition, plain and simple. volatile does not lead to thread synchronization, and that function is (as gcc correctly identified) nonsensical in a single-threaded world. The code expects that a different thread writes to the variable at the same time that this thread might read from it - that's the definition of a race condition, which is straight up UB (volatile or not). You are trying to reason about compiler optimizations from the perspective of the machine and memory model you know ("prevent register caching of a value" etc.). That's wrong though - compiler optimizations operate on the abstract machine that C++ is defined on. In the abstract machine, this code has a race condition (the absence of which is guaranteed to compilers by the C++ standard) and so optimizations could easily violate whatever mental model you are using to conclude the code is fine. The code will likely break again some time in the future, just like it broke this time for a newer compiler version. Or maybe it compiles to subtly broken code right now, who knows! > I also don't understand your aliasing issues. A `char* ` can point anything and you are allowed to cast back out of it to the correct type. int is not the "correct type" because no int object lives at the pointed-to location after the cast. They could just memcpy and everything would be fine, but writing to/reading from type aliased pointers (other than char* and friends, in specific circumstances) is UB. Here is a good description (in the context of C, not C++) of strict aliasing: https://stackoverflow.com/questions/98650/what-is-the-strict-aliasing-rule https://stackoverflow.com/questions/98650/what-is-the-strict...