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

Deprecated, unsupported functions compiling with clang 14 on MacOS 13 #514

Closed
coolbikerdad opened this issue Dec 5, 2022 · 17 comments
Closed

Comments

@coolbikerdad
Copy link
Contributor

While building with Xcode 14 command-line tools (clang 14.0) compiler warnings are generated for deprecated declarations including:
get_etext()
get_end()
getsectbynamefromheader_64()
with the accompanying note:
... is deprecated: first deprecated in macOS 13.0 [-Wdeprecated-declarations]
and
_dyld_bind_fully_image_containing_address()
... is deprecated: first deprecated in macOS 10.5 - dlopen(RTLD_NOW) [-Wdeprecated-declarations]

My concern would be that should these functions be removed in future macOS releases, bdwgc would not build.
Is there a plan to mitigate these deprecations?

@ivmai
Copy link
Owner

ivmai commented Dec 5, 2022

As of now, there is no proposal how to avoid these deprecated symbols.

@ivmai
Copy link
Owner

ivmai commented Nov 29, 2023

Does anyone have any proposal how not to use the deprecated functions?

@coolbikerdad
Copy link
Contributor Author

I have been looking through the source for bdwgc looking at use of get_etext() and get_end() (they only appear in the definition of DATASTART and DATAEND) and I think there is an error in os_dep.c. These functions should not be used in a dynamic loading environment (they give the wrong answers) and indeed do not actually get called. But the definition of GC_register_data_segments still gets compiled and causes the deprecation warnings. I think the test for DYNAMIC_LOADING is reversed.

Line 2071 and following says:

#   if !defined(DYNAMIC_LOADING) && defined(GC_DONT_REGISTER_MAIN_STATIC_DATA)
      /* Avoid even referencing DATASTART and DATAEND as they are       */
      /* unnecessary and cause linker errors when bitcode is enabled.   */
      /* GC_register_data_segments() is not called anyway.              */
...

The intention seems to be to compile a No-op for DYNAMIC_LOADING environments, but the test is inverted.

Changing line 2071 to read:

# if defined(DYNAMIC_LOADING) /*&& defined(GC_DONT_REGISTER_MAIN_STATIC_DATA)*/

allows the collector to be compiled on macOS without deprecation warnings and the "make check" tests pass.

@coolbikerdad
Copy link
Contributor Author

I have an idea for a solution for the getsectbynamefromheader deprecation warning, it will take a DARWIN specific change dyn_load.c that may be best protected by a configure definition. I am only compiling via Makefile.direct so I'll have to give that change to someone else to figure out. I'll test it out and add a reply here.

As for the final warning about _dyld_bind_fully_image_containing_address, I'll take a look, but it may be a little while.

Regards
Neil

@ivmai
Copy link
Owner

ivmai commented Dec 1, 2023

When you have a draft, let me know, I could test it. Thank you!

@coolbikerdad
Copy link
Contributor Author

Hi. Attached revised os_dep.c and dyn_load.c
Archive.zip

Fixes deprecated functions get_end and get_etext by making the calling function a NO-OP for DARWIN as it never gets called.

