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.

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

Remove "-rdynamic" flag for static builds #300

Merged
merged 7 commits into from
May 29, 2019
Merged

Remove "-rdynamic" flag for static builds #300

merged 7 commits into from
May 29, 2019

Conversation

ericpruitt
Copy link
Contributor

I ran into problems trying to build the statically linked cmark executable using musl libc that was caused by the "-rdynamic" flag being implicitly added to the build command. The resulting executable would had references to a musl libc shared object instead of being hermetic as expected. I'm not sure if this has any unintended consequences, but I at least have no problems building a dynamically linked executable with this patch applied on my Linux and macOS machines.

@ericpruitt
Copy link
Contributor Author

The CI pipeline shows this fails to build with MSVC due to the number of arguments used with the REGEX command. I have no experience with C development on Windows, and I'm a CMake novice, so I'm not sure how I should address the build failure.

@jgm
Copy link
Member

jgm commented May 27, 2019

I'm not a cmake expert either, but I'm sure this isn't the right approach to fix this problem. (If there is a problem: it would be good to have a fuller account of the problem, so we can confirm it.) Probably if something needs to be changed, it's in src/CMakeLists.txt, where the parameters for static and dynamic builds are being defined.

@ericpruitt
Copy link
Contributor Author

Probably if something needs to be changed, it's in src/CMakeLists.txt, where the parameters for static and dynamic builds are being defined.

I don't know if this makes a difference, but from what I saw when using CMake with verbosity/debugging, "-rdynamic" is added in based on rules in my systems global CMake configuration files, not something in the cmark build rules.

(If there is a problem: it would be good to have a fuller account of the problem, so we can confirm it.)

In the text below, I've gone through the process of generating the faulty build by generating the musl compiler wrapper (musl-gcc) and using the wrapper to compile cmark:

Extract musl and generate musl-gcc

$ tar xf musl-1.1.20.tar.gz
$ cd musl-1.1.20/
musl-1.1.20$ ./configure --disable-shared --prefix=$PWD
checking for C compiler... gcc
checking whether C compiler works... yes
...
checking preprocessor condition __ILP32__... false
checking whether compiler's long double definition matches float.h... yes
checking preprocessor condition __FAST_MATH__... false
creating config.mak... done
musl-1.1.20$ make install
musl-1.1.20$ make install
mkdir -p lib
mkdir -p obj
...
nistd/unlinkat.lo obj/src/unistd/usleep.lo obj/src/unistd/write.lo obj/src/unistd/writev.lo
ranlib lib/libc.a
./tools/install.sh -D -m 644 lib/libc.a ...musl-1.1.20/lib/libc.a
musl-1.1.20$ ls bin/
musl-gcc

Prepare the cmark build

musl-1.1.20$ cd ../cmark-src/
cmark-src$ mkdir build && cd build
build$ CFLAGS='-static -fpie -fPIC' CFLAGS='-O3 -static -fpie -fPIC' CC=.../musl-1.1.20/bin/musl-gcc cmake -G "Unix Makefiles" -DCMARK_TESTS=OFF -DCMARK_SHARED=OFF ..
-- The C compiler identification is GNU 6.3.0
-- The CXX compiler identification is GNU 6.3.0
-- Check for working C compiler: .../musl-1.1.20/bin/musl-gcc
-- Check for working C compiler: .../musl-1.1.20/bin/musl-gcc -- works
...
-- Generating done
-- Build files have been written to: .../cmark/build

Create the executable

build$ make
Scanning dependencies of target libcmark_static
[  4%] Building C object src/CMakeFiles/libcmark_static.dir/node.c.o
[  9%] Building C object src/CMakeFiles/libcmark_static.dir/buffer.c.o
...
[ 95%] Building C object src/CMakeFiles/cmark.dir/main.c.o
[100%] Linking C executable cmark
[100%] Built target cmark

Test the executable

build$ ./src/cmark --help
bash: ./src/cmark: No such file or directory
(127)

This shows the embedded shared object path which shouldn't be there.

build$ strings src/cmark | head -n5
/lib/ld-musl-x86_64.so.1
__stdout_used
stdout
__lctrans_cur
__strtoull_internal

@jgm
Copy link
Member

jgm commented May 27, 2019

@nwellnhof do you have any insight on this issue?

@nwellnhof
Copy link
Contributor

Seems like a CMake issue. The option probably comes from here: https://github.com/Kitware/CMake/blob/07cfb18f9d29cfc0588ede928846a03ec5599c48/Modules/Platform/Linux-GNU.cmake

Maybe setting -DCMAKE_SYSTEM_NAME=Generic helps.

@ericpruitt
Copy link
Contributor Author

Maybe setting -DCMAKE_SYSTEM_NAME=Generic helps.

That doesn't make a difference for me:

