On Wed, 31 Jan 2024 06:27:33 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Chris' review suggestion - replace LOG_MISC with ERROR_MESSAGE
>
> src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 35:
>
>> 33: #include "sys.h"
>> 34: #include "util.h"
>> 35: #include "error_messages.h"
>
> Nit: to maintain include sort order this should have gone where
> `log_messages.h` was removed.
Hello David, I had actually first put it in that line you noted, but that then
lead to a compilation error:
In file included from /jdk/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c:33:
/jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:35:20: error:
unknown type name 'FILE'
void print_message(FILE *fp, const char *prefix, const char *suffix,
^
/jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:41:29: error: a
parameter list without types is only allowed in a function definition
const char * jvmtiErrorText(jvmtiError);
^
/jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:43:28: error: a
parameter list without types is only allowed in a function definition
const char * jdwpErrorText(jdwpError);
^
3 errors generated.
I think the reason for those compilation errors is that `error_messages.h` and
a few other header files are not explicitly listing their necessary includes
and are relying on transitive includes. So I decided to take the easy route
out, by adding the `error_messages.h` after the `util.h` to rely on the
transitive includes to get past that compilation error.
I think to properly fix that compile error, this following change (which I
tested locally and works) would be needed:
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h
b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h
index 92a9f457e8a..04f10bcb019 100644
--- a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h
@@ -27,6 +27,7 @@
#define JDWP_JDWP_H
#include "JDWPCommands.h"
+#include "vm_interface.h"
/*
* JDWPCommands.h is the javah'ed version of all the constants defined
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h
b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h
index 4126b76f226..810ab384f1a 100644
--- a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h
@@ -26,6 +26,9 @@
#ifndef JDWP_ERROR_MESSAGES_H
#define JDWP_ERROR_MESSAGES_H
+#include <stdio.h>
+#include "JDWP.h"
+
/* It is assumed that ALL strings are UTF-8 safe on entry */
#define TTY_MESSAGE(args) ( tty_message args )
#define ERROR_MESSAGE(args) ( \
diff --git a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
b/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
index 44f8a463522..0c6ee5b8b6a 100644
--- a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
+++ b/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
@@ -30,9 +30,9 @@
#include <unistd.h>
#include <string.h>
#include <ctype.h>
+#include "error_messages.h"
#include "sys.h"
#include "util.h"
-#include "error_messages.h"
static char *skipWhitespace(char *p) {
while ((*p != '\0') && isspace(*p)) {
Do you think this should be fixed? I can file a JBS issue for it and address it
as a separate PR.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1472425903
