Skip to content

[lldb][AIX] Introducing _ALL_SOURCE macro into driver CMakeLists #120607

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

Merged
merged 1 commit into from
Dec 20, 2024
Merged

[lldb][AIX] Introducing _ALL_SOURCE macro into driver CMakeLists #120607

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions lldb/tools/driver/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ if(APPLE)
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-sectcreate,__TEXT,__info_plist,${CMAKE_CURRENT_BINARY_DIR}/lldb-Info.plist")
endif()

if (UNIX AND ${CMAKE_SYSTEM_NAME} MATCHES "AIX")
remove_definitions("-D_XOPEN_SOURCE=700")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fairly fragile. It may be better to do a -U_XOPEN_SOURCE.

Some comment about why you're undoing the changes here might be in order as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It may be better to do a -U_XOPEN_SOURCE

.. which, at least according to my experiments, isn't possible it seems. However, also according to my experiments, remove_definitions doesn't actually remove macros defined by add_compile_definitions (only add_definitions), which brings up another question: Is this actually needed? Would it be sufficient to define _ALL_SOURCE as well?

Copy link
Contributor Author

@DhruvSrivastavaX DhruvSrivastavaX Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, It was added by another team in IBM but I think the intention to add this way was to only utilise the relevant posix standard definitions and avoid any extra un-needed definitions. But then again, some definitions being used in LLDB code, do not come under the _X_OPEN_SOURCE for AIX, in which case we need to use _ALL_SOURCE instead. But yes, I can check if only having add_definitions does not cause any compilation conflicts and update accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. The way I'd start is the see if the remove_definitions call is actually doing something (in my experiments, it didn't, but I only tried it on toy programs). If it isn't, then you already have both of these defined, and there's nothing further to check.

That said, if you can get it touch with them, it might be good to check they were trying to achieve, as having different definition in different parts of the program is a recipe for problems. From that comment, it sounds like they had problems building llvm with _ALL_SOURCE, but adding the definition here still requires llvm (at least, parts included by this code) to build in this mode.

add_definitions("-D_ALL_SOURCE")
endif()

add_lldb_tool(lldb
Driver.cpp
Platform.cpp
Expand Down
Loading