Skip to content

Commit

Permalink
Merge bitcoin#18088: build: ensure we aren't using GNU extensions
Browse files Browse the repository at this point in the history 
0ae8f18 build: add -Wgnu to compile flags (fanquake)
3a0fd77 Remove use of non-standard zero variadic macros (Ben Woosley)
49f6178 Drop unused LOG_TIME_MICROS helper (Ben Woosley)
5d49999 prevector: Avoid unnamed struct, which is a GNU extension (DesWurstes)

Pull request description:

  Since we [started using](bitcoin#7165) the `ax_cxx_compile_stdcxx.m4` macro we've been passing `[noext]` to indicate that we don't want to use an extended mode, i.e GNU extensions. Speaking to Cory he clarified that the intention was to "require only vanilla c++11 and turn _off_ extension support so they would fail to compile".

  However in the codebase we are currently making use of some GNU extensions. We should either remove there usage, or at least amend our CXX compiler checks. I'd prefer the former.

  #### anonymous structs
  ```bash
  ./prevector.h:153:9: warning: anonymous structs are a GNU extension [-Wgnu-anonymous-struct]
          struct {
  ```

  This is fixed in bitcoin@b849212.

  #### variadic macros

  ```bash
  ./undo.h:57:50: warning: must specify at least one argument for '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]
              ::Unserialize(s, VARINT(nVersionDummy));
  ```

  This is taken care of in bitcoin#18087.

  The `LOG_TIME_*` macros introduced in bitcoin#16805 make use of a [GNU extension](https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html).

  ```bash
  In file included from validation.cpp:22:
  ./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
      BCLog::Timer<std::chrono::milliseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
                                                                                                    ^
  ./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  ./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  ./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  ./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  ./logging/timer.h:101:92: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
      BCLog::Timer<std::chrono::seconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
                                                                                             ^
  6 warnings generated.
  ```

  This is fixed in 081a0ab and 612e8e1.

  #### prevention
  To ensure that usage doesn't creep back in we can add [`-Wgnu`](https://clang.llvm.org/docs/DiagnosticsReference.html#wgnu) to our compile time flags, which will make Clang warn whenever it encounters GNU extensions.

  This would close bitcoin#14130.
  Also related to bitcoin#17230, where it's suggested we use a GNU extension, the `gnu::pure` attribute.

ACKs for top commit:
  practicalswift:
    ACK 0ae8f18 -- diff looks correct
  MarcoFalke:
    ACK 0ae8f18
  vasild:
    utACK 0ae8f18
  dongcarl:
    ACK 0ae8f18

Tree-SHA512: c517404681ef8edf04c785731d26105bac9f3c9c958605aa24cbe399c649e7c5ee0c4aa8e714fd2b2d335e2fbea4d571e09b0dec36678ef871f0a6683ba6bb7f
  • Loading branch information
@fanquake @vijaydasmp
fanquake authored and vijaydasmp committed Mar 3, 2023
1 parent cb3be0f commit afdb731
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 23 deletions.
1 change: 1 addition & 0 deletions configure.ac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@ fi
if test "x$CXXFLAGS_overridden" = "xno"; then
AX_CHECK_COMPILE_FLAG([-Wall],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wall"],,[[$CXXFLAG_WERROR]])
AX_CHECK_COMPILE_FLAG([-Wextra],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wextra"],,[[$CXXFLAG_WERROR]])
AX_CHECK_COMPILE_FLAG([-Wgnu],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wgnu"],,[[$CXXFLAG_WERROR]])
AX_CHECK_COMPILE_FLAG([-Wformat],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat"],,[[$CXXFLAG_WERROR]])
AX_CHECK_COMPILE_FLAG([-Wvla],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]])
AX_CHECK_COMPILE_FLAG([-Wshadow-field],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wshadow-field"],,[[$CXXFLAG_WERROR]])
Expand Down
10 changes: 4 additions & 6 deletions src/logging/timer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,10 @@ class Timer
} // namespace BCLog


#define LOG_TIME_MICROS(end_msg, ...) \
BCLog::Timer<std::chrono::microseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
#define LOG_TIME_MILLIS(end_msg, ...) \
BCLog::Timer<std::chrono::milliseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
#define LOG_TIME_SECONDS(end_msg, ...) \
BCLog::Timer<std::chrono::seconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
#define LOG_TIME_MILLIS_WITH_CATEGORY(end_msg, log_category) \
BCLog::Timer<std::chrono::milliseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, log_category)
#define LOG_TIME_SECONDS(end_msg) \
BCLog::Timer<std::chrono::seconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg)


#endif // BITCOIN_LOGGING_TIMER_H
24 changes: 12 additions & 12 deletions src/prevector.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ class prevector {
struct {
char* indirect;
size_type capacity;
};
} indirect_contents;
};
#pragma pack(pop)
alignas(char*) direct_or_indirect _union = {};
Expand All @@ -163,8 +163,8 @@ class prevector {

T* direct_ptr(difference_type pos) { return reinterpret_cast<T*>(_union.direct) + pos; }
const T* direct_ptr(difference_type pos) const { return reinterpret_cast<const T*>(_union.direct) + pos; }
T* indirect_ptr(difference_type pos) { return reinterpret_cast<T*>(_union.indirect) + pos; }
const T* indirect_ptr(difference_type pos) const { return reinterpret_cast<const T*>(_union.indirect) + pos; }
T* indirect_ptr(difference_type pos) { return reinterpret_cast<T*>(_union.indirect_contents.indirect) + pos; }
const T* indirect_ptr(difference_type pos) const { return reinterpret_cast<const T*>(_union.indirect_contents.indirect) + pos; }
bool is_direct() const { return _size <= N; }

void change_capacity(size_type new_capacity) {
Expand All @@ -182,17 +182,17 @@ class prevector {
/* FIXME: Because malloc/realloc here won't call new_handler if allocation fails, assert
success. These should instead use an allocator or new/delete so that handlers
are called as necessary, but performance would be slightly degraded by doing so. */
_union.indirect = static_cast<char*>(realloc(_union.indirect, ((size_t)sizeof(T)) * new_capacity));
assert(_union.indirect);
_union.capacity = new_capacity;
_union.indirect_contents.indirect = static_cast<char*>(realloc(_union.indirect_contents.indirect, ((size_t)sizeof(T)) * new_capacity));
assert(_union.indirect_contents.indirect);
_union.indirect_contents.capacity = new_capacity;
} else {
char* new_indirect = static_cast<char*>(malloc(((size_t)sizeof(T)) * new_capacity));
assert(new_indirect);
T* src = direct_ptr(0);
T* dst = reinterpret_cast<T*>(new_indirect);
memcpy(dst, src, size() * sizeof(T));
_union.indirect = new_indirect;
_union.capacity = new_capacity;
_union.indirect_contents.indirect = new_indirect;
_union.indirect_contents.capacity = new_capacity;
_size += N + 1;
}
}
Expand Down Expand Up @@ -318,7 +318,7 @@ class prevector {
if (is_direct()) {
return N;
} else {
return _union.capacity;
return _union.indirect_contents.capacity;
}
}

Expand Down Expand Up @@ -485,8 +485,8 @@ class prevector {
clear();
}
if (!is_direct()) {
free(_union.indirect);
_union.indirect = nullptr;
free(_union.indirect_contents.indirect);
_union.indirect_contents.indirect = nullptr;
}
}

Expand Down Expand Up @@ -538,7 +538,7 @@ class prevector {
if (is_direct()) {
return 0;
} else {
return ((size_t)(sizeof(T))) * _union.capacity;
return ((size_t)(sizeof(T))) * _union.indirect_contents.capacity;
}
}

Expand Down
10 changes: 5 additions & 5 deletions src/validation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2488,11 +2488,11 @@ bool CChainState::FlushStateToDisk(
LOCK(cs_LastBlockFile);
if (fPruneMode && (fCheckForPruning || nManualPruneHeight > 0) && !fReindex) {
if (nManualPruneHeight > 0) {
LOG_TIME_MILLIS("find files to prune (manual)", BCLog::BENCHMARK);
LOG_TIME_MILLIS_WITH_CATEGORY("find files to prune (manual)", BCLog::BENCHMARK);

FindFilesToPruneManual(g_chainman, setFilesToPrune, nManualPruneHeight);
} else {
LOG_TIME_MILLIS("find files to prune", BCLog::BENCHMARK);
LOG_TIME_MILLIS_WITH_CATEGORY("find files to prune", BCLog::BENCHMARK);

FindFilesToPrune(g_chainman, setFilesToPrune, chainparams.PruneAfterHeight());
fCheckForPruning = false;
Expand Down Expand Up @@ -2533,15 +2533,15 @@ bool CChainState::FlushStateToDisk(
}
// First make sure all block and undo data is flushed to disk.
{
LOG_TIME_MILLIS("write block and undo data to disk", BCLog::BENCHMARK);
LOG_TIME_MILLIS_WITH_CATEGORY("write block and undo data to disk", BCLog::BENCHMARK);

// First make sure all block and undo data is flushed to disk.
FlushBlockFile();
}

// Then update all block file information (which may refer to block and undo files).
{
LOG_TIME_MILLIS("write block index to disk", BCLog::BENCHMARK);
LOG_TIME_MILLIS_WITH_CATEGORY("write block index to disk", BCLog::BENCHMARK);

std::vector<std::pair<int, const CBlockFileInfo*> > vFiles;
vFiles.reserve(setDirtyFileInfo.size());
Expand All @@ -2561,7 +2561,7 @@ bool CChainState::FlushStateToDisk(
}
// Finally remove any pruned files
if (fFlushForPrune) {
LOG_TIME_MILLIS("unlink pruned files", BCLog::BENCHMARK);
LOG_TIME_MILLIS_WITH_CATEGORY("unlink pruned files", BCLog::BENCHMARK);

UnlinkPrunedFiles(setFilesToPrune);
}
Expand Down

0 comments on commit afdb731

Please sign in to comment.