build$ CFLAGS='-static -fpie -fPIC' CFLAGS='-O3 -static -fpie -fPIC' CC=.../musl-1.1.20/bin/musl-gcc cmake -G "Unix Makefiles" -DCMARK_TESTS=OFF -DCMARK_SHARED=OFF -DCMAKE_SYSTEM_NAME=Generic ..
-- The C compiler identification is GNU 6.3.0
-- The CXX compiler identification is Clang 3.8.1
-- Check for working C compiler: /home/ericpruitt/emus/core/musl-1.1.20/bin/musl-gcc
-- Check for working C compiler: /home/ericpruitt/emus/core/musl-1.1.20/bin/musl-gcc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/clang++
-- Check for working CXX compiler: /usr/bin/clang++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test HAVE_FLAG_ADDRESS_SANITIZER
-- Performing Test HAVE_FLAG_ADDRESS_SANITIZER - Failed
-- Performing Test HAVE_FLAG_SANITIZE_ADDRESS
-- Performing Test HAVE_FLAG_SANITIZE_ADDRESS - Failed
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY - Success
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY - Success
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR - Success
-- Looking for stdbool.h
-- Looking for stdbool.h - found
-- Performing Test HAVE___BUILTIN_EXPECT
-- Performing Test HAVE___BUILTIN_EXPECT - Success
-- Performing Test HAVE___ATTRIBUTE__
-- Performing Test HAVE___ATTRIBUTE__ - Success
-- Configuring done
-- Generating done
-- Build files have been written to: /home/ericpruitt/emus/core/cmark/build
build$ ./src/cmark --help
bash: ./src/cmark: No such file or directory
(127)

At the time of this writing, it seems that CMake only uses "-rdynamic"
on Linux:

```
$ git clone https://github.com/Kitware/CMake.git
Cloning into 'CMake'...
remote: Enumerating objects: 335, done.
remote: Counting objects: 100% (335/335), done.
remote: Compressing objects: 100% (254/254), done.
remote: Total 272793 (delta 86), reused 125 (delta 79), pack-reused 272458
Receiving objects: 100% (272793/272793), 88.11 MiB | 29.21 MiB/s, done.
Resolving deltas: 100% (207174/207174), done.
$ cd CMake/Modules/
Modules$ fgrep -r -w rdynamic .
./Platform/Linux-TinyCC-C.cmake:set(CMAKE_EXE_EXPORTS_C_FLAG "-rdynamic ")
./Platform/Linux-NAG-Fortran.cmake:set(CMAKE_SHARED_LIBRARY_LINK_Fortran_FLAGS "-Wl,-rdynamic")
./Platform/Linux-Intel.cmake:  set(CMAKE_SHARED_LIBRARY_LINK_${lang}_FLAGS "-rdynamic")
./Platform/Linux-GNU.cmake:  set(CMAKE_SHARED_LIBRARY_LINK_${lang}_FLAGS "-rdynamic")
```

So the conditional has been modified accordingly.
Fix typo by changing "STATIC" to "CMARK_STATIC".
@ericpruitt
Copy link
Contributor Author

The only place I see mention of "rdynamic" in the CMake source is in the modules for Linux, so I've updated the conditional in my patch accordingly, and the build now succeeds on MSVC.

LINUX is not a CMake platform variable, so the expression has been
updated accordingly and the build functionality verified.
ericpruitt added a commit to ericpruitt/emus that referenced this pull request May 28, 2019
- At the time of this writing, "-rdynamic" is only added when using
  CMake on Linux, so the conditional expression used for patching
  cmark's build system has been updated to specifically check for Linux.
  Thanks for Nick Wellnhofer (nwellnhof) for pointing this out in
  commonmark/cmark#300.
ericpruitt added a commit to ericpruitt/emus that referenced this pull request May 28, 2019
- At the time of this writing, "-rdynamic" is only added when using
  CMake on Linux, so the conditional expression used for patching
  cmark's build system has been updated to specifically check for Linux.
  Thanks to Nick Wellnhofer (nwellnhof) for pointing this out in
  commonmark/cmark#300.
@nwellnhof
Copy link
Contributor

I think you have to put the final ${CMAKE_SHARED_LIBRARY_LINK_C_FLAGS} in double quotes to avoid an error if the variable isn't defined.

@jgm
Copy link
Member

jgm commented May 28, 2019

I found this post which recommends simply

SET(CMAKE_SHARED_LIBRARY_LINK_C_FLAGS)

That should be safe for a static build, right? And then we don't need to worry about regexes.

@ericpruitt
Copy link
Contributor Author

SET(CMAKE_SHARED_LIBRARY_LINK_C_FLAGS)

That works. I've updated pull request.

@jgm
Copy link
Member

jgm commented May 28, 2019

Should we also now remove the check for Linux? If I recall, it was only necessary because of the regex stuff.

@ericpruitt
Copy link
Contributor Author

I would argue that it should be scoped to Linux to minimize the chances of collateral damage because that's the only platform where this problem is known to exist since, when I grepped for "rdynamic" in the CMake repositories, the only matches were in the Linux modules directory. However, I'll do whatever's requested to get this merged as long as it fixes the problem for me :P, so I've updated the pull request accordingly.

@jgm
Copy link
Member

jgm commented May 29, 2019

Your argument is reasonable. I don't feel strongly about it either way. If you want to revert the last change, that's fine, I'll merge it either way!

@ericpruitt
Copy link
Contributor Author

If you want to revert the last change, that's fine, I'll merge it either way!

Thanks, done.

@jgm jgm merged commit f4895a6 into commonmark:master May 29, 2019
@jgm
Copy link
Member

jgm commented May 29, 2019

Thanks!

ericpruitt added a commit to ericpruitt/emus that referenced this pull request May 29, 2019
- Update cmark to pull in commonmark/cmark#300
  which includes a fix to ensure "-rdynamic" is not used for statically
  linked builds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants