Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes Builds where the C++ runtime library does not have std::put_time() #2439

Merged
merged 3 commits into from
Jan 26, 2023
Merged

Fixes Builds where the C++ runtime library does not have std::put_time() #2439

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c13ae26
Fixes Builds where the CXX runtime library does not have sdt::put_tim…
jlsantiago0 Aug 22, 2022
33c92e6
Fix opy paste mishap.
jlsantiago0 Aug 22, 2022
8f1deba
Fix typo.
jlsantiago0 Aug 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,15 @@ CheckGCCAtomicIntrinsics()
include(CheckCXXAtomic)
CheckCXXAtomic()

# Check for sd::put_time():
maxsharabayko marked this conversation as resolved.
Show resolved Hide resolved
# Sets:
# HAVE_CXX_STD_PUT_TIME
include(CheckCXXStdPutTime)
CheckCXXStdPutTime()
if (HAVE_CXX_STD_PUT_TIME)
add_definitions(-DHAVE_CXX_STD_PUT_TIME=1)
endif()

if (DISABLE_CXX11)
set (ENABLE_CXX11 0)
elseif( DEFINED ENABLE_CXX11 )
Expand Down Expand Up @@ -1070,6 +1079,11 @@ elseif (HAVE_LIBATOMIC AND HAVE_LIBATOMIC_COMPILES_STATIC)
if (srt_libspec_static)
target_link_libraries(${TARGET_srt}_static PUBLIC atomic)
endif()
elseif (LINUX AND HAVE_LIBATOMIC AND HAVE_LIBATOMIC_COMPILES)
# This is a workaround for some older Linux Toolchains.
if (srt_libspec_static)
target_link_libraries(${TARGET_srt}_static PUBLIC atomic)
endif()
Comment on lines +1082 to +1086
Copy link
Collaborator

@maxsharabayko maxsharabayko Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlsantiago0
Please confirm if this is the intended change.

P.S. Sorry, it's been quite some time since you opened the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't all these conditions be consolidated so that this target_link_libraries call happens only once?

Copy link
Contributor Author

@jlsantiago0 jlsantiago0 Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlsantiago0 Please confirm if this is the intended change.

P.S. Sorry, it's been quite some time since you opened the PR.

Yes I was fixing the build with Ubuntu14 and the Clang compiler that installed with apt get install clang . That compiler uses the GCC Standard C++ library with partial support for C++11. The __STDC_VERSION__ is set to 201112 so C++11 is indicated, but it does not seem to be complete. It seems to have all of TR1 and most of C++11, but it does not have std::puttime(). So this checks for it programatically. BTW. This also fixes the builds on some of the pre 10.9 MacOS SDKs which are in a similar state.

Also that toolchain on Ubuntu14 requires linking of the atomic library when compiling statically. So it is safe to link in the atomic library if the tests show that it is linkable when compiling statically. Because I was unable to figure out a way to test whether it was required or not in every situation. But def needed for the Clang toolchain anyway on this platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IRT @ethouris questions. I was trying to minimize the changes to implement this check. I assume that the target_link_libraries() stuff could be consolidated. But It would require some restructuring I think anyway. And was not related to this particular issue i was trying to solve.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, it looks kinda weird once you merge them. It's something like:

if (HAVE_LIBATOMIC and srt_libspec_static)
    if (HAVE_LIBATOMIC_COMPILES_STATIC OR (LINUX AND HAVE_LIBATOMIC_COMPILES))
         target_link_libraries...
    endif()
endif()

Meaning, you add libatomic to the static SRT library target (though not to shared one - in which case it might be expected to be added indirectly?) in case when you have statically compiled-in atomic library or if you have also the shared one but on Linux (although not for, for example, Android, for which the first of the condition branches was done). What exactly should then happen in case when you have a shared atomic library and you request to compile the static library? Does HAVE_LIBATOMIC_COMPILES imply HAVE_LIBATOMIC_COMPILES_STATIC? Can adding this library be in specific situations superfluous?

Copy link
Contributor Author

@jlsantiago0 jlsantiago0 Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah i found that on some versions of linux, LIBATOMIC Cannot be linked to a static application, but it can be linked with a static SRT library. The HAVE_LIBATOMIC_COMPILES_STATIC is set if we could link LIBATOMIC with an application built with -static. The application itself is static. If that works, then we can unconiditionally link in libatomic here. The other check (LINUX AND HAVE_LIBATOMIC_COMPILES) means that we are on LINUX and LIBATOMIC can be compiled and linked into an application. It just may or may not be able to link into a static application. This usually means that the host does not have a libatomic.a, but only libatomic.so or that there is some unexpected dependency on libatomic.a that I couldnt figure out. This can happen using the libatomic with clang when the sanitizers are enabled for instance. And in that case it is safe to let libatomic be a link dependency.

Most of this has been worked out now with the newer toolchains. It is just a bit painful for GCC<6 and Clang<4.2ish i think or if you are using the sanitizers. I tend to run the SRT library through the sanitizer builds on both GCC and Clang so I find weird things like this.

In the case I was specifically fixing here: It was Ubuntu14, gcc (Ubuntu 4.8.4-2ubuntu1~14.04.4) 4.8.4, and clang version 3.4.2 (tags/RELEASE_34/dot2-final)

In particular, this C++ runtime has most of C++11, except it does not have std::put_time() and when building with the Clang toolchain on this platform we need to specify a link dependency for libatomic for the SRT static library. It does not have a static version of libatomic.a, so the static application build fails, but the static srt library is usable as long as we specify the libatomic link dependency. I have had similar issues with other older platforms as well. I dont remember all of them since it has been while since i looked at this issue.

endif()

# Cygwin installs the *.dll libraries in bin directory and uses PATH.
Expand Down
18 changes: 7 additions & 11 deletions apps/statswriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@
#include "netinet_any.h"
#include "srt_compat.h"

// Note: std::put_time is supported only in GCC 5 and higher
#if !defined(__GNUC__) || defined(__clang__) || (__GNUC__ >= 5)
#define HAS_PUT_TIME
#endif

using namespace std;

Expand Down Expand Up @@ -100,7 +96,7 @@ string srt_json_cat_names [] = {
"recv"
};

#ifdef HAS_PUT_TIME
#ifdef HAVE_CXX_STD_PUT_TIME
// Follows ISO 8601
std::string SrtStatsWriter::print_timestamp()
{
Expand All @@ -125,10 +121,10 @@ std::string SrtStatsWriter::print_timestamp()

// This is a stub. The error when not defining it would be too
// misleading, so this stub will work if someone mistakenly adds
// the item to the output format without checking that HAS_PUT_TIME.
// the item to the output format without checking that HAVE_CXX_STD_PUT_TIME
string SrtStatsWriter::print_timestamp()
{ return "<NOT IMPLEMENTED>"; }
#endif // HAS_PUT_TIME
#endif // HAVE_CXX_STD_PUT_TIME


class SrtStatsJson : public SrtStatsWriter
Expand Down Expand Up @@ -170,7 +166,7 @@ class SrtStatsJson : public SrtStatsWriter
output << pretty_tab << quotekey("sid") << sid;

// Extra Timepoint is also displayed manually
#ifdef HAS_PUT_TIME
#ifdef HAVE_CXX_STD_PUT_TIME
// NOTE: still assumed SSC_GEN category
output << "," << pretty_cr << pretty_tab
<< quotekey("timepoint") << quote(print_timestamp());
Expand Down Expand Up @@ -246,7 +242,7 @@ class SrtStatsCsv : public SrtStatsWriter
// Header
if (!first_line_printed)
{
#ifdef HAS_PUT_TIME
#ifdef HAVE_CXX_STD_PUT_TIME
output << "Timepoint,";
#endif
output << "Time,SocketID";
Expand All @@ -260,10 +256,10 @@ class SrtStatsCsv : public SrtStatsWriter
}

// Values
#ifdef HAS_PUT_TIME
#ifdef HAVE_CXX_STD_PUT_TIME
// HDR: Timepoint
output << print_timestamp() << ",";
#endif // HAS_PUT_TIME
#endif // HAVE_CXX_STD_PUT_TIME

// HDR: Time,SocketID
output << mon.msTimeStamp << "," << sid;
Expand Down
57 changes: 57 additions & 0 deletions scripts/CheckCXXStdPutTime.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
#
# SRT - Secure, Reliable, Transport Copyright (c) 2022 Haivision Systems Inc.
#
# This Source Code Form is subject to the terms of the Mozilla Public License,
# v. 2.0. If a copy of the MPL was not distributed with this file, You can
# obtain one at http://mozilla.org/MPL/2.0/.
#

# Check for C++11 std::put_time().
#
# Sets:
# HAVE_CXX_STD_PUT_TIME

include(CheckCSourceCompiles)

function(CheckCXXStdPutTime)

unset(HAVE_CXX_STD_PUT_TIME CACHE)

set(CMAKE_TRY_COMPILE_TARGET_TYPE EXECUTABLE) # CMake 3.6

unset(CMAKE_REQUIRED_FLAGS)
unset(CMAKE_REQUIRED_LIBRARIES)
unset(CMAKE_REQUIRED_LINK_OPTIONS)

set(CheckCXXStdPutTime_CODE
"
#include <iostream>
#include <iomanip>
#include <ctime>
int main(void)
{
const int result = 0;
std::time_t t = std::time(nullptr);
std::tm tm = *std::localtime(&t);
std::cout
<< std::put_time(&tm, \"%FT%T\")
<< std::setfill('0')
<< std::setw(6)
<< std::endl;
return result;
}
"
)

# NOTE: Should we set -std or use the current compiler configuration.
# It seems that the top level build does not track the compiler
# in a consistent manner. So Maybe we need this?
set(CMAKE_REQUIRED_FLAGS "-std=c++11")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using particular C++ standard is quite a complicated thing; not sure what these "required" flags do, but whether C++11 standard is required to compile the C++ library, it's also checked elsewhere. This check upon confirmation should add its brick to requiring C++11 for compiling SRT library, but this is only one of the pieces and should be checked toghether with the others, for example, to check conflict with --disable-c++11 flag.

Copy link
Contributor Author

@jlsantiago0 jlsantiago0 Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CXX_STANDARD and CXX_STANDARD_REQUIRED is available to underlying try_compile(), which is used by check_cxx_source_compiles(), but is not available before CMake v3.8. This project has a minimum required CMake v2.8.x. So I couldnt use it. Otherwise I would have.

The CMAKE_REQUIRED_FLAGS are used by check_cxx_source_compiles() when trying to compile and is also of a course a local variable in the function. So it will not bleed over to the parent scope. You can use this method for instance to try and deduce what flags are needed to compile a bit of code. check_cxx_source_compiles() will use the current state of the CMAKE_CXX_FLAGS as well as any prior compiler configurations in addition to the CMAKE_REQUIRED_* , but since in the top level sometimes these are added to target configurations, I could not rely on them being set in the top level toolchain configuration because the toolchain configuration is not always consistently configured globally, but some of the configuration is only added at target configuration. So this at least determines if this function is callable when -std=c++11 is set. Someone could add more conditions before calling add_definitions(-DHAVE_CXX_STD_PUT_TIME=1) in the top level project should that become necessary.

This resolves some build breakage and should not break anything since the current source file that uses that definition is only built when ENABLE_CXX11 is set.


# Check that the compiler can build the std::put_time() example:
message(STATUS "Checking for C++ 'std::put_time()':")
check_cxx_source_compiles(
"${CheckCXXStdPutTime_CODE}"
HAVE_CXX_STD_PUT_TIME)

endfunction(CheckCXXStdPutTime)
1 change: 1 addition & 0 deletions scripts/ShowProjectConfig.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ function(ShowProjectConfig)
" HAVE_GCCATOMIC_INTRINSICS_REQUIRES_LIBATOMIC: ${HAVE_GCCATOMIC_INTRINSICS_REQUIRES_LIBATOMIC}\n"
" HAVE_CXX_ATOMIC: ${HAVE_CXX_ATOMIC}\n"
" HAVE_CXX_ATOMIC_STATIC: ${HAVE_CXX_ATOMIC_STATIC}\n"
" HAVE_CXX_STD_PUT_TIME: ${HAVE_CXX_STD_PUT_TIME}\n"
" Project Configuration:\n"
" ENABLE_DEBUG: ${ENABLE_DEBUG}\n"
" ENABLE_CXX11: ${ENABLE_CXX11}\n"
Expand Down