Hi Petr,
Since we still have a lot of warnings/errors by default and most of them are duplicated a lot of times for larger object it makes sense to me to default the printing of duplicate messages to something like 16. The attached patch does this. Do you think this is a good idea?
Thanks,
Mark
If limiting output is the default, it should say something like "more <flavor> errors follow" as ld does for some cases.
On Fri, 2011-04-08 at 07:10 -0700, Roland McGrath wrote:
If limiting output is the default, it should say something like "more <flavor> errors follow" as ld does for some cases.
It does, it says:
(threshold [--dups=16] reached for the following message) warning: .debug_info: DIE 0x286a21: DW_AT_low_pc value not below DW_AT_high_pc.
Mark Wielaard mjw@redhat.com writes:
Since we still have a lot of warnings/errors by default and most of them are duplicated a lot of times for larger object it makes sense to me to default the printing of duplicate messages to something like 16. The attached patch does this. Do you think this is a good idea?
Yes. I don't like the threshold message very much (I'd like it to be printed _after_ the message in question, but that's not easily done with C++ streams), but it's not that bad, and the feature is useful.
Note that there are a couple things one needs to do to support the duplication filtering.
The message counting is keyed by the address of the first string sent to the stream and counted. That means that non-unique strings that are merged by GCC are not considered separate messages, and that dynamically generated strings are not filtered at all. This can be mended by stating the key explicitly like so:
wr_message (wh, mc_error).id (msgid) << "blah" //...
There's currently no standard way to obtain msgids. It's simply a void* as of now. I'm using check descriptor in a couple places as the id, with the downside that all messages in one check get one counter. I guess "this" might be used, or instance variable addresses, etc., but really it's all a bit too ad-hoc at the moment. Eventually, I hope to have some sort of assignment of message IDs akin to how command line options are handled now.
Another thing, when several messages are related, we want them to be filtered consistently. There's a bit of code in there to support this:
bool shown = false; wr_message (wh, mc_error).id (descriptor (), shown) << "attribute `" << dwarf::attributes::name ((*at).first) //... // this can repeat a couple times, e.g. in a loop wr_message (wh_parent, mc_error).when (shown) << "in this context: " << cov::format_ranges (cov) // ...
The "shown" flag is set to true when the message is displayed and is untouched otherwise, so it can later be used to decide whether the final context message should be printed or not.
In DIE checks, one might use this to throw check_base::unscheduled to turn off the check when the sole message is not displayed anymore, but I don't think that's too important.
Hope this all makes some sort of sense.
Thanks, PM
It seems pretty straightforward to define a tiny object for each message, which has a method to emit the message and a dup counter. (These objects would all live just for each file or whatever granularity we want for considering messages to be duplicates.)
On Sat, 2011-04-09 at 00:21 +0200, Petr Machata wrote:
The message counting is keyed by the address of the first string sent to the stream and counted. That means that non-unique strings that are merged by GCC are not considered separate messages, and that dynamically generated strings are not filtered at all. This can be mended by stating the key explicitly like so:
wr_message (wh, mc_error).id (msgid) << "blah" //...
I added one to check_dups_abstract_origin.
commit 41aa345222e2981e395fecb9c498885253063409 Author: Mark Wielaard mjw@redhat.com Date: Mon Apr 11 16:34:17 2011 +0200
dwarflint: check_dups_abstract_origin use wr_message id for filtering dups.
There's currently no standard way to obtain msgids. It's simply a void* as of now. I'm using check descriptor in a couple places as the id, with the downside that all messages in one check get one counter.
I think this is fine (at least for now).
The duplication checking doesn't currently work for non-stream based wr_messages that take a format string. The following makes it work for those too (which really helps when you have lots of low level checks firing the same message over and over again, just try dwarflint on dwarflint itself and see all the range warnings).
diff --git a/dwarflint/messages.cc b/dwarflint/messages.cc index 2c0810e..995220c 100644 --- a/dwarflint/messages.cc +++ b/dwarflint/messages.cc @@ -258,7 +258,7 @@ wr_verror (const struct where *wh, const char *format, va_list ap) static void wr_vwarning (const struct where *wh, const char *format, va_list ap) { - printf ("warning: %s", where_fmt (wh, NULL)); + printf ("%s", where_fmt (wh, NULL)); vprintf (format, ap); where_fmt_chain (wh, "warning"); ++error_count; @@ -279,7 +279,11 @@ wr_message (unsigned long category, const struct where *wh, { va_list ap; va_start (ap, format); - if (message_accept (&warning_criteria, category)) + // Clumsy duplicate filtering. Use format as key. + bool whether = false; + message_category cat = (message_category) category; + wr_message (cat).id (format, whether); + if (whether && message_accept (&warning_criteria, category)) { if (message_accept (&error_criteria, category)) wr_verror (wh, format, ap);
This is slightly ugly (because we ask for the id () filtering the "warning: " string is already printed), but seems to work fine. Does this make sense, or is there a cleaner way to do it?
Thanks,
Mark
Mark Wielaard mjw@redhat.com writes:
This is slightly ugly (because we ask for the id () filtering the "warning: " string is already printed), but seems to work fine. Does this make sense, or is there a cleaner way to do it?
This seems OK to me. It uses the same kind of hack that the default case of stream filtering does, so I can't even consider it ugly ;)
Thanks, PM
elfutils-devel@lists.fedorahosted.org
-
Mark Wielaard -
Petr Machata -
Roland McGrath