Fixes deprecated function getsectnamefromheader_64 by using getsectiondata instead (changes enabled by #ifdef USE_GETSECTIONDATA, defined by default).

Fixes deprecated function _dyld_bind_fully_image_containing_address by using dlopen(NULL,RTLD_NOW) instead (change enabled by #ifndef USE_DYLD_TO_BIND - not defined by default).

With these changes on macOS the collector builds with no errors or warnings and passes the tests. Would benefit from more regression testing if you have a more extensive set.

Regards
Neil

@ivmai
Copy link
Owner

ivmai commented Dec 4, 2023

Thank you, I will process it in a week.

@ivmai
Copy link
Owner

ivmai commented Dec 6, 2023

I have been looking through the source for bdwgc looking at use of get_etext() and get_end() (they only appear in the definition of DATASTART and DATAEND) and I think there is an error in os_dep.c.

No, this code part is correct. But I got your point, some change is needed.

@coolbikerdad
Copy link
Contributor Author

I have been looking through the source for bdwgc looking at use of get_etext() and get_end() (they only appear in the definition of DATASTART and DATAEND) and I think there is an error in os_dep.c.

No, this code part is correct. But I got your point, some change is needed.

You are correct, I realised this, and my change in os_dep.c I attached to my last message has a simpler DARWIN-specific change as you will see.

@ivmai
Copy link
Owner

ivmai commented Dec 6, 2023

Actually get_end and get_etext were not called since commit 111a44f (v6.2, year 2003).

@ivmai
Copy link
Owner

ivmai commented Dec 6, 2023

Do we need to call dlclose after dlopen?

@coolbikerdad
Copy link
Contributor Author

Do we need to call dlclose after dlopen?

No, I don't think so, passing 0 or NULL refers to the running program, so no library gets loaded specifically by this call, it forces dynamically linked functions referenced in the main program to be fully loaded.

ivmai pushed a commit that referenced this issue Dec 8, 2023
Issue #514 (bdwgc).

get_etext() and get_end() are only referenced (from DATASTART and
DATAEND, respectively) but are never invoked.

* os_dep.c [DYNAMIC_LOADING && DARWIN] (GC_register_data_segments): Do
nothing (instead of GC_add_roots_inner(DATASTART,DATAEND)); update
comment.
ivmai pushed a commit that referenced this issue Dec 8, 2023
Issue #514 (bdwgc).

Use dlopen(0,RTLD_NOW) instead; do not pass -Wno-deprecated-declarations
option to the compiler any more.

For the old behavior, pass "-D USE_DYLD_TO_BIND" option to the compiler.

* CMakeLists.txt [enable_werror && APPLE]: Do not add
-Wno-deprecated-declarations to compiler options.
* configure.ac [$werror_flag==yes && $host==*-*-darwin*]
(WERROR_CFLAGS): Do not add -Wno-deprecated-declarations.
* dyn_load.c [DARWIN && !USE_DYLD_TO_BIND
&& !NO_DYLD_BIND_FULLY_IMAGE]: Include dlfcn.h.
* dyn_load.c [GC_MUST_RESTORE_REDEFINED_DLOPEN] (dlopen): Move
redefinition down (to be near end of the file).
* dyn_load.c [SOLARISDL && THREADS && !PCR && !GC_SOLARIS_THREADS
&& !CPPCHECK]: Move error preprocessor directive upper (to be near
GC_MUST_RESTORE_REDEFINED_DLOPEN).
* dyn_load.c [DARWIN && !NO_DYLD_BIND_FULLY_IMAGE] (GC_init_dyld):
Change FIXME item about _dyld_bind_fully_image_containing_address
to a note comment.
* dyn_load.c [DARWIN && !NO_DYLD_BIND_FULLY_IMAGE && !USE_DYLD_TO_BIND]
(GC_init_dyld): Call dlopen(NULL,RTLD_NOW) instead of
_dyld_bind_fully_image_containing_address(GC_malloc); define dl_handle
local variable.
* include/private/gcconfig.h [(I386 || AARCH64 || ARM32 || X86_64)
&& DARWIN && (TARGET_OS_IPHONE || TARGET_OS_XR || TARGET_OS_VISION)
&& !NO_DYLD_BIND_FULLY_IMAGE] (NO_DYLD_BIND_FULLY_IMAGE): Do not
define.
ivmai pushed a commit that referenced this issue Dec 8, 2023
Issue #514 (bdwgc).

* dyn_load.c [DARWIN] (dyld_section_add_del): Add comment (to document
USE_GETSECTBYNAME macro).
* dyn_load.c [DARWIN && !USE_GETSECTBYNAME] (dyld_section_add_del):
Do not define sec local variable; makr slide argument as unused; call
getsectiondata() instead of getsectbynamefromheader[_64]().
@ivmai
Copy link
Owner

ivmai commented Dec 8, 2023

I've refactored and merged your code. Please test it on your side. Automatic testing is performed on Travis CI and on Github Actions.
Thank you for fixing this long standing issue!

@ivmai ivmai closed this as completed Dec 8, 2023
@coolbikerdad
Copy link
Contributor Author

I've refactored and merged your code. Please test it on your side. Automatic testing is performed on Travis CI and on Github Actions. Thank you for fixing this long standing issue!

Builds and checks OK using Makefile.direct

However, autogen.sh fails for me. I'd like to be able to use configure, any idea how to fix?

neil@BETH-STUDIO bdwgc ./autogen.sh
configure.ac:27: installing './compile'
configure.ac:18: installing './config.guess'
configure.ac:18: installing './config.sub'
configure.ac:21: installing './install-sh'
configure.ac:21: installing './missing'
src/Makefile.am:14: error: Libtool library used but 'LIBTOOL' is undefined
src/Makefile.am:14: The usual way to define 'LIBTOOL' is to add 'LT_INIT'
src/Makefile.am:14: to 'configure.ac' and run 'aclocal' and 'autoconf' again.
src/Makefile.am:14: If 'LT_INIT' is in 'configure.ac', make sure
src/Makefile.am:14: its definition is in aclocal's search path.
src/Makefile.am: installing './depcomp'
parallel-tests: installing './test-driver'
autoreconf: error: automake failed with exit status: 1

@ivmai
Copy link
Owner

ivmai commented Dec 10, 2023

However, autogen.sh fails for me. I'd like to be able to use configure, any idea how to fix?
neil@BETH-STUDIO bdwgc ./autogen.sh
..
src/Makefile.am:14: error: Libtool library used but 'LIBTOOL' is undefined

Please try the advice reported in #75 (comment)

@ivmai
Copy link
Owner

ivmai commented Dec 13, 2023

Commit 18b5111 caused a regression: issue #596

ivmai added a commit that referenced this issue Dec 13, 2023
(fix of commit 18b5111)

Issue #514 (bdwgc).

getsectbynamefromheader is deprecated (first time in macOS 13.0),
getsectiondata (introduced in macOS 10.7) is used instead to avoid
a warning about the deprecated symbol, at least.

Note: define USE_GETSECTBYNAME or USE_GETSECTIONDATA to control which
symbol to use manually, if needed.

* dyn_load.c [DARWIN && !USE_GETSECTBYNAME && !USE_GETSECTIONDATA
&& MAC_OS_X_VERSION_MAX_ALLOWED<=1200] (USE_GETSECTBYNAME): Define
macro (before dyld_section_add_del); update comment.
ivmai pushed a commit that referenced this issue Dec 13, 2023
(fix of commit 0a6e4c3)

Issue #514 (bdwgc).

MAC_OS_X_VERSION_12_0 constant is 120000, not 1200.
(Up to 10.9 Apple had used version number as 1090, and since 10.10
they switched to 101000.  So, here should be 120000.)

* dyn_load.c [DARWIN && !USE_GETSECTBYNAME && !USE_GETSECTIONDATA]:
Change MAC_OS_X_VERSION_MAX_ALLOWED<=1200 to
MAC_OS_X_VERSION_MAX_ALLOWED<=120000.
ivmai added a commit that referenced this issue Dec 13, 2023
(fix of commit 0a6e4c3)

Issue #514 (bdwgc).

* dyn_load.c [DARWIN && !USE_GETSECTBYNAME]: Do not check
USE_GETSECTIONDATA; change MAC_OS_X_VERSION_MAX_ALLOWED<=120000 to
MAC_OS_X_VERSION_MIN_REQUIRED<1070; update comment.

Co-authored-by: Ivan Maidanski <ivmai@mail.ru>
@ivmai
Copy link
Owner

ivmai commented Dec 13, 2023

I hope this is the final change;)

ivmai added a commit that referenced this issue Dec 24, 2023
(fix of commit 8b90a7c)

Issue #514 (bdwgc).

* os_dep.c [DARWIN]: Do not include mach-o/getsect.h if DYNAMIC_LOADING
or GC_DONT_REGISTER_MAIN_STATIC_DATA.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants