CMake support #19

tehKaiN · 2018-07-16T09:31:54Z

Incomplete since I can only support Windows. Tested on MinGW with GCC 7.3 & DWARF2 EH

Fixes #12, at least for me ;)

tested on MinGW with GCC 7.3 & DWARF2 EH

thanm · 2018-07-16T12:48:45Z

I tested this on Linux and it seems to work ok. It would be nice to have more complete support (e.g. check target, shared version of library) but those can be added later I suppose.

One thing that I think would be worth fixing is that the generated files are emitted into the source directory instead of the build directory -- I don't think this is a good idea. It is a minor "POLA violation" (so to speak) and makes it difficult to use same source dir for multiple parallel builds. Better to emit into CMAKE_CURRENT_BINARY_DIR and then add a -I... to the build flags.

tehKaiN · 2018-07-17T06:37:37Z

That should do it. I've also added minor comment regarding C/CXX flags. I'm quite amazed that this works on linux since I haven't even tried to make it so. ;)

ianlancetaylor

Would you be willing to sign the Google CLA at https://cla.developers.google.com/about/google-individual ?

Or have you signed an FSF copyright assignment?

If we add this change, how will people use it with cmake? Sorry if that is an obvious question--I don't know how cmake works.

ianlancetaylor · 2018-07-17T14:13:07Z

.gitignore

@@ -3,3 +3,8 @@
 *.lo
 *.a
 *.la
+
+backtrace-supported\.h


Does cmake support building in a separate directory, so that the sources can be read-only? If so, let's not modify .gitignore here.

I'm not really that familiar with autotools but I've built this library using ./configure and make and it generated headers in source directory, as well as .a files. So I thought having .a and not .h in gitignore was a bug, not a feature. If I'm wrong I'll remove this line.

The expectation is that people will run the equivalent of

mkdir x cd x ../libbacktrace/configure

rather than running .configure. It's OK to run ./configure, but it shouldn't be necessary, and we need to make sure that running configure from a different directory works without modifying the source directory.

I've reverted my changes in 28d7445

Does cmake support building in a separate directory, so that the sources can be read-only? If so, let's not modify .gitignore here.

Yes, via this command:

cmake -S <source-dir> -B <build-dir> [optional_arguments] # configure step cmake --build <build-dir> # build step

The old CMake also allows separate build via:

mkdir build cd build cmake <path-to-source>

ianlancetaylor · 2018-07-17T14:15:58Z

CMakeLists.txt

+	# Assume multi-threaded environment
+	set(BACKTRACE_SUPPORTS_THREADS 1)
+
+	# Assume ELF/DWARF, meaning that BACKTRACE_SUPPORTS_DATA is hard-coded on.


This seems like an unfortunate assumption, since we have PE and XCOFF support, and we really need Mach-O support. Do people not use cmake on those systems? Though I suppose we could also fix this up later.

I vote for adding things later, since I've never worked with XCOFF or Mach-O files and platforms supporting them. Therefore I can't add support to them, so I guess there's little more options than putting notice in README that CMake support is in it's infancy.

fixed in 9f5df7e

ianlancetaylor · 2018-07-17T14:17:21Z

backtrace-supported.h.cmake

@@ -0,0 +1,51 @@
+/* backtrace-supported.h.cmake -- Whether stack backtrace is supported.


Is there some way we could generate this file from backtrace-supported.h.in, so that we don't have the unnecessary duplication?

I've checked right now and it's entirely possible. I'll do proper changes right away. config.h.in uses autotools-specific syntax, so unfortunately we need CMake variant.

ianlancetaylor · 2018-07-17T14:18:02Z

config.h.cmake

@@ -0,0 +1,58 @@
+/* config.h.cmake */


Similarly, can we generate this from config.h.in?

config.h.in is useless from the perspective of cmake, as all of the entries in it are of the form #undef .....

A separate pre-processing step would be needed to transform the file into something cmake can use.

Does this work?

file(READ "config.h.in" original_config_h_in) string(REGEX REPLACE "#undef" "#cmakedefine" new_config_h_cmake "${original_config_h_in}") file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/config.h.cmake" "${new_config_h_cmake}") configure_file("${CMAKE_CURRENT_BINARY_DIR}/config.h.cmake" "${CMAKE_CURRENT_BINARY_DIR}/config.h")

tehKaiN · 2018-07-17T15:07:01Z

Regarding CMake usage. Typically, one creates build directory (name irrelevant) and there executes cmake .. for makefile generation and completes building with make, sometimes with make install target which gets automatically generated - it will install libs nicely on *nix OSes.

