Bug 11158 - Enhance log messages by prefixing with variable name, etc. and add a few more log calls.
Enhance log messages by prefixing with variable name, etc. and add a few more...
Status: RESOLVED WONTFIX
Product: ccache
Classification: Unclassified
Component: ccache
dev
All All
: P5 normal
: ---
Assigned To: Joel Rosdahl
Joel Rosdahl
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-03-11 16:01 UTC by ishikawa
Modified: 2016-06-10 02:58 UTC (History)
0 users

See Also:


Attachments
Enhance log message by adding variable prefix, etc. (3.83 KB, patch)
2015-03-11 16:01 UTC, ishikawa
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description ishikawa 2015-03-11 16:01:29 UTC
Created attachment 10859 [details]
Enhance log message by adding variable prefix, etc.

CAVEAT: The uploaded patch was created during the work to support
-gsplit-dwarf (FissionDwarf) of GCC.  (See bug 10005 )
Some of the patches have interdependence and if earlier patches are
not applied, the later ones may fail. I will re-create patches
appropriately depending on how an earlier patch is incorporated (or
dropped).

The whole list of broken patches below were uploaded in bug 10005 for
convenience, but now separate bugzilla is filed to follow up on each of it
(except for the main -gsplit-dwarf support, in bug 10005).

 0001-Harden-logging-perror-replaced-with-cc_log-and-fatal.patch
 0002-hash-file-creation-failure-is-recorded-now.patch
 0003-Use-x_unlink-instead-of-unlink-per-HACKING.txt.patch
 0004-make-sure-the-result-of-stat-call-is-checked.patch
*0005-Enhance-log-message-add-prefix-var.-name-and-a-few-a.patch
 0006-warning-about-missing-x_unlinks-when-get_file_from_c.patch
 0007-Support-for-gsplit-dwarf.patch
 0008-Missing-copy-of-diagnostics-file-in-case-of-failure.patch

What this entry is about:

Short explanation.

5: 0005-Enhance-log-message-add-prefix-var.-name-and-a-few-a.patch

   Enhance log message: add prefix (variable name appropriately) and
   a few additions of cc_log() calls..

   These enhanced log messages were crucial in developing and
   debugging the support for -gsplit-dwarf. Also, I could find the now
   fixed extra unnecessary copy between the cached and real object
   files by looking at the enhanced log messages.

   So I think this change is very important for enhancing ccache in
   the future or when subtle bugs may creep in.

   This is actually not related to -gsplit-dwarf itself, and so I am
   filing this separate bugzilla.

   Since this changes ccache.c, it may interfere with the later patch
   application if this is not applied to ccache repository.
   (see bug 10005)

TIA
Comment 1 Joel Rosdahl 2015-03-16 21:04:24 UTC
Not sure about this patch. I'm all for improved logging, but I don't really see why adding variable names to the log messages would make it easier to debug - don't the file names already contain enough information? For instance, cached_stderr has a ".stderr" suffix. (As mentioned in bug 11154, my view is that the reader of the ccache logfile is not expected to know about ccache's internal function/variable names.)

I also don't quite see the point of adding log messages after calls to get_file_from_cache and put_file_in_cache since those functions already log what they have done.

But I'm probably missing some aspect since you apparently feel that you were helped by the changes in the patch. :-) Perhaps you could describe in what way you were helped by the variable name prefixes? In my mind, a developer can easily map the essentially unique error messages to code location already without the mentioning of internal variable names in the log file.
Comment 2 ishikawa 2015-03-17 06:29:37 UTC
(In reply to Joel Rosdahl from comment #1)

Thank you for your comment.

Re:
> I also don't quite see the point of adding log messages after calls to 
> get_file_from_cache and put_file_in_cache since those functions already log 
> what they have done.

One of the log message can go. Yes, there is a duplication.

That said,
my point is the necessity for a new developer who needs hack on the existing ccache code base as opposed to the ordinary user of ccache as you correctly pointed out: that the reader of the ccache logfile is not expected to know about ccache's internal function/variable names.".
When a new developer comes along and tries to figure out what ccache is doing under its hood, prefixing the file name with the internal variable name
was a huge plus (well, to be honest, the suffix doesn't mean much, or rather
due to the longish filename after hash string part, following the suffix is not easy. I found the prefixing the output with the variable name was a huge plush: I mean that the developer does not need to grok the detailed hash string to understand the logical operation and when something goes wrong, only then
can take a look at the hashed file name very carefully. The variable name prefix was really helpful (WITHOUT the need to scan past the lengthy file name to check for the suffix.)

Maybe we can change the output based on a flag: a bit of an ugly solution, but
to new unaided eyes the variable prefix was a great help.

TIA
Comment 3 ishikawa 2015-04-28 20:09:27 UTC
(In reply to Joel Rosdahl from comment #1)

Dear Joel,

I think hard about this patch, and decided that this log message patch can wait (wait forever if necessary).

Instead, I would like to have the main patch to support "-gsplit-dwarf".
So I refreshed the patch there.

Please take a look there.

I will wait for the merge of main patch iin 10005, which should be easier now that "profiling" support is in the repository 
since the handling of cov file is similar in nature to the handling of dwo file.
So the patch for -gsplit-dwarf support now seems more familiar than before.

I can resurrect this patch to see if I can improve the
log messages for easier debugging when someone attempts major surgery like
the addition of FissionDwarf or profiling support.

TIA
Comment 4 Joel Rosdahl 2016-06-09 19:41:30 UTC
Closing this now since no progress has been made for a long time. Please create a new issue at https://github.com/ccache/ccache/issues if wanted.
Comment 5 ishikawa 2016-06-10 02:58:34 UTC
Thank you again for sharing ccache with developers community.
I will come back when I need to have a detailed log facility when I need to hack the internal of ccache again. (I just found out a few days ago that, despite my successful use of ccache locally on my PC, mozilla's infrastructure is now using a similar but different system called sccache, which still does not understand -gsplit-dwarf. sccache seems to be tailored to Amazon S3, i.e., cloud usage. Oh well. My motivation to use ccache and support -gsplit-dwarf was to compile mozilla thunderbird fast on my PC for debugging.)

ccache running locally on my Debian GNU/linux is indispensable.

Happy Hacking (in the good sense of the word).

Thank you again.