Commit 975fc2d5 authored by Ingo Molnar's avatar Ingo Molnar

Merge branch 'perf' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux-2.6 into perf/core

parents 8e6d5573 ef7b93a1
...@@ -506,8 +506,8 @@ PERFLIBS = $(LIB_FILE) ...@@ -506,8 +506,8 @@ PERFLIBS = $(LIB_FILE)
-include config.mak -include config.mak
ifndef NO_DWARF ifndef NO_DWARF
ifneq ($(shell sh -c "(echo '\#include <dwarf.h>'; echo '\#include <libdw.h>'; echo 'int main(void) { Dwarf *dbg; dbg = dwarf_begin(0, DWARF_C_READ); return (long)dbg; }') | $(CC) -x c - $(ALL_CFLAGS) -I/usr/include/elfutils -ldw -lelf -o $(BITBUCKET) $(ALL_LDFLAGS) $(EXTLIBS) "$(QUIET_STDERR)" && echo y"), y) ifneq ($(shell sh -c "(echo '\#include <dwarf.h>'; echo '\#include <libdw.h>'; echo '\#include <version.h>'; echo '\#ifndef _ELFUTILS_PREREQ'; echo '\#error'; echo '\#endif'; echo 'int main(void) { Dwarf *dbg; dbg = dwarf_begin(0, DWARF_C_READ); return (long)dbg; }') | $(CC) -x c - $(ALL_CFLAGS) -I/usr/include/elfutils -ldw -lelf -o $(BITBUCKET) $(ALL_LDFLAGS) $(EXTLIBS) "$(QUIET_STDERR)" && echo y"), y)
msg := $(warning No libdw.h found or old libdw.h found, disables dwarf support. Please install elfutils-devel/libdw-dev); msg := $(warning No libdw.h found or old libdw.h found or elfutils is older than 0.138, disables dwarf support. Please install new elfutils-devel/libdw-dev);
NO_DWARF := 1 NO_DWARF := 1
endif # Dwarf support endif # Dwarf support
endif # NO_DWARF endif # NO_DWARF
...@@ -560,6 +560,8 @@ ifneq ($(shell sh -c "(echo '\#include <newt.h>'; echo 'int main(void) { newtIni ...@@ -560,6 +560,8 @@ ifneq ($(shell sh -c "(echo '\#include <newt.h>'; echo 'int main(void) { newtIni
msg := $(warning newt not found, disables TUI support. Please install newt-devel or libnewt-dev); msg := $(warning newt not found, disables TUI support. Please install newt-devel or libnewt-dev);
BASIC_CFLAGS += -DNO_NEWT_SUPPORT BASIC_CFLAGS += -DNO_NEWT_SUPPORT
else else
# Fedora has /usr/include/slang/slang.h, but ubuntu /usr/include/slang.h
BASIC_CFLAGS += -I/usr/include/slang
EXTLIBS += -lnewt EXTLIBS += -lnewt
LIB_OBJS += $(OUTPUT)util/newt.o LIB_OBJS += $(OUTPUT)util/newt.o
endif endif
...@@ -592,6 +594,9 @@ endif ...@@ -592,6 +594,9 @@ endif
ifdef NO_DEMANGLE ifdef NO_DEMANGLE
BASIC_CFLAGS += -DNO_DEMANGLE BASIC_CFLAGS += -DNO_DEMANGLE
else ifdef HAVE_CPLUS_DEMANGLE
EXTLIBS += -liberty
BASIC_CFLAGS += -DHAVE_CPLUS_DEMANGLE
else else
has_bfd := $(shell sh -c "(echo '\#include <bfd.h>'; echo 'int main(void) { bfd_demangle(0, 0, 0); return 0; }') | $(CC) -x c - $(ALL_CFLAGS) -o $(BITBUCKET) $(ALL_LDFLAGS) $(EXTLIBS) -lbfd "$(QUIET_STDERR)" && echo y") has_bfd := $(shell sh -c "(echo '\#include <bfd.h>'; echo 'int main(void) { bfd_demangle(0, 0, 0); return 0; }') | $(CC) -x c - $(ALL_CFLAGS) -o $(BITBUCKET) $(ALL_LDFLAGS) $(EXTLIBS) -lbfd "$(QUIET_STDERR)" && echo y")
...@@ -945,6 +950,9 @@ $(OUTPUT)builtin-init-db.o: builtin-init-db.c $(OUTPUT)PERF-CFLAGS ...@@ -945,6 +950,9 @@ $(OUTPUT)builtin-init-db.o: builtin-init-db.c $(OUTPUT)PERF-CFLAGS
$(OUTPUT)util/config.o: util/config.c $(OUTPUT)PERF-CFLAGS $(OUTPUT)util/config.o: util/config.c $(OUTPUT)PERF-CFLAGS
$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) -DETC_PERFCONFIG='"$(ETC_PERFCONFIG_SQ)"' $< $(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) -DETC_PERFCONFIG='"$(ETC_PERFCONFIG_SQ)"' $<
$(OUTPUT)util/newt.o: util/newt.c $(OUTPUT)PERF-CFLAGS
$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) -DENABLE_SLFUTURE_CONST $<
$(OUTPUT)util/rbtree.o: ../../lib/rbtree.c $(OUTPUT)PERF-CFLAGS $(OUTPUT)util/rbtree.o: ../../lib/rbtree.c $(OUTPUT)PERF-CFLAGS
$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) -DETC_PERFCONFIG='"$(ETC_PERFCONFIG_SQ)"' $< $(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) -DETC_PERFCONFIG='"$(ETC_PERFCONFIG_SQ)"' $<
......
...@@ -34,68 +34,8 @@ static bool full_paths; ...@@ -34,68 +34,8 @@ static bool full_paths;
static bool print_line; static bool print_line;
struct sym_hist {
u64 sum;
u64 ip[0];
};
struct sym_ext {
struct rb_node node;
double percent;
char *path;
};
struct sym_priv {
struct sym_hist *hist;
struct sym_ext *ext;
};
static const char *sym_hist_filter; static const char *sym_hist_filter;
static int sym__alloc_hist(struct symbol *self)
{
struct sym_priv *priv = symbol__priv(self);
const int size = (sizeof(*priv->hist) +
(self->end - self->start) * sizeof(u64));
priv->hist = zalloc(size);
return priv->hist == NULL ? -1 : 0;
}
/*
* collect histogram counts
*/
static int annotate__hist_hit(struct hist_entry *he, u64 ip)
{
unsigned int sym_size, offset;
struct symbol *sym = he->ms.sym;
struct sym_priv *priv;
struct sym_hist *h;
if (!sym || !he->ms.map)
return 0;
priv = symbol__priv(sym);
if (priv->hist == NULL && sym__alloc_hist(sym) < 0)
return -ENOMEM;
sym_size = sym->end - sym->start;
offset = ip - sym->start;
pr_debug3("%s: ip=%#Lx\n", __func__, he->ms.map->unmap_ip(he->ms.map, ip));
if (offset >= sym_size)
return 0;
h = priv->hist;
h->sum++;
h->ip[offset]++;
pr_debug3("%#Lx %s: count++ [ip: %#Lx, %#Lx] => %Ld\n", he->ms.sym->start,
he->ms.sym->name, ip, ip - he->ms.sym->start, h->ip[offset]);
return 0;
}
static int hists__add_entry(struct hists *self, struct addr_location *al) static int hists__add_entry(struct hists *self, struct addr_location *al)
{ {
struct hist_entry *he; struct hist_entry *he;
...@@ -115,7 +55,7 @@ static int hists__add_entry(struct hists *self, struct addr_location *al) ...@@ -115,7 +55,7 @@ static int hists__add_entry(struct hists *self, struct addr_location *al)
if (he == NULL) if (he == NULL)
return -ENOMEM; return -ENOMEM;
return annotate__hist_hit(he, al->addr); return hist_entry__inc_addr_samples(he, al->addr);
} }
static int process_sample_event(event_t *event, struct perf_session *session) static int process_sample_event(event_t *event, struct perf_session *session)
...@@ -140,101 +80,6 @@ static int process_sample_event(event_t *event, struct perf_session *session) ...@@ -140,101 +80,6 @@ static int process_sample_event(event_t *event, struct perf_session *session)
return 0; return 0;
} }
struct objdump_line {
struct list_head node;
s64 offset;
char *line;
};
static struct objdump_line *objdump_line__new(s64 offset, char *line)
{
struct objdump_line *self = malloc(sizeof(*self));
if (self != NULL) {
self->offset = offset;
self->line = line;
}
return self;
}
static void objdump_line__free(struct objdump_line *self)
{
free(self->line);
free(self);
}
static void objdump__add_line(struct list_head *head, struct objdump_line *line)
{
list_add_tail(&line->node, head);
}
static struct objdump_line *objdump__get_next_ip_line(struct list_head *head,
struct objdump_line *pos)
{
list_for_each_entry_continue(pos, head, node)
if (pos->offset >= 0)
return pos;
return NULL;
}
static int parse_line(FILE *file, struct hist_entry *he,
struct list_head *head)
{
struct symbol *sym = he->ms.sym;
struct objdump_line *objdump_line;
char *line = NULL, *tmp, *tmp2;
size_t line_len;
s64 line_ip, offset = -1;
char *c;
if (getline(&line, &line_len, file) < 0)
return -1;
if (!line)
return -1;
c = strchr(line, '\n');
if (c)
*c = 0;
line_ip = -1;
/*
* Strip leading spaces:
*/
tmp = line;
while (*tmp) {
if (*tmp != ' ')
break;
tmp++;
}
if (*tmp) {
/*
* Parse hexa addresses followed by ':'
*/
line_ip = strtoull(tmp, &tmp2, 16);
if (*tmp2 != ':')
line_ip = -1;
}
if (line_ip != -1) {
u64 start = map__rip_2objdump(he->ms.map, sym->start);
offset = line_ip - start;
}
objdump_line = objdump_line__new(offset, line);
if (objdump_line == NULL) {
free(line);
return -1;
}
objdump__add_line(head, objdump_line);
return 0;
}
static int objdump_line__print(struct objdump_line *self, static int objdump_line__print(struct objdump_line *self,
struct list_head *head, struct list_head *head,
struct hist_entry *he, u64 len) struct hist_entry *he, u64 len)
...@@ -439,27 +284,11 @@ static void annotate_sym(struct hist_entry *he) ...@@ -439,27 +284,11 @@ static void annotate_sym(struct hist_entry *he)
struct symbol *sym = he->ms.sym; struct symbol *sym = he->ms.sym;
const char *filename = dso->long_name, *d_filename; const char *filename = dso->long_name, *d_filename;
u64 len; u64 len;
char command[PATH_MAX*2];
FILE *file;
LIST_HEAD(head); LIST_HEAD(head);
struct objdump_line *pos, *n; struct objdump_line *pos, *n;
if (!filename) if (hist_entry__annotate(he, &head) < 0)
return;
if (dso->origin == DSO__ORIG_KERNEL) {
if (dso->annotate_warned)
return;
dso->annotate_warned = 1;
pr_err("Can't annotate %s: No vmlinux file was found in the "
"path:\n", sym->name);
vmlinux_path__fprintf(stderr);
return; return;
}
pr_debug("%s: filename=%s, sym=%s, start=%#Lx, end=%#Lx\n", __func__,
filename, sym->name, map->unmap_ip(map, sym->start),
map->unmap_ip(map, sym->end));
if (full_paths) if (full_paths)
d_filename = filename; d_filename = filename;
...@@ -477,29 +306,6 @@ static void annotate_sym(struct hist_entry *he) ...@@ -477,29 +306,6 @@ static void annotate_sym(struct hist_entry *he)
printf(" Percent | Source code & Disassembly of %s\n", d_filename); printf(" Percent | Source code & Disassembly of %s\n", d_filename);
printf("------------------------------------------------\n"); printf("------------------------------------------------\n");
if (verbose >= 2)
printf("annotating [%p] %30s : [%p] %30s\n",
dso, dso->long_name, sym, sym->name);
sprintf(command, "objdump --start-address=0x%016Lx --stop-address=0x%016Lx -dS %s|grep -v %s",
map__rip_2objdump(map, sym->start),
map__rip_2objdump(map, sym->end),
filename, filename);
if (verbose >= 3)
printf("doing: %s\n", command);
file = popen(command, "r");
if (!file)
return;
while (!feof(file)) {
if (parse_line(file, he, &head) < 0)
break;
}
pclose(file);
if (verbose) if (verbose)
hist_entry__print_hits(he); hist_entry__print_hits(he);
......
...@@ -106,8 +106,18 @@ static int perf_session__add_hist_entry(struct perf_session *self, ...@@ -106,8 +106,18 @@ static int perf_session__add_hist_entry(struct perf_session *self,
if (he == NULL) if (he == NULL)
goto out_free_syms; goto out_free_syms;
err = 0; err = 0;
if (symbol_conf.use_callchain) if (symbol_conf.use_callchain) {
err = append_chain(he->callchain, data->callchain, syms); err = append_chain(he->callchain, data->callchain, syms);
if (err)
goto out_free_syms;
}
/*
* Only in the newt browser we are doing integrated annotation,
* so we don't allocated the extra space needed because the stdio
* code will not use it.
*/
if (use_browser)
err = hist_entry__inc_addr_samples(he, al->addr);
out_free_syms: out_free_syms:
free(syms); free(syms);
return err; return err;
...@@ -301,10 +311,7 @@ static int __cmd_report(void) ...@@ -301,10 +311,7 @@ static int __cmd_report(void)
hists__collapse_resort(hists); hists__collapse_resort(hists);
hists__output_resort(hists); hists__output_resort(hists);
if (use_browser) if (use_browser)
perf_session__browse_hists(&hists->entries, hists__browse(hists, help, input_name);
hists->nr_entries,
hists->stats.total, help,
input_name);
else { else {
if (rb_first(&session->hists.entries) == if (rb_first(&session->hists.entries) ==
rb_last(&session->hists.entries)) rb_last(&session->hists.entries))
...@@ -461,6 +468,13 @@ int cmd_report(int argc, const char **argv, const char *prefix __used) ...@@ -461,6 +468,13 @@ int cmd_report(int argc, const char **argv, const char *prefix __used)
if (strcmp(input_name, "-") != 0) if (strcmp(input_name, "-") != 0)
setup_browser(); setup_browser();
/*
* Only in the newt browser we are doing integrated annotation,
* so don't allocate extra space that won't be used in the stdio
* implementation.
*/
if (use_browser)
symbol_conf.priv_size = sizeof(struct sym_priv);
if (symbol__init() < 0) if (symbol__init() < 0)
return -1; return -1;
......
...@@ -784,3 +784,246 @@ size_t hists__fprintf(struct hists *self, struct hists *pair, ...@@ -784,3 +784,246 @@ size_t hists__fprintf(struct hists *self, struct hists *pair,
return ret; return ret;
} }
enum hist_filter {
HIST_FILTER__DSO,
HIST_FILTER__THREAD,
};
void hists__filter_by_dso(struct hists *self, const struct dso *dso)
{
struct rb_node *nd;
self->nr_entries = self->stats.total = 0;
self->max_sym_namelen = 0;
for (nd = rb_first(&self->entries); nd; nd = rb_next(nd)) {
struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
if (symbol_conf.exclude_other && !h->parent)
continue;
if (dso != NULL && (h->ms.map == NULL || h->ms.map->dso != dso)) {
h->filtered |= (1 << HIST_FILTER__DSO);
continue;
}
h->filtered &= ~(1 << HIST_FILTER__DSO);
if (!h->filtered) {
++self->nr_entries;
self->stats.total += h->count;
if (h->ms.sym &&
self->max_sym_namelen < h->ms.sym->namelen)
self->max_sym_namelen = h->ms.sym->namelen;
}
}
}
void hists__filter_by_thread(struct hists *self, const struct thread *thread)
{
struct rb_node *nd;
self->nr_entries = self->stats.total = 0;
self->max_sym_namelen = 0;
for (nd = rb_first(&self->entries); nd; nd = rb_next(nd)) {
struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
if (thread != NULL && h->thread != thread) {
h->filtered |= (1 << HIST_FILTER__THREAD);
continue;
}
h->filtered &= ~(1 << HIST_FILTER__THREAD);
if (!h->filtered) {
++self->nr_entries;
self->stats.total += h->count;
if (h->ms.sym &&
self->max_sym_namelen < h->ms.sym->namelen)
self->max_sym_namelen = h->ms.sym->namelen;
}
}
}
static int symbol__alloc_hist(struct symbol *self)
{
struct sym_priv *priv = symbol__priv(self);
const int size = (sizeof(*priv->hist) +
(self->end - self->start) * sizeof(u64));
priv->hist = zalloc(size);
return priv->hist == NULL ? -1 : 0;
}
int hist_entry__inc_addr_samples(struct hist_entry *self, u64 ip)
{
unsigned int sym_size, offset;
struct symbol *sym = self->ms.sym;
struct sym_priv *priv;
struct sym_hist *h;
if (!sym || !self->ms.map)
return 0;
priv = symbol__priv(sym);
if (priv->hist == NULL && symbol__alloc_hist(sym) < 0)
return -ENOMEM;
sym_size = sym->end - sym->start;
offset = ip - sym->start;
pr_debug3("%s: ip=%#Lx\n", __func__, self->ms.map->unmap_ip(self->ms.map, ip));
if (offset >= sym_size)
return 0;
h = priv->hist;
h->sum++;
h->ip[offset]++;
pr_debug3("%#Lx %s: count++ [ip: %#Lx, %#Lx] => %Ld\n", self->ms.sym->start,
self->ms.sym->name, ip, ip - self->ms.sym->start, h->ip[offset]);
return 0;
}
static struct objdump_line *objdump_line__new(s64 offset, char *line)
{
struct objdump_line *self = malloc(sizeof(*self));
if (self != NULL) {
self->offset = offset;
self->line = line;
}
return self;
}
void objdump_line__free(struct objdump_line *self)
{
free(self->line);
free(self);
}
static void objdump__add_line(struct list_head *head, struct objdump_line *line)
{
list_add_tail(&line->node, head);
}
struct objdump_line *objdump__get_next_ip_line(struct list_head *head,
struct objdump_line *pos)
{
list_for_each_entry_continue(pos, head, node)
if (pos->offset >= 0)
return pos;
return NULL;
}
static int hist_entry__parse_objdump_line(struct hist_entry *self, FILE *file,
struct list_head *head)
{
struct symbol *sym = self->ms.sym;
struct objdump_line *objdump_line;
char *line = NULL, *tmp, *tmp2, *c;
size_t line_len;
s64 line_ip, offset = -1;
if (getline(&line, &line_len, file) < 0)
return -1;
if (!line)
return -1;
while (line_len != 0 && isspace(line[line_len - 1]))
line[--line_len] = '\0';
c = strchr(line, '\n');
if (c)
*c = 0;
line_ip = -1;
/*
* Strip leading spaces:
*/
tmp = line;
while (*tmp) {
if (*tmp != ' ')
break;
tmp++;
}
if (*tmp) {
/*
* Parse hexa addresses followed by ':'
*/
line_ip = strtoull(tmp, &tmp2, 16);
if (*tmp2 != ':')
line_ip = -1;
}
if (line_ip != -1) {
u64 start = map__rip_2objdump(self->ms.map, sym->start);
offset = line_ip - start;
}
objdump_line = objdump_line__new(offset, line);
if (objdump_line == NULL) {
free(line);
return -1;
}
objdump__add_line(head, objdump_line);
return 0;
}
int hist_entry__annotate(struct hist_entry *self, struct list_head *head)
{
struct symbol *sym = self->ms.sym;
struct map *map = self->ms.map;
struct dso *dso = map->dso;
const char *filename = dso->long_name;
char command[PATH_MAX * 2];
FILE *file;
u64 len;
if (!filename)
return -1;
if (dso->origin == DSO__ORIG_KERNEL) {
if (dso->annotate_warned)
return 0;
dso->annotate_warned = 1;
pr_err("Can't annotate %s: No vmlinux file was found in the "
"path:\n", sym->name);
vmlinux_path__fprintf(stderr);
return -1;
}
pr_debug("%s: filename=%s, sym=%s, start=%#Lx, end=%#Lx\n", __func__,
filename, sym->name, map->unmap_ip(map, sym->start),
map->unmap_ip(map, sym->end));
len = sym->end - sym->start;
pr_debug("annotating [%p] %30s : [%p] %30s\n",
dso, dso->long_name, sym, sym->name);
snprintf(command, sizeof(command),
"objdump --start-address=0x%016Lx --stop-address=0x%016Lx -dS %s|grep -v %s|expand",
map__rip_2objdump(map, sym->start),
map__rip_2objdump(map, sym->end),
filename, filename);
pr_debug("Executing: %s\n", command);
file = popen(command, "r");
if (!file)
return -1;
while (!feof(file))
if (hist_entry__parse_objdump_line(self, file, head) < 0)
break;
pclose(file);
return 0;
}
...@@ -11,6 +11,32 @@ struct addr_location; ...@@ -11,6 +11,32 @@ struct addr_location;
struct symbol; struct symbol;
struct rb_root; struct rb_root;
struct objdump_line {
struct list_head node;
s64 offset;
char *line;
};
void objdump_line__free(struct objdump_line *self);
struct objdump_line *objdump__get_next_ip_line(struct list_head *head,
struct objdump_line *pos);
struct sym_hist {
u64 sum;
u64 ip[0];
};
struct sym_ext {
struct rb_node node;
double percent;
char *path;
};
struct sym_priv {
struct sym_hist *hist;
struct sym_ext *ext;
};
struct events_stats { struct events_stats {
u64 total; u64 total;
u64 lost; u64 lost;
...@@ -44,4 +70,22 @@ void hists__output_resort(struct hists *self); ...@@ -44,4 +70,22 @@ void hists__output_resort(struct hists *self);
void hists__collapse_resort(struct hists *self); void hists__collapse_resort(struct hists *self);
size_t hists__fprintf(struct hists *self, struct hists *pair, size_t hists__fprintf(struct hists *self, struct hists *pair,
bool show_displacement, FILE *fp); bool show_displacement, FILE *fp);
int hist_entry__inc_addr_samples(struct hist_entry *self, u64 ip);
int hist_entry__annotate(struct hist_entry *self, struct list_head *head);
void hists__filter_by_dso(struct hists *self, const struct dso *dso);
void hists__filter_by_thread(struct hists *self, const struct thread *thread);
#ifdef NO_NEWT_SUPPORT
static inline int hists__browse(struct hists self __used,
const char *helpline __used,
const char *input_name __used)
{
return 0;
}
#else
int hists__browse(struct hists *self, const char *helpline,
const char *input_name);
#endif
#endif /* __PERF_HIST_H */ #endif /* __PERF_HIST_H */
This diff is collapsed.
...@@ -102,21 +102,6 @@ int perf_session__create_kernel_maps(struct perf_session *self); ...@@ -102,21 +102,6 @@ int perf_session__create_kernel_maps(struct perf_session *self);
int do_read(int fd, void *buf, size_t size); int do_read(int fd, void *buf, size_t size);
void perf_session__update_sample_type(struct perf_session *self); void perf_session__update_sample_type(struct perf_session *self);
#ifdef NO_NEWT_SUPPORT
static inline int perf_session__browse_hists(struct rb_root *hists __used,
u64 nr_hists __used,
u64 session_total __used,
const char *helpline __used,
const char *input_name __used)
{
return 0;
}
#else
int perf_session__browse_hists(struct rb_root *hists, u64 nr_hists,
u64 session_total, const char *helpline,
const char *input_name);
#endif
static inline static inline
struct machine *perf_session__find_host_machine(struct perf_session *self) struct machine *perf_session__find_host_machine(struct perf_session *self)
{ {
......
...@@ -48,12 +48,6 @@ struct hist_entry { ...@@ -48,12 +48,6 @@ struct hist_entry {
u64 count_us; u64 count_us;
u64 count_guest_sys; u64 count_guest_sys;
u64 count_guest_us; u64 count_guest_us;
/*
* XXX WARNING!
* thread _has_ to come after ms, see
* hist_browser__selected_thread in util/newt.c
*/
struct map_symbol ms; struct map_symbol ms;
struct thread *thread; struct thread *thread;
u64 ip; u64 ip;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment