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

gh-131591: Handle includes for iOS in remote_debugging.c #132050

Merged
merged 2 commits into from
Apr 6, 2025
+14 −20
Merged

gh-131591: Handle includes for iOS in remote_debugging.c #132050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e29ec6
gh-131591: Handle includes for iOS in remote_debugging.c
pablogsal Apr 3, 2025
37f90b6
Update Include/internal/pycore_ceval.h
pablogsal Apr 4, 2025
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
1 change: 1 addition & 0 deletions Include/internal/pycore_ceval.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ PyAPI_FUNC(_PyStackRef) _PyFloat_FromDouble_ConsumeInputs(_PyStackRef left, _PyS

#ifndef Py_SUPPORTS_REMOTE_DEBUG
#if defined(__APPLE__)
#include <TargetConditionals.h>
# if !defined(TARGET_OS_OSX)
// Older macOS SDKs do not define TARGET_OS_OSX
# define TARGET_OS_OSX 1
Expand Down
33 changes: 13 additions & 20 deletions Python/remote_debugging.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,17 @@
# include <sys/mman.h>
#endif

#if defined(__APPLE__)
# include <TargetConditionals.h>
// Older macOS SDKs do not define TARGET_OS_OSX
# if !defined(TARGET_OS_OSX)
# define TARGET_OS_OSX 1
# endif
# if TARGET_OS_OSX
# include <libproc.h>
# include <mach-o/fat.h>
# include <mach-o/loader.h>
# include <mach-o/nlist.h>
# include <mach/mach.h>
# include <mach/mach_vm.h>
# include <mach/machine.h>
# include <sys/mman.h>
# include <sys/proc.h>
# include <sys/sysctl.h>
# endif
#if defined(__APPLE__) && TARGET_OS_OSX
# include <libproc.h>
Comment on lines +23 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break on older macOS versions (essentially any macOS version that predates the existence of iPhone) because TARGET_OS_OSX doesn't exist. However, on all versions of iOS, TARGET_OS_OSX will exist with a value of 0. So - my suggested approach here would be:

Suggested change
#if defined(__APPLE__) && TARGET_OS_OSX
# include <libproc.h>
#if defined(__APPLE__)
# include <TargetConditionals.h>
# if !defined(TARGET_OS_OSX) || TARGET_OS_OSX

I can see there's a couple of other places (L99 of this file; L352 of pycore_eval.h) where an analogous change would be required.

I guess the other option would be to modify the code that sets TARGET_OS_OSX from:

#  if !defined(TARGET_OS_OSX)
 #     define TARGET_OS_OSX 1
 #  endif

to

#  if !defined(TARGET_OS_OSX) && !(defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE)
#     define TARGET_OS_OSX 1
#  endif

This works because TARGET_OS_IPHONE is any Apple mobile device (iOS, tvOS, watchOS, visionOS); TARGET_OS_IOS is specifically an iPhone/iPad. However, futzing with TARGET_OS_OSX seems like asking for trouble, so the explicit approach would be my preferred option.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you may be missing the context that this file is importing this header where we are doing the dance:

cpython/Include/internal/pycore_ceval.h

Lines 350 to 360 in 0dbaeb9

#ifndef Py_SUPPORTS_REMOTE_DEBUG
#if defined(__APPLE__)
# if !defined(TARGET_OS_OSX)
// Older macOS SDKs do not define TARGET_OS_OSX
# define TARGET_OS_OSX 1
# endif
#endif
#if ((defined(__APPLE__) && TARGET_OS_OSX) || defined(MS_WINDOWS) || (defined(__linux__) && HAVE_PROCESS_VM_READV))
# define Py_SUPPORTS_REMOTE_DEBUG 1
#endif
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes - I did miss that import would be in effect. Given that imported definition, the form you've got in the PR at present should work - but it still makes me a bit twitchy because it's redefining an Apple-provided constant that has otherwise well defined behavior.

This is essentially what bit me here - the logic didn't make sense because I didn't notice the import where the behavior of the Apple-provided constant was being redefined). We've also had examples in the past where someone adds/removes an import of an otherwise unrelated file, and suddenly things break because that file has redefined basic platform constants (or has removed an import that was making those constants work in a predictable way).

If we're going to go down the path of redefining the constant, it might be worth looking at a top-level replacement for TargetConditionals.h that uses those base definitions and ensures they're consistently defined (so we don't need to do the defined(TARGET_OS_OSX) && TARGET_OS_OSX dance everywhere) - but that's a much bigger set of changes, and then comes with the overhead of making sure that everyone remembers to use the updated definitions, rather than importing TargetConditionals.h.

So - call my preference for using the definitions-as-provided a "strong opinion, weakly held". With the indentation nitpick flagged in pycore_ceval.h, I can live with this as-is.

# include <mach-o/fat.h>
# include <mach-o/loader.h>
# include <mach-o/nlist.h>
# include <mach/mach.h>
# include <mach/mach_vm.h>
# include <mach/machine.h>
# include <sys/mman.h>
# include <sys/proc.h>
# include <sys/sysctl.h>
#endif

#ifdef MS_WINDOWS
Expand Down Expand Up @@ -65,6 +58,8 @@
# define HAVE_PROCESS_VM_READV 0
#endif

#if defined(Py_REMOTE_DEBUG) && defined(Py_SUPPORTS_REMOTE_DEBUG)

// Define a platform-independent process handle structure
typedef struct {
pid_t pid;
Expand Down Expand Up @@ -101,8 +96,6 @@ cleanup_proc_handle(proc_handle_t *handle) {
handle->pid = 0;
}

#if defined(Py_REMOTE_DEBUG) && defined(Py_SUPPORTS_REMOTE_DEBUG)

#if defined(__APPLE__) && TARGET_OS_OSX
static uintptr_t
return_section_address(
Expand Down
Loading