On Windows there's no standard way how make install should be done, but one can invoke it in manner such as following: cmake .. -G "MinGW Makefiles" -DCMAKE_INSTALL_PREFIX=c:/prg/cmake_install. This way make install will copy installed files to inc/lib/etc. inside folder specified by prefix, keeping filesystem relatively clean and easy to backup and/or manage when using different types/versions of compilers. Also, CMake may be configured to use such directory to look for libs when using find_library() command.

I'm new to this CLA business and I don't mind transferring all the rights for contributions to this repo, but currently I'm not aware of any drawbacks related to signing. Which one is preferred: FSF (I see them listed in LICENSE) or Google?

ianlancetaylor · 2018-07-17T16:22:25Z

The Google CLA is easier to sign. The FSF approach involves a transfer of copyright and takes longer. Some people trust the FSF more than they trust Google (although the CLA is not a copyright transfer and requires very little trust). It's up to you which path to follow. I just want to make sure there is no copyright question on these changes. Thanks.

tehKaiN · 2018-07-18T10:37:46Z

I don't really feel like creating Google account just to sign into their CLA, especially since I see no direct connection between Google and this repo (Okay, you're Google employee, but repo's LICENSE states that it's FSF property). FSF is truly a pain because of postage thingy, so that's a no-go too.

From what I've seen many repos use CLAHub which integrates nicely with GH. If this legal business is truly needed, could you please set it up using CLAHub for mine and future contributors' convenience? You can also integrate with pull requests so that there will be auto checks for CLA signage, so that's another benefit.

ianlancetaylor · 2018-07-18T14:11:42Z

At present the master sources for this library live in the GCC sources, which are covered by the FSF license. Google has an agreement with the FSF such that anything written by Google may be used by the FSF, so I feel moderately comfortable accepting code under the Google CLA. But I'm not comfortable using something else. Sorry.

I can understand the desire to not create a Google account. I really only suggested that because the Google CLA is simpler. For the FSF agreement, start by filling out the form below and mailing it to [email protected].

Yes, this legal stuff is unfortunately truly needed. There have been significant legal problems in the past due to the lack of careful use of licensing agreements. These have had real world consequences. See, for example, https://en.wikipedia.org/wiki/SCO%E2%80%93Linux_disputes , in particular the so-called SCOsource program for users of GNU/Linux.

Please email the following information to [email protected].

Please also CC the package maintainer(s). If you are uncomfortable
sharing any of this information with the maintainer(s), let us know.

We will send you the assignment form for your past and future changes.

PLEASE USE YOUR FULL NAME AS THE SUBJECT LINE OF THE MESSAGE.*

[What is the name of the program or package you're contributing to?]

[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]

[Do you have an employer who might have a basis to claim to own
your changes? Do you attend a school which might make such a claim?]

[For the copyright registration, what country are you a citizen of?]

[What year were you born?]

[Please write your email address here.]

[Please write your snail address here.]

[Which files have you changed so far, and which new files have you written
so far?]

tehKaiN · 2018-07-18T14:41:25Z

Done. Now we wait, I guess. Do I need to correct anything else in sources, or are we just waiting for legal thingy?

ianlancetaylor · 2018-07-18T16:44:25Z

.gitignore

@@ -2,4 +2,4 @@
 *.o
 *.lo
 *.a
-*.la


Not sure what is changing here--no reason for .gitignore to change at all.

ianlancetaylor · 2018-07-18T16:44:51Z

CMakeLists.txt

@@ -0,0 +1,109 @@
+cmake_minimum_required(VERSION 3.5)


I suppose we should add copyright header like the one in configure.ac.

ianlancetaylor · 2018-07-18T16:45:25Z

CMakeLists.txt

@@ -0,0 +1,109 @@
+cmake_minimum_required(VERSION 3.5)
+
+project(backtrace)


backtrace or libbacktrace? People usually say libbacktrace, and that is what autoconf uses. Should probably libbacktrace unless there is some reason not to.

I suggest to use libbacktrace, that way it will be different from the FindBacktrace CMake module associated with backtrace(3)

ianlancetaylor · 2018-07-18T16:46:02Z

cmake/libbacktraceConfig.cmake

@@ -0,0 +1,14 @@
+# Usage:


Add a copyright header.

ianlancetaylor · 2018-07-18T16:46:19Z

cmake/libbacktraceConfigVersion.cmake

@@ -0,0 +1,21 @@
+set(PACKAGE_VERSION "1.0.0")


Add a copyright header.

ianlancetaylor

What about the idea of generating config.h.cmake from config.h.in?

jcfr

Thanks for working on this @tehKaiN , I posted few comments and would also be happy to help move this forward.

jcfr · 2018-08-11T08:15:38Z

CMakeLists.txt

@@ -0,0 +1,109 @@
+cmake_minimum_required(VERSION 3.5)
+
+project(backtrace)


I suggest to use libbacktrace, that way it will be different from the FindBacktrace CMake module associated with backtrace(3)

jcfr · 2018-08-11T08:16:25Z

CMakeLists.txt

@@ -0,0 +1,109 @@
+cmake_minimum_required(VERSION 3.5)


Since this project is just being converted to CMake, I suggest to use the latest 3.12.1

I'll change CMake version, good point.

Regarding project name, I thought so too, but then it occurred to me that most CMake-packaged libs omit "lib" in their name - you type find_library(curl) instead of find_library(libcurl). Also, IIRC if you tell cmake to find_library(blah) it will look for libblah.a automatically, so find_library(libbacktrace) would result inf looking for liblibbacktrace.a. I also wonder, for which package is FindBacktrace designed? What do you think about my points?

CMakeLists.txt

jcfr · 2018-08-11T08:25:24Z

CMakeLists.txt

+)
+
+add_library(${PROJECT_NAME} ${sources} ${export_headers})
+include_directories(${PROJECT_NAME} ${CMAKE_CURRENT_BINARY_DIR})


Instead, prefer setting the variable set(CMAKE_INCLUDE_CURRENT_DIR ON) at the top of the project

jcfr · 2018-08-11T08:26:07Z

CMakeLists.txt

+	if(ZLIB_FOUND)
+		SET(HAVE_LIBZ 1)
+		SET(HAVE_ZLIB 1)
+		target_link_libraries(${PROJECT_NAME} z)


target_link_libraries(${PROJECT_NAME} ZLIB::ZLIB)

CMakeLists.txt

jcfr · 2018-08-11T08:35:08Z

CMakeLists.txt

+# Automake uses -frandom-seed initialized with file name of given file
+# but AFAIK it can't be done on CMake, so here's always same seed
+set(CMAKE_CXX_FLAGS "-DHAVE_CONFIG_H -funwind-tables -frandom-seed=mySeed -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wmissing-format-attribute -Wcast-qual -g -O2")
+set(CMAKE_C_FLAGS "-DHAVE_CONFIG_H -funwind-tables -frandom-seed=mySeed -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wmissing-format-attribute -Wcast-qual -g -O2")


Public, private and interface flags should be differentiated and directly associated with the library target. For example:

target_compile_options(${PROJECT_NAME} PRIVATE -funwind-tables -frandom-seed=mySeed -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wmissing-format-attribute -Wcast-qual)

If there are compile option that must be used by project using the library, they should be declared as INTERFACE. INTERFACE means that project linking against the libbacktrace imported target will transitively use the flag.

HAVE_CONFIG_H is not used in the project and should probably be removed.

-g -O2 flags are automatically listed when building the project with RELWITHDEBINFO build type. Is this a strong requirement ?

The default flags for each build type are available here:

https://github.com/Kitware/CMake/blob/da3dc2f0cfb8e2aed207c21e419a60525eea0c6f/Modules/Compiler/GNU.cmake#L42-L47

jcfr · 2018-08-11T08:40:36Z

CMakeLists.txt

+project(backtrace)
+
+# Automake uses -frandom-seed initialized with file name of given file
+# but AFAIK it can't be done on CMake, so here's always same seed


This should be possible, which file is expected to use as seed ?

According to automake each file was using it's own file for seed. I don't think there's strong reason for that, so I guess we can just use same seed for every file.

and it's redundant

RichardLangFromNZ · 2019-02-28T00:58:35Z

Just in case you guys hadn't come across it, there's a version of this project cloned out of the GCC sources in 2014 at https://github.com/ErwanLegrand/libbacktrace that includes cmake support.

tehKaiN · 2019-02-28T14:19:34Z

Just in case you guys hadn't come across it, there's a version of this project cloned out of the GCC sources in 2014 at https://github.com/ErwanLegrand/libbacktrace that includes cmake support.

I don't mind if someone merges that code and not mine, as long as linux/rpi/windows platforms are supported by it.

RichardLangFromNZ · 2019-02-28T18:08:34Z

Not suggesting that. It's not forked from this repo. so is inherently out of date, spent an hour trying to get his cmake script to work with this code without any success, then gave up as more trouble than it's worth. Was just drawing attention to it in case it was a useful reference.

advancedwebdeveloper · 2020-08-09T07:12:01Z

Incomplete since I can only support Windows. Tested on MinGW with GCC 7.3 & DWARF2 EH

Fixes #12, at least for me ;)

I am able to support something on Windows 10 (32bit; 64bit) with MSVC.

ianlancetaylor · 2020-08-10T00:48:11Z

@googlebot rescan

maowen · 2021-09-07T18:40:24Z

What's blocking this from being merged? Is it just the unresolved feedback from the code review (e.g. adding copyright headers to a few files) or is it something else?

I'd be willing to make the requested changes and get it moving along again.

ianlancetaylor · 2021-09-07T19:35:26Z

