Skip to content
Projects
Groups
Snippets
Help
This project
Loading...
Sign in / Register
Toggle navigation
W
wine-winehq
Project
Project
Details
Activity
Cycle Analytics
Repository
Repository
Files
Commits
Branches
Tags
Contributors
Graph
Compare
Charts
Issues
0
Issues
0
List
Board
Labels
Milestones
Merge Requests
0
Merge Requests
0
CI / CD
CI / CD
Pipelines
Jobs
Schedules
Charts
Registry
Registry
Wiki
Wiki
Snippets
Snippets
Members
Members
Collapse sidebar
Close sidebar
Activity
Graph
Charts
Create a new issue
Jobs
Commits
Issue Boards
Open sidebar
wine
wine-winehq
Commits
6a232560
Commit
6a232560
authored
Oct 04, 2021
by
Alexandre Julliard
Browse files
Options
Browse Files
Download
Email Patches
Plain Diff
winedbg: Use winsock for the gdb socket.
Signed-off-by:
Alexandre Julliard
<
julliard@winehq.org
>
parent
6bc71db0
Hide whitespace changes
Inline
Side-by-side
Showing
2 changed files
with
122 additions
and
152 deletions
+122
-152
Makefile.in
programs/winedbg/Makefile.in
+1
-2
gdbproxy.c
programs/winedbg/gdbproxy.c
+121
-150
No files found.
programs/winedbg/Makefile.in
View file @
6a232560
MODULE
=
winedbg.exe
IMPORTS
=
dbghelp advapi32
DELAYIMPORTS
=
comdlg32 shell32 comctl32 user32 gdi32
EXTRALIBS
=
$(POLL_LIBS)
DELAYIMPORTS
=
comdlg32 shell32 comctl32 user32 gdi32 ws2_32
EXTRADLLFLAGS
=
-mconsole
-mcygwin
...
...
programs/winedbg/gdbproxy.c
View file @
6a232560
...
...
@@ -25,39 +25,22 @@
*/
#include "config.h"
#include "wine/port.h"
#define NONAMELESSUNION
#define NONAMELESSSTRUCT
#include "ntstatus.h"
#define WIN32_NO_STATUS
#include "winsock2.h"
#include <assert.h>
#include <errno.h>
#include <fcntl.h>
#include <signal.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#ifdef HAVE_SYS_POLL_H
# include <sys/poll.h>
#endif
#ifdef HAVE_SYS_WAIT_H
# include <sys/wait.h>
#endif
#ifdef HAVE_SYS_SOCKET_H
# include <sys/socket.h>
#endif
#ifdef HAVE_NETINET_IN_H
# include <netinet/in.h>
#endif
#ifdef HAVE_NETINET_TCP_H
# include <netinet/tcp.h>
#endif
#ifdef HAVE_UNISTD_H
# include <unistd.h>
#endif
/* if we don't have poll support on this system
* we won't provide gdb proxy support here...
*/
#ifdef HAVE_POLL
#include "debugger.h"
#include "windef.h"
...
...
@@ -82,7 +65,7 @@ struct gdb_xpoint
struct
gdb_context
{
/* gdb information */
int
sock
;
SOCKET
sock
;
/* incoming buffer */
char
*
in_buf
;
int
in_buf_alloc
;
...
...
@@ -109,6 +92,27 @@ struct gdb_context
BOOL
no_ack_mode
;
};
/* assume standard signal and errno values */
enum
host_error
{
HOST_ESRCH
=
3
,
HOST_EFAULT
=
14
,
HOST_EINVAL
=
22
,
};
enum
host_signal
{
HOST_SIGINT
=
2
,
HOST_SIGILL
=
4
,
HOST_SIGTRAP
=
5
,
HOST_SIGABRT
=
6
,
HOST_SIGFPE
=
8
,
HOST_SIGBUS
=
10
,
HOST_SIGSEGV
=
11
,
HOST_SIGALRM
=
14
,
HOST_SIGTERM
=
15
,
};
static
void
gdbctx_delete_xpoint
(
struct
gdb_context
*
gdbctx
,
struct
dbg_thread
*
thread
,
dbg_ctx_t
*
ctx
,
struct
gdb_xpoint
*
x
)
{
...
...
@@ -314,9 +318,9 @@ static unsigned char signal_from_debug_event(DEBUG_EVENT* de)
DWORD
ec
;
if
(
de
->
dwDebugEventCode
==
EXIT_PROCESS_DEBUG_EVENT
)
return
SIGTERM
;
return
HOST_
SIGTERM
;
if
(
de
->
dwDebugEventCode
!=
EXCEPTION_DEBUG_EVENT
)
return
SIGTRAP
;
return
HOST_
SIGTRAP
;
ec
=
de
->
u
.
Exception
.
ExceptionRecord
.
ExceptionCode
;
switch
(
ec
)
...
...
@@ -325,12 +329,12 @@ static unsigned char signal_from_debug_event(DEBUG_EVENT* de)
case
EXCEPTION_PRIV_INSTRUCTION
:
case
EXCEPTION_STACK_OVERFLOW
:
case
EXCEPTION_GUARD_PAGE
:
return
SIGSEGV
;
return
HOST_
SIGSEGV
;
case
EXCEPTION_DATATYPE_MISALIGNMENT
:
return
SIGBUS
;
return
HOST_
SIGBUS
;
case
EXCEPTION_SINGLE_STEP
:
case
EXCEPTION_BREAKPOINT
:
return
SIGTRAP
;
return
HOST_
SIGTRAP
;
case
EXCEPTION_FLT_DENORMAL_OPERAND
:
case
EXCEPTION_FLT_DIVIDE_BY_ZERO
:
case
EXCEPTION_FLT_INEXACT_RESULT
:
...
...
@@ -338,23 +342,23 @@ static unsigned char signal_from_debug_event(DEBUG_EVENT* de)
case
EXCEPTION_FLT_OVERFLOW
:
case
EXCEPTION_FLT_STACK_CHECK
:
case
EXCEPTION_FLT_UNDERFLOW
:
return
SIGFPE
;
return
HOST_
SIGFPE
;
case
EXCEPTION_INT_DIVIDE_BY_ZERO
:
case
EXCEPTION_INT_OVERFLOW
:
return
SIGFPE
;
return
HOST_
SIGFPE
;
case
EXCEPTION_ILLEGAL_INSTRUCTION
:
return
SIGILL
;
return
HOST_
SIGILL
;
case
CONTROL_C_EXIT
:
return
SIGINT
;
return
HOST_
SIGINT
;
case
STATUS_POSSIBLE_DEADLOCK
:
return
SIGALRM
;
return
HOST_
SIGALRM
;
/* should not be here */
case
EXCEPTION_INVALID_HANDLE
:
case
EXCEPTION_WINE_NAME_THREAD
:
return
SIGTRAP
;
return
HOST_
SIGTRAP
;
default:
ERR
(
"Unknown exception code 0x%08x
\n
"
,
ec
);
return
SIGABRT
;
return
HOST_
SIGABRT
;
}
}
...
...
@@ -551,29 +555,26 @@ static void handle_step_or_continue(struct gdb_context* gdbctx, int tid, BOOL st
static
BOOL
check_for_interrupt
(
struct
gdb_context
*
gdbctx
)
{
struct
pollfd
pollfd
;
int
ret
;
char
pkt
;
pollfd
.
fd
=
gdbctx
->
sock
;
pollfd
.
events
=
POLLIN
;
pollfd
.
revents
=
0
;
if
((
ret
=
poll
(
&
pollfd
,
1
,
0
))
==
1
)
{
ret
=
read
(
gdbctx
->
sock
,
&
pkt
,
1
);
if
(
ret
!=
1
)
{
ERR
(
"read failed
\n
"
);
return
FALSE
;
}
if
(
pkt
!=
'\003'
)
{
ERR
(
"Unexpected break packet %#02x
\n
"
,
pkt
);
return
FALSE
;
}
return
TRUE
;
}
else
if
(
ret
==
-
1
)
{
ERR
(
"poll failed
\n
"
);
}
return
FALSE
;
fd_set
read_fds
;
struct
timeval
tv
=
{
0
,
0
};
FD_ZERO
(
&
read_fds
);
FD_SET
(
gdbctx
->
sock
,
&
read_fds
);
if
(
select
(
0
,
&
read_fds
,
NULL
,
NULL
,
&
tv
)
>
0
)
{
if
(
recv
(
gdbctx
->
sock
,
&
pkt
,
1
,
0
)
!=
1
)
{
ERR
(
"read failed
\n
"
);
return
FALSE
;
}
if
(
pkt
!=
'\003'
)
{
ERR
(
"Unexpected break packet %#02x
\n
"
,
pkt
);
return
FALSE
;
}
return
TRUE
;
}
return
FALSE
;
}
static
void
wait_for_debuggee
(
struct
gdb_context
*
gdbctx
)
...
...
@@ -638,12 +639,8 @@ static void get_process_info(struct gdb_context* gdbctx, char* buffer, size_t le
switch
(
GetPriorityClass
(
gdbctx
->
process
->
handle
))
{
case
0
:
break
;
#ifdef ABOVE_NORMAL_PRIORITY_CLASS
case
ABOVE_NORMAL_PRIORITY_CLASS
:
strcat
(
buffer
,
", above normal priority"
);
break
;
#endif
#ifdef BELOW_NORMAL_PRIORITY_CLASS
case
BELOW_NORMAL_PRIORITY_CLASS
:
strcat
(
buffer
,
", below normal priority"
);
break
;
#endif
case
HIGH_PRIORITY_CLASS
:
strcat
(
buffer
,
", high priority"
);
break
;
case
IDLE_PRIORITY_CLASS
:
strcat
(
buffer
,
", idle priority"
);
break
;
case
NORMAL_PRIORITY_CLASS
:
strcat
(
buffer
,
", normal priority"
);
break
;
...
...
@@ -925,7 +922,7 @@ static enum packet_return packet_reply_status(struct gdb_context* gdbctx)
case
UNLOAD_DLL_DEBUG_EVENT
:
packet_reply_open
(
gdbctx
);
packet_reply_add
(
gdbctx
,
"T"
);
packet_reply_val
(
gdbctx
,
SIGTRAP
,
1
);
packet_reply_val
(
gdbctx
,
HOST_
SIGTRAP
,
1
);
packet_reply_add
(
gdbctx
,
"library:;"
);
packet_reply_close
(
gdbctx
);
return
packet_done
;
...
...
@@ -1219,7 +1216,7 @@ static enum packet_return packet_read_memory(struct gdb_context* gdbctx)
buffer
,
blk_len
,
&
r
)
||
r
==
0
)
{
/* fail at first address, return error */
if
(
nread
==
0
)
return
packet_reply_error
(
gdbctx
,
EFAULT
);
if
(
nread
==
0
)
return
packet_reply_error
(
gdbctx
,
HOST_EFAULT
);
/* something has already been read, return partial information */
break
;
}
...
...
@@ -1535,7 +1532,7 @@ static enum packet_return packet_query_remote_command(struct gdb_context* gdbctx
(
qd
->
handler
)(
gdbctx
,
len
-
qd
->
len
,
buffer
+
qd
->
len
);
return
packet_done
;
}
return
packet_reply_error
(
gdbctx
,
EINVAL
);
return
packet_reply_error
(
gdbctx
,
HOST_EINVAL
);
}
static
BOOL
CALLBACK
packet_query_libraries_cb
(
PCSTR
mod_name
,
DWORD64
base
,
PVOID
ctx
)
...
...
@@ -1960,10 +1957,10 @@ static enum packet_return packet_thread_alive(struct gdb_context* gdbctx)
tid
=
strtol
(
gdbctx
->
in_packet
,
&
end
,
16
);
if
(
tid
==
-
1
||
tid
==
0
)
return
packet_reply_error
(
gdbctx
,
EINVAL
);
return
packet_reply_error
(
gdbctx
,
HOST_EINVAL
);
if
(
dbg_get_thread
(
gdbctx
->
process
,
tid
)
!=
NULL
)
return
packet_ok
;
return
packet_reply_error
(
gdbctx
,
ESRCH
);
return
packet_reply_error
(
gdbctx
,
HOST_ESRCH
);
}
/* =============================================== *
...
...
@@ -2024,13 +2021,13 @@ static BOOL extract_packets(struct gdb_context* gdbctx)
if
(
cksum
==
checksum
(
ptr
+
1
,
len
))
{
TRACE
(
"Acking: %s
\n
"
,
debugstr_an
(
ptr
,
sum
-
ptr
));
write
(
gdbctx
->
sock
,
"+"
,
1
);
send
(
gdbctx
->
sock
,
"+"
,
1
,
0
);
}
else
{
ERR
(
"Nacking: %s (checksum: %d != %d)
\n
"
,
debugstr_an
(
ptr
,
sum
-
ptr
),
cksum
,
checksum
(
ptr
+
1
,
len
));
write
(
gdbctx
->
sock
,
"-"
,
1
);
send
(
gdbctx
->
sock
,
"-"
,
1
,
0
);
}
}
...
...
@@ -2071,7 +2068,7 @@ static BOOL extract_packets(struct gdb_context* gdbctx)
}
TRACE
(
"Reply: %s
\n
"
,
debugstr_an
(
gdbctx
->
out_buf
,
gdbctx
->
out_len
));
i
=
write
(
gdbctx
->
sock
,
gdbctx
->
out_buf
,
gdbctx
->
out_len
);
i
=
send
(
gdbctx
->
sock
,
gdbctx
->
out_buf
,
gdbctx
->
out_len
,
0
);
assert
(
i
==
gdbctx
->
out_len
);
gdbctx
->
out_len
=
0
;
}
...
...
@@ -2098,7 +2095,7 @@ static int fetch_data(struct gdb_context* gdbctx)
if
(
gdbctx
->
in_len
+
STEP
>
gdbctx
->
in_buf_alloc
)
gdbctx
->
in_buf
=
packet_realloc
(
gdbctx
->
in_buf
,
gdbctx
->
in_buf_alloc
+=
STEP
);
#undef STEP
len
=
re
ad
(
gdbctx
->
sock
,
gdbctx
->
in_buf
+
gdbctx
->
in_len
,
gdbctx
->
in_buf_alloc
-
gdbctx
->
in_len
-
1
);
len
=
re
cv
(
gdbctx
->
sock
,
gdbctx
->
in_buf
+
gdbctx
->
in_len
,
gdbctx
->
in_buf_alloc
-
gdbctx
->
in_len
-
1
,
0
);
if
(
len
<=
0
)
break
;
gdbctx
->
in_len
+=
len
;
assert
(
gdbctx
->
in_len
<=
gdbctx
->
in_buf_alloc
);
...
...
@@ -2118,6 +2115,7 @@ static BOOL gdb_exec(unsigned port, unsigned flags)
int
fd
;
const
char
*
gdb_path
,
*
tmp_path
;
FILE
*
f
;
const
char
*
argv
[
6
];
if
(
!
(
gdb_path
=
getenv
(
"WINE_GDB"
)))
gdb_path
=
"gdb"
;
if
(
!
(
tmp_path
=
getenv
(
"TMPDIR"
)))
tmp_path
=
"/tmp"
;
...
...
@@ -2141,36 +2139,44 @@ static BOOL gdb_exec(unsigned port, unsigned flags)
/* tell gdb to delete this file when done handling it... */
fprintf
(
f
,
"shell rm -f
\"
%s
\"\n
"
,
buf
);
fclose
(
f
);
argv
[
0
]
=
"xterm"
;
argv
[
1
]
=
"-e"
;
argv
[
2
]
=
gdb_path
;
argv
[
3
]
=
"-x"
;
argv
[
4
]
=
buf
;
argv
[
5
]
=
NULL
;
if
(
flags
&
FLAG_WITH_XTERM
)
execlp
(
"xterm"
,
"xterm"
,
"-e"
,
gdb_path
,
"-x"
,
buf
,
NULL
);
__wine_unix_spawnvp
(
(
char
**
)
argv
,
FALSE
);
else
execlp
(
gdb_path
,
gdb_path
,
"-x"
,
buf
,
NULL
);
assert
(
0
);
/* never reached */
__wine_unix_spawnvp
(
(
char
**
)
argv
+
2
,
FALSE
);
return
TRUE
;
}
static
BOOL
gdb_startup
(
struct
gdb_context
*
gdbctx
,
unsigned
flags
,
unsigned
port
)
{
int
sock
;
struct
sockaddr_in
s_addrs
=
{
0
};
socklen_t
s_len
=
sizeof
(
s_addrs
);
struct
pollfd
pollfd
;
SOCKET
sock
;
struct
sockaddr_in
s_addrs
=
{
0
};
int
s_len
=
sizeof
(
s_addrs
);
fd_set
read_fds
;
WSADATA
data
;
BOOL
ret
=
FALSE
;
WSAStartup
(
MAKEWORD
(
2
,
2
),
&
data
);
/* step 1: create socket for gdb connection request */
if
((
sock
=
socket
(
AF_INET
,
SOCK_STREAM
,
0
))
==
-
1
)
if
((
sock
=
socket
(
AF_INET
,
SOCK_STREAM
,
0
))
==
INVALID_SOCKET
)
{
ERR
(
"Failed to create socket: %
s
\n
"
,
strerror
(
errno
));
ERR
(
"Failed to create socket: %
u
\n
"
,
WSAGetLastError
(
));
return
FALSE
;
}
s_addrs
.
sin_family
=
AF_INET
;
s_addrs
.
sin_addr
.
s
_addr
=
INADDR_ANY
;
s_addrs
.
sin_addr
.
S_un
.
S
_addr
=
INADDR_ANY
;
s_addrs
.
sin_port
=
htons
(
port
);
if
(
bind
(
sock
,
(
struct
sockaddr
*
)
&
s_addrs
,
sizeof
(
s_addrs
))
==
-
1
)
goto
cleanup
;
if
(
listen
(
sock
,
1
)
==
-
1
||
getsockname
(
sock
,
(
struct
sockaddr
*
)
&
s_addrs
,
&
s_len
)
==
-
1
)
if
(
listen
(
sock
,
1
)
==
-
1
||
getsockname
(
sock
,
(
struct
sockaddr
*
)
&
s_addrs
,
&
s_len
)
==
-
1
)
goto
cleanup
;
/* step 2: do the process internal creation */
...
...
@@ -2180,54 +2186,31 @@ static BOOL gdb_startup(struct gdb_context* gdbctx, unsigned flags, unsigned por
if
(
flags
&
FLAG_NO_START
)
fprintf
(
stderr
,
"target remote localhost:%d
\n
"
,
ntohs
(
s_addrs
.
sin_port
));
else
switch
(
fork
())
{
case
-
1
:
/* error in parent... */
ERR
(
"Failed to start gdb: fork: %s
\n
"
,
strerror
(
errno
));
goto
cleanup
;
default:
/* in parent... success */
signal
(
SIGINT
,
SIG_IGN
);
break
;
case
0
:
/* in child... and alive */
gdb_exec
(
s_addrs
.
sin_port
,
flags
);
/* if we're here, exec failed, so report failure */
goto
cleanup
;
}
gdb_exec
(
s_addrs
.
sin_port
,
flags
);
/* step 4: wait for gdb to connect actually */
pollfd
.
fd
=
sock
;
pollfd
.
events
=
POLLIN
;
pollfd
.
revents
=
0
;
FD_ZERO
(
&
read_fds
);
FD_SET
(
sock
,
&
read_fds
);
switch
(
poll
(
&
pollfd
,
1
,
-
1
)
)
if
(
select
(
0
,
&
read_fds
,
NULL
,
NULL
,
NULL
)
>
0
)
{
case
1
:
if
(
pollfd
.
revents
&
POLLIN
)
int
dummy
=
1
;
gdbctx
->
sock
=
accept
(
sock
,
(
struct
sockaddr
*
)
&
s_addrs
,
&
s_len
);
if
(
gdbctx
->
sock
!=
INVALID_SOCKET
)
{
int
dummy
=
1
;
gdbctx
->
sock
=
accept
(
sock
,
(
struct
sockaddr
*
)
&
s_addrs
,
&
s_len
);
if
(
gdbctx
->
sock
==
-
1
)
break
;
ret
=
TRUE
;
TRACE
(
"connected on %
d
\n
"
,
gdbctx
->
sock
);
TRACE
(
"connected on %
lu
\n
"
,
gdbctx
->
sock
);
/* don't keep our small packets too long: send them ASAP back to GDB
* without this, GDB really crawls
*/
setsockopt
(
gdbctx
->
sock
,
IPPROTO_TCP
,
TCP_NODELAY
,
(
char
*
)
&
dummy
,
sizeof
(
dummy
));
}
break
;
case
0
:
ERR
(
"Timed out connecting to gdb
\n
"
);
break
;
case
-
1
:
ERR
(
"Failed to connect to gdb: poll: %s
\n
"
,
strerror
(
errno
));
break
;
default:
assert
(
0
);
}
else
ERR
(
"Failed to connect to gdb: %u
\n
"
,
WSAGetLastError
());
cleanup:
close
(
sock
);
close
socket
(
sock
);
return
ret
;
}
...
...
@@ -2235,7 +2218,7 @@ static BOOL gdb_init_context(struct gdb_context* gdbctx, unsigned flags, unsigne
{
int
i
;
gdbctx
->
sock
=
-
1
;
gdbctx
->
sock
=
INVALID_SOCKET
;
gdbctx
->
in_buf
=
NULL
;
gdbctx
->
in_buf_alloc
=
0
;
gdbctx
->
in_len
=
0
;
...
...
@@ -2272,50 +2255,40 @@ static BOOL gdb_init_context(struct gdb_context* gdbctx, unsigned flags, unsigne
static
int
gdb_remote
(
unsigned
flags
,
unsigned
port
)
{
struct
pollfd
pollfd
;
struct
gdb_context
gdbctx
;
BOOL
doLoop
;
for
(
doLoop
=
gdb_init_context
(
&
gdbctx
,
flags
,
port
);
doLoop
;)
if
(
!
gdb_init_context
(
&
gdbctx
,
flags
,
port
))
return
0
;
for
(;;)
{
pollfd
.
fd
=
gdbctx
.
sock
;
pollfd
.
events
=
POLLIN
;
pollfd
.
revents
=
0
;
fd_set
read_fds
,
err_fds
;
FD_ZERO
(
&
read_fds
);
FD_ZERO
(
&
err_fds
);
FD_SET
(
gdbctx
.
sock
,
&
read_fds
);
FD_SET
(
gdbctx
.
sock
,
&
err_fds
);
switch
(
poll
(
&
pollfd
,
1
,
-
1
))
if
(
select
(
0
,
&
read_fds
,
NULL
,
&
err_fds
,
NULL
)
==
-
1
)
break
;
if
(
FD_ISSET
(
gdbctx
.
sock
,
&
err_fds
))
{
case
1
:
/* got something */
if
(
pollfd
.
revents
&
(
POLLHUP
|
POLLERR
))
{
ERR
(
"gdb hung up
\n
"
);
/* kill also debuggee process - questionnable - */
detach_debuggee
(
&
gdbctx
,
TRUE
);
doLoop
=
FALSE
;
break
;
}
if
((
pollfd
.
revents
&
POLLIN
)
&&
fetch_data
(
&
gdbctx
)
>
0
)
ERR
(
"gdb hung up
\n
"
);
/* kill also debuggee process - questionnable - */
detach_debuggee
(
&
gdbctx
,
TRUE
);
break
;
}
if
(
FD_ISSET
(
gdbctx
.
sock
,
&
read_fds
))
{
if
(
fetch_data
(
&
gdbctx
)
>
0
)
{
if
(
extract_packets
(
&
gdbctx
))
doLoop
=
FALSE
;
if
(
extract_packets
(
&
gdbctx
))
break
;
}
break
;
case
0
:
/* timeout, should never happen (infinite timeout) */
break
;
case
-
1
:
ERR
(
"poll failed: %s
\n
"
,
strerror
(
errno
));
doLoop
=
FALSE
;
break
;
}
}
wait
(
NULL
);
return
0
;
}
#endif
int
gdb_main
(
int
argc
,
char
*
argv
[])
{
#ifdef HAVE_POLL
unsigned
gdb_flags
=
0
,
port
=
0
;
char
*
port_end
;
...
...
@@ -2350,8 +2323,6 @@ int gdb_main(int argc, char* argv[])
if
(
dbg_active_attach
(
argc
,
argv
)
==
start_ok
||
dbg_active_launch
(
argc
,
argv
)
==
start_ok
)
return
gdb_remote
(
gdb_flags
,
port
);
#else
fprintf
(
stderr
,
"GdbProxy mode not supported on this platform
\n
"
);
#endif
return
-
1
;
}
Write
Preview
Markdown
is supported
0%
Try again
or
attach a new file
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment