Add an API for native libraries #7682

AditiS11 · 2025-03-07T15:31:11Z

Use /proc file system to capture the dependent libraries. Combine multiple sections into one and iterate over the list plus invoke the callback

babsingh · 2025-03-07T16:07:22Z

@keithc-ca could you please help in reviewing this PR?

keithc-ca · 2025-03-07T21:31:56Z

port/common/omrsl.c

+/**
+ * Native Libraries
+ *
+ * This function is called go get all the shared  libraries loaded by the Java process. 


This is not specific to Java: please remove that reference.

keithc-ca · 2025-03-07T21:37:08Z

port/unix/omrsl.c

+	intptr_t fd=portLibrary->file_open(portLibrary,"/proc/self/maps",EsOpenRead,0);
+	if(0 > fd){
+		return -1;
+	}
+	char buffer[READ_CHUNK_SIZE +1];
+	char carryOver[READ_CHUNK_SIZE +1]="";
+	intptr_t bytesRead = 0;
+	uintptr_t result=0;
+	while (0<(bytesRead = portLibrary->file_read(portLibrary, fd, buffer, READ_CHUNK_SIZE))){


This is not strictly legal C code: declarations must be at the beginning of a block.

Please first update this so the additions follow the coding standard. Some examples of things that don't follow those guidelines:

comparisons (== and != where the constant is not on the left

conditions with embedded assignments

code should be indented consistently one tab per level (not spaces)

missing comments on #else and #endif

binary operators (e.g. +, =, <) should have a space before and after

a space should appear consistently after if, while, switch, etc.

compound expressions should have parentheses to avoid any concern for precedence rules

a space should appear before { for each if, while, else etc, block

keithc-ca · 2025-03-12T21:21:32Z

include_core/omrport.h

@@ -2115,6 +2116,8 @@ typedef struct OMRPortLibrary {
 	uintptr_t (*sl_open_shared_library)(struct OMRPortLibrary *portLibrary, char *name, uintptr_t *descriptor, uintptr_t flags) ;
 	/** see @ref omrsl.c::omrsl_lookup_name "omrsl_lookup_name"*/
 	uintptr_t (*sl_lookup_name)(struct OMRPortLibrary *portLibrary, uintptr_t descriptor, char *name, uintptr_t *func, const char *argSignature) ;
+	/** see @ref omrsl.c::omrsl_get_shared_libraries "omrsl_open_shared_library"*/


Please correct the comment:

/** see @ref omrsl.c::omrsl_get_libraries "omrsl_get_libraries"*/

keithc-ca · 2025-03-12T21:22:24Z

port/common/omrport.c

@@ -29,6 +29,7 @@
 #include "omrport.h"
 #include "omrportpriv.h"
 #include "ut_omrport.h"
+#include "omrsl.h"


I don't think this is wanted: the declaration of omrsl_get_libraries() should be in omrportpriv.h instead.

keithc-ca · 2025-03-12T21:22:59Z

port/common/omrsl.h

+uintptr_t
+omrsl_get_libraries(struct OMRPortLibrary *portLibrary, OMRLibraryInfoCallback callback, void *userData);


Please move this to omrportpriv.h.

keithc-ca · 2025-03-12T21:24:15Z

port/unix/omrsl.c

+/* Max Length of Library name */
+#define READ_CHUNK_SIZE 4096


The comment isn't really accurate, should end with a period, and might be better placed just before the function that uses it.

keithc-ca · 2025-03-12T21:24:43Z

port/unix/omrsl.c

+/**
+ * Native Libraries
+ *
+ * This function is called go get all the shared  libraries loaded by a process. 


Just one space between "shared" and "libraries", please.

keithc-ca · 2025-03-12T21:31:29Z

port/unix/omrsl.c

+			count = sscanf(
+					line,"%p-%p %4s %lx %x:%x %lu %s",
+					&addrLow, &addrHigh, permissions,
+					&offset, &devMajor, &devMinor, &inode, path);


The first two arguments (at least) should be on lines by themselves:

count = sscanf( line, "%p-%p %4s %lx %x:%x %lu %s", &addrLow, &addrHigh, permissions, &offset, &devMajor, &devMinor, &inode, path);

keithc-ca · 2025-03-12T21:32:37Z

port/unix/omrsl.c

+				result = callback(path,addrLow,addrHigh, userData);
+			}
+			if (0 != result) {
+				portLibrary->file_close(portLibrary, fd);
+				return result;
+			}


Missing spaces, break avoids repetition:

result = callback(path, addrLow, addrHigh, userData); if (0 != result) { break; }

keithc-ca · 2025-03-12T21:33:36Z

port/unix/omrsl.c

+#else
+        /* Platform not supported */
+	return OMRPORT_ERROR_NOT_SUPPORTED_ON_THIS_PLATFORM;