There are various unresolved comments as you can see.

I'm also somewhat concerned about keeping this up to date, as I don't know or use cmake. But I guess we can try it and see.

jonesmz · 2022-08-01T15:01:26Z

CMakeLists.txt

+			else()
+				set(ALLOC_FILE "alloc.c")
+			endif()
+			if(${ALLOC_FILE} STREQUAL "alloc.c")


no need to use ${} here, cmake will automatically unwrap the variable.

But you'd be better off moving the two SET() calls into the if(use_mmap_for_alloc) check anyway.

madebr · 2023-09-01T14:33:28Z

cmake/libbacktraceConfigVersion.cmake

@@ -0,0 +1,21 @@
+set(PACKAGE_VERSION "1.0.0")


Use CMakePackageConfigHelpers to automatically generate libbacktraceConfig.cmake (from libbacktraceConfig.cmake.in) and libbacktraceConfigVersion.cmake.

CMake support for MinGW toolchain

72ce7c6

tested on MinGW with GCC 7.3 & DWARF2 EH

generated headers ar now placed in build dir, added -frandom-seed note

b08cb33

ianlancetaylor requested changes Jul 17, 2018

View reviewed changes

Removed backtrace-supported.h.cmake

9f5df7e

Removed things I've added to .gitignore

28d7445

ianlancetaylor reviewed Jul 18, 2018

View reviewed changes

cmake/libbacktraceConfig.cmake

@@ -0,0 +1,14 @@

# Usage:

Copy link

Owner

ianlancetaylor Jul 18, 2018

Choose a reason for hiding this comment

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

Add a copyright header.

ianlancetaylor reviewed Jul 18, 2018

View reviewed changes

cmake/libbacktraceConfigVersion.cmake

@@ -0,0 +1,21 @@

set(PACKAGE_VERSION "1.0.0")

Copy link

Owner

ianlancetaylor Jul 18, 2018

Choose a reason for hiding this comment

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

Add a copyright header.

ianlancetaylor requested changes Jul 18, 2018

View reviewed changes

jcfr reviewed Aug 11, 2018

View reviewed changes

tehKaiN added 4 commits February 8, 2019 10:05

fix for builds outside source tree

cd6f82f

removed GLOB 'cuz it messes with backtrace-supported.h install

b3b27a5

and it's redundant

cmake compile & install works on ubuntu 16.04

fa2fe5e

fix for case where ${CMAKE_EXECUTABLE_FORMAT} isn't set to any value

e653f86

lifflander mentioned this pull request Jul 8, 2020

914 Fix stacktrace symbols when dladdr fails DARMA-tasking/vt#915

Merged

advancedwebdeveloper mentioned this pull request Aug 9, 2020

Unresolved path to libbacktrace, for gollvm project, on Windows 10 #45

Closed

lurobi mentioned this pull request Jan 29, 2021

mingw issue with executable path #42

Closed

jonesmz reviewed Aug 1, 2022

View reviewed changes

madebr reviewed Sep 1, 2023

View reviewed changes

add copyright

c6c1ce2

		@@ -0,0 +1,51 @@
		/* backtrace-supported.h.cmake -- Whether stack backtrace is supported.

@@ @@ -2,4 +2,4 @@ @@
               *.o
               *.lo
               *.a
-              *.la

		@@ -0,0 +1,109 @@
		cmake_minimum_required(VERSION 3.5)

		project(backtrace)

CMake support #19

Are you sure you want to change the base?

CMake support #19

Conversation

tehKaiN commented Jul 16, 2018 • edited Loading

thanm commented Jul 16, 2018

tehKaiN commented Jul 17, 2018

ianlancetaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tehKaiN commented Jul 17, 2018 • edited Loading

ianlancetaylor commented Jul 17, 2018

tehKaiN commented Jul 18, 2018

ianlancetaylor commented Jul 18, 2018

tehKaiN commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfr Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianlancetaylor left a comment

Choose a reason for hiding this comment

jcfr left a comment

Choose a reason for hiding this comment

jcfr Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

jcfr Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardLangFromNZ commented Feb 28, 2019

tehKaiN commented Feb 28, 2019

RichardLangFromNZ commented Feb 28, 2019

advancedwebdeveloper commented Aug 9, 2020

ianlancetaylor commented Aug 10, 2020

maowen commented Sep 7, 2021 • edited Loading

ianlancetaylor commented Sep 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tehKaiN commented Jul 16, 2018 •

edited

Loading

tehKaiN commented Jul 17, 2018 •

edited

Loading

jcfr Aug 11, 2018 •

edited

Loading

jcfr Aug 11, 2018 •

edited

Loading

jcfr Aug 11, 2018 •

edited

Loading

maowen commented Sep 7, 2021 •

edited

Loading