Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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 src/hotspot/os/posix/forbiddenFunctions_posix.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

#include "utilities/compilerWarnings.hpp"
#include <stddef.h> // for size_t
#include <unistd.h> // clang workaround for _exit - see FORBID macro.

// If needed, add os::strndup and use that instead.
FORBID_C_FUNCTION(char* strndup(const char*, size_t), "don't use");
Expand Down
9 changes: 6 additions & 3 deletions src/hotspot/share/utilities/forbiddenFunctions.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
#include "utilities/macros.hpp"
#include <stdarg.h> // for va_list
#include <stddef.h> // for size_t
#include <stdlib.h> // clang workaround for exit, _exit - see FORBID macro.
#include <stdlib.h> // clang workaround for exit, _exit, _Exit - see FORBID macro.

#include OS_HEADER(forbiddenFunctions)
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought there was an OS_VARIANT_HEADER to dispatch directly to the posix variant, but I guess it doesn't exist. Using the semaphore example, this could save your dispatch header files, at the cost of an #ifdef. Not sure which is worse:

#if defined(LINUX) || defined(AIX)
# include "semaphore_posix.hpp"
#else
# include OS_HEADER(semaphore)
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

park.hpp has the same:

#if defined(LINUX) || defined(AIX) || defined(BSD)
# include "park_posix.hpp"
#else
# include OS_HEADER(park)
#endif

Copy link
Author

Choose a reason for hiding this comment

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

If I was going to go that route, I'd be more inclined toward

#if !define(_WINDOWS)
#include "forbiddenFunctions_windows.hpp"
#else
#include "forbiddenFunctions_posix.hpp"
#endif

rather than list all (or maybe just a subset) of the posix-like ports. My
inclination is to leave it as-is with OS_HEADERS, since I think that's the
"intended" idiom.

Copy link
Member

Choose a reason for hiding this comment

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

Overall I like this change. I appreciate the effort that has been put in to try and find an elegant solution to this problem.

but having OS specific files created just to include the posix version runs counter to why we have the posix variants in the first place IMO. Please select one of the above approaches so that the new aix/bsd/linux specific files can be removed in favour of the posix one. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

I disagree. It seems to me that breaking the abstraction like that is just asking for trouble.

Copy link
Author

Choose a reason for hiding this comment

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

@dholmes-ora - OS_HEADER and CPU_HEADER instead of explicit knowledge of what platforms exist and
which ones share some code in a "_posix" file and which ones have some unshared code. IMO the includer
shouldn't need to know that kind of implementation detail.

But oh well, all y'all seem to really hate the simple forwarding files and would prefer to skip that in favor of
hard-coding the file organization. In the interest of making progress, I'll do that.

Copy link
Author

Choose a reason for hiding this comment

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

@coleenp The copyrights on the new files being 2024-2025 is intentional. They were first published (in this PR)
in 2024, so I think are supposed to have that starting year.

Copy link
Member

Choose a reason for hiding this comment

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

@kimbarrett I understand. I also don't like the includer needing to know about this, and maybe it is time to add OS_FAMILY_HEADER to deal with it. But in the absence of the new macro I prefer this break of abstraction to the creation of a bunch of tiny forwarding files.

Copy link
Contributor

@coleenp coleenp Jan 10, 2025

Choose a reason for hiding this comment

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

I prefer the opposite. If you break these up into a bunch of forwarding files, do all the rest of the VM the same way.

It may be my problem but I don't have a good IDE to have to navigate through a slew of similarly named files to find the real implementation.

Copy link
Member

Choose a reason for hiding this comment

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

@coleenp I thought you and I were on the same page not liking the creation of all these little files that just forward to the posix one. ???


Expand All @@ -39,7 +39,10 @@
// avoided entirely.

FORBID_IMPORTED_NORETURN_C_FUNCTION(void exit(int), "use os::exit")
FORBID_IMPORTED_NORETURN_C_FUNCTION(void _exit(int), "use os::_exit")
FORBID_IMPORTED_NORETURN_C_FUNCTION(void _Exit(int), "use os::exit")

// Windows puts _exit in <stdlib.h>, POSIX in <unistd.h>.
FORBID_IMPORTED_NORETURN_C_FUNCTION(void _exit(int), "use os::exit")

FORBID_IMPORTED_C_FUNCTION(char* strerror(int), "use os::strerror");
FORBID_IMPORTED_C_FUNCTION(char* strtok(char*, const char*), "use strtok_r");
Expand All @@ -50,7 +53,7 @@ FORBID_C_FUNCTION(int vsnprintf(char*, size_t, const char*, va_list), "use os::v

// All of the following functions return raw C-heap pointers (sometimes as an
// option, e.g. realpath or getwd) or, in case of free(), take raw C-heap
// pointers.
// pointers. We generally want allocation to be done through NMT.
FORBID_IMPORTED_C_FUNCTION(void* malloc(size_t size), "use os::malloc");
FORBID_IMPORTED_C_FUNCTION(void free(void *ptr), "use os::free");
FORBID_IMPORTED_C_FUNCTION(void* calloc(size_t nmemb, size_t size), "use os::malloc and zero out manually");
Expand Down