Missing comment, missing period, inconsistent indentation:

#else /* defined(LINUX) */ /* Platform not supported. */ return OMRPORT_ERROR_NOT_SUPPORTED_ON_THIS_PLATFORM;

keithc-ca · 2025-03-12T21:35:14Z

port/unix/omrsl.c

+	bytesRead = portLibrary->file_read(portLibrary, fd, buffer, READ_CHUNK_SIZE);
+	do  {


Moving the read inside the loop avoids repetition:

do { bytesRead = portLibrary->file_read(portLibrary, fd, buffer, sizeof(buffer) - 1);

(line 522 should then be deleted)

keithc-ca · 2025-03-12T21:36:59Z

port/unix/omrsl.c

+		while (NULL != line) {
+			nextLine = strtok(NULL, "\n");
+			if (NULL == nextLine && !lastCharIsNewline) {
+				portLibrary->str_printf(portLibrary, carryOver, sizeof(carryOver), "%s", line);


line may point within combinedBuffer and thus may not fit in carryOver: this must be checked here.

I will change the length of the buffer to 4146 bytes. The max path length in Unix is 4096 bytes and I am considering another 50 bytes for the addresses, inode number etc. That way line can never exceed the size of the carryOver buffer.

It should be defined within the LINUX block and be defined in terms of PATH_MAX:

#define READ_CHUNK_SIZE (PATH_MAX + 50)

However, I'm not sure that addresses my concern. No matter what size the buffers are, parts of combinedBuffer and carryOver may need more space than is available.

I don't think we want line 512 any more - line 502 already wrote to carryOver. After removing that line, line 510 becomes redundant.

keithc-ca

Please fix the commit message:

lines in the body should be no more than 72 characters long
this does not (nor should it) "combine multiple sections"

keithc-ca · 2025-03-14T16:33:28Z

port/common/omrport.c

@@ -840,3 +841,4 @@ omrport_allocate_library(struct OMRPortLibrary **portLibrary)
 	}
 	return rc;
 }
+


Please remove this extra blank line (the last two characters should be "}\n").

keithc-ca · 2025-03-14T16:34:20Z

port/common/omrsl.c

@@ -135,3 +135,19 @@ omrsl_startup(struct OMRPortLibrary *portLibrary)
 }




Please delete this blank line (we don't need more than one in a row).

keithc-ca · 2025-03-14T16:35:01Z

port/common/omrsl.c

+/**
+ * Native Libraries
+ *
+ * This function is called to get all the shared libraries loaded by a process. 


No trailing spaces (here or on any line you add or modify).

keithc-ca · 2025-03-14T16:42:56Z

port/unix/omrsl.c

@@ -45,6 +45,7 @@
 #include <errno.h>
 #include <dlfcn.h>
 #include "ut_omrport.h"
+#include "omrport.h"


Please remove this (see line 76).

keithc-ca · 2025-03-14T16:55:12Z

port/unix/omrsl.c

+		while (NULL != line) {
+			nextLine = strtok(NULL, "\n");
+			if (NULL == nextLine && !lastCharIsNewline) {
+				portLibrary->str_printf(portLibrary, carryOver, sizeof(carryOver), "%s", line);


It should be defined within the LINUX block and be defined in terms of PATH_MAX:

#define READ_CHUNK_SIZE (PATH_MAX + 50)

However, I'm not sure that addresses my concern. No matter what size the buffers are, parts of combinedBuffer and carryOver may need more space than is available.

keithc-ca · 2025-03-14T16:56:04Z

port/unix/omrsl.c

+			if (0 != result) {
+				break;
+			}


Lines 518-520 can go before line 517 (only there might result be non-zero).

keithc-ca · 2025-03-14T16:56:17Z

port/unix/omrsl.c

+	portLibrary->file_close(portLibrary, fd);
+	return result;
+#else /* defined(LINUX) */
+        /* Platform not supported. */


This indentation is still incorrect.

keithc-ca · 2025-03-17T14:40:43Z

This should not make reference to JFR; yes, it might be one of the first consumers of this new API, but this project aims to be language neutral.

keithc-ca · 2025-03-17T14:42:05Z

port/unix/omrsl.c

+omrsl_get_libraries(struct OMRPortLibrary *portLibrary, OMRLibraryInfoCallback callback, void *userData)
+{
+#if defined(LINUX)
+	#include <limits.h>


This isn't likely to work (within the function body); please move it near the top of the file (if needed).

keithc-ca · 2025-03-17T14:42:37Z

port/unix/omrsl.c

+	carryOver[0] = '\0';
+	intptr_t fd = portLibrary->file_open(portLibrary, "/proc/self/maps", EsOpenRead, 0);


This isn't legal C code (an assignment statement before a declaration).

keithc-ca · 2025-03-17T14:44:13Z

port/unix/omrsl.c

+		int portableError = portLibrary->error_last_error_number(portLibrary);
+		Trc_PRT_find_executable_name_failedOpeningProcFS(portableError);
+		portLibrary->error_set_last_error_with_message(portLibrary, portableError, "Failed to open /proc fs");
+		return -1;


The return type is unsigned; a negative value is not appropriate.

keithc-ca · 2025-03-17T14:44:26Z

port/unix/omrsl.c

+			char *nextLine = strtok(NULL, "\n");
+			if ((NULL == nextLine) && (0 == lastCharIsNewline)) {
+				int readChars = portLibrary->str_printf(portLibrary, carryOver, sizeof(carryOver), "%s", line);
+				if(readChars >= sizeof(carryOver)) {


Space before (, please.

keithc-ca · 2025-03-17T14:45:38Z

port/unix/omrsl.c

+		while (NULL != line) {
+			char *nextLine = strtok(NULL, "\n");
+			if ((NULL == nextLine) && (0 == lastCharIsNewline)) {
+				int readChars = portLibrary->str_printf(portLibrary, carryOver, sizeof(carryOver), "%s", line);


int is inappropriate: str_printf returns uintptr_t

keithc-ca · 2025-03-17T14:50:13Z

port/unix/omrsl.c

+			}
+			line = nextLine;
+		}
+	} while (bytesRead < 0);


bytesRead cannot be negative here (see lines 486-488); please replace the "do...while" loop with a for (;;) loop.

keithc-ca · 2025-03-18T20:46:52Z

port/unix/omrsl.c

+	carryOver[0] = '\0';
+	if (-1 == fd) {
+		portableError = portLibrary->error_last_error_number(portLibrary);
+		Trc_PRT_find_executable_name_failedOpeningProcFS(portableError);


Please create a new tracepoint, that one has a different meaning/purpose.

keithc-ca · 2025-03-18T20:48:39Z

port/unix/omrsl.c

+		portLibrary->error_set_last_error_with_message(
+				portLibrary,
+				portableError,
+				"Failed to open /proc fs");


The error message should mention the actual path for which open failed.

keithc-ca · 2025-03-18T20:49:45Z

port/unix/omrsl.c

+				portLibrary,
+				portableError,
+				"Failed to open /proc fs");
+		return (uintptr_t)portableError;


Casts can't change both the size and signedness in one step; change the size, then the signedness:

return (uintptr_t)(intptr_t)portableError;

keithc-ca · 2025-03-18T20:50:46Z

port/unix/omrsl.c

+					portLibrary->error_set_last_error_with_message(
+							portLibrary,
+							portableError,
+							"Line from /proc/self/maps was truncated due to exceeding buffer size");


The part after "truncated" doesn't help; please omit it.

keithc-ca · 2025-03-18T20:51:59Z

port/unix/omrsl.c

+{
+#if defined(LINUX)
+	/* Length of buffer. */
+	#define READ_CHUNK_SIZE (PATH_MAX + 100)


Please don't indent macro definitions.

port/unix/omrsl.c

keithc-ca · 2025-03-25T20:51:47Z

port/unix/omrsl.c

+			char *nextLine = strtok(NULL, "\n");
+			if ((NULL == nextLine) && (0 == lastCharIsNewline)) {
+				readChars = portLibrary->str_printf(portLibrary, carryOver, sizeof(carryOver), "%s", line);
+				if (readChars >= sizeof(carryOver)) {
+					portableError = portLibrary->error_last_error_number(portLibrary);
+					portLibrary->error_set_last_error_with_message(
+							portLibrary,
+							portableError,
+							"Line from /proc/self/maps was truncated");
+					carryOver[0] = '\0';
+					break;
+				}


I don't think the timing of this is appropriate: the content of line should be consumed before considering text after the newline.

The only reason I am extracting nextLine in the start is because if nextLine is NULL and the last character of the current line is not \n, I want the line to be copied to the carryOver buffer because the line is incomplete. I don't want the line to be processed further (extracting the name and addresses). I don't want to end up extracting the name and address ranges for incomplete lines. Hence I am adding this check in the start itself.

That's not how strtok() works. If combinedBuffer doesn't contain a newline, then line will be NULL.

By the way, we would use strtok_r() instead (it is thread-safe).

But I don't think we need to use strtok_r() either: Instead, this should use file_read_text() which reads only up to the first newline (that will fit in the supplied buffer). Only if one is not present should it be joined to any previous fragment. When a newline is found, the line should be scanned and the information provided to the callback.

I removed the carryOver buffer completely as I have changed the implementation to use file_read_text( ). Since the function reads a single line which fits in the buffer.

keithc-ca · 2025-03-28T19:05:23Z

port/unix/omrsl.c

+				"Failed to open /proc/self/maps");
+		return (uintptr_t)(intptr_t)portableError;
+	}
+	while (NULL != portLibrary->file_read_text(portLibrary, fd, buffer, sizeof(buffer))) {


file_read_text() may return a partial line if it doesn't fit in the supplied buffer; this needs to check whether the line ends with a newline. If it does not, it might be reasonable for this function to signal failure (an unexpectedly long path name).

Please let me know if the function should signal failure and stop further enumeration or continue with reading the further entries in the file?

keithc-ca · 2025-03-28T19:09:19Z

port/unix/omrsl.c

+		memset(path, 0, sizeof(path));
+		count = sscanf(
+				buffer,
+				"%p-%p %4s %lx %x:%x %lu %s",


Suggest that the local path is not needed; instead of %s for the final pattern, use %n to learn the offset within buffer where the path is located.

Use /proc file system to capture the dependent libraries. Invoke the callback on each library.

AditiS11 requested review from babsingh, mstoodle and 0xdaryl as code owners March 7, 2025 15:31

keithc-ca suggested changes Mar 7, 2025

View reviewed changes

AditiS11 force-pushed the nativelib-x64 branch 2 times, most recently from 3b237c5 to a21d95e Compare March 11, 2025 11:23

keithc-ca suggested changes Mar 12, 2025

View reviewed changes

AditiS11 force-pushed the nativelib-x64 branch 3 times, most recently from e619b0d to af91ad3 Compare March 14, 2025 14:30

keithc-ca suggested changes Mar 14, 2025

View reviewed changes

AditiS11 force-pushed the nativelib-x64 branch from af91ad3 to 20fe8cd Compare March 14, 2025 20:19

keithc-ca reviewed Mar 17, 2025

View reviewed changes

keithc-ca suggested changes Mar 17, 2025

View reviewed changes

AditiS11 force-pushed the nativelib-x64 branch from 20fe8cd to eba8dad Compare March 18, 2025 15:29

keithc-ca suggested changes Mar 18, 2025

View reviewed changes

AditiS11 force-pushed the nativelib-x64 branch from eba8dad to a2f94d8 Compare March 19, 2025 11:20

AditiS11 changed the title ~~jfr: add an api for native libraries~~ Add an API for native libraries Mar 20, 2025

AditiS11 force-pushed the nativelib-x64 branch from a2f94d8 to bda56eb Compare March 21, 2025 08:05

keithc-ca suggested changes Mar 25, 2025

View reviewed changes

AditiS11 force-pushed the nativelib-x64 branch 2 times, most recently from 27ada4e to f8455bf Compare March 28, 2025 17:03

keithc-ca suggested changes Mar 28, 2025

View reviewed changes

Add an API for native libraries

16ba0d4

Use /proc file system to capture the dependent libraries. Invoke the callback on each library.

AditiS11 force-pushed the nativelib-x64 branch from f8455bf to 16ba0d4 Compare March 31, 2025 11:30

		uintptr_t
		omrsl_get_libraries(struct OMRPortLibrary portLibrary, OMRLibraryInfoCallback callback, void userData);

		/* Max Length of Library name */
		#define READ_CHUNK_SIZE 4096

		bytesRead = portLibrary->file_read(portLibrary, fd, buffer, READ_CHUNK_SIZE);
		do {

		@@ -135,3 +135,19 @@ omrsl_startup(struct OMRPortLibrary *portLibrary)
		}

		carryOver[0] = '\0';
		intptr_t fd = portLibrary->file_open(portLibrary, "/proc/self/maps", EsOpenRead, 0);

Add an API for native libraries #7682

Are you sure you want to change the base?

Add an API for native libraries #7682

Conversation

AditiS11 commented Mar 7, 2025

babsingh commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AditiS11 Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AditiS11 Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca Mar 18, 2025 •

edited

Loading

AditiS11 Mar 26, 2025 •

edited

Loading

AditiS11 Mar 28, 2025 •

edited

Loading