Implemented in a straightforward way. Compile-tested.
Please review.
On Mon, 2010-09-13 at 15:18 +0200, Jiri Moskovcak wrote:
On 09/13/2010 02:56 PM, Denys Vlasenko wrote:
Implemented in a straightforward way. Compile-tested.
Please review.
+# GdbTimeoutSec = 60
The patch is ok, but I would make the default value bigger, because 60sec is nt enough for apps like openoffice.
I think we don't have evidence yet that 60 seconds are too long: in https://bugzilla.redhat.com/show_bug.cgi?id=632955 "Timeout exceeded: 60 second, killing gdb", this may be actually a case where timeout worked as designed: gdb hung, and was killed. With bigger timeout it would just hang longer...
I think so because it is suspicious that backtrace is *half*-generated. If gdb had no time to load all the needed files, I'd expect an empty backtrace, not a backtrace which stops in the middle of the stack.
But ok, I am setting the default timeout to 120 seconds.
Pushed to git.
On 09/13/2010 02:56 PM, Denys Vlasenko wrote:
diff -x '*.po' -d -urpN abrt.0/lib/plugins/CCpp.conf abrt.1/lib/plugins/CCpp.conf --- abrt.0/lib/plugins/CCpp.conf 2010-08-31 14:49:06.000000000 +0200 +++ abrt.1/lib/plugins/CCpp.conf 2010-09-13 14:53:53.659617932 +0200 @@ -11,6 +11,8 @@ SaveBinaryImage = no
# Generate backtrace Backtrace = yes +# How long to wait for gdb to finish. Default is 60 seconds. +# GdbTimeoutSec = 60
# Generate backtrace for crashes uploaded from remote machines. # Note that for reliable backtrace generation, your local machine diff -x '*.po' -d -urpN abrt.0/lib/plugins/CCpp.cpp abrt.1/lib/plugins/CCpp.cpp --- abrt.0/lib/plugins/CCpp.cpp 2010-09-07 18:57:31.000000000 +0200 +++ abrt.1/lib/plugins/CCpp.cpp 2010-09-13 13:51:15.750764456 +0200 @@ -49,7 +49,8 @@ CAnalyzerCCpp::CAnalyzerCCpp() : m_bBacktraceRemotes(false), m_bMemoryMap(false), m_bInstallDebugInfo(true),
- m_nDebugInfoCacheMB(4000)
m_nDebugInfoCacheMB(4000),
m_nGdbTimeoutSec(60) {}
static string create_hash(const char *pInput)
@@ -113,7 +114,7 @@ static string create_hash(const char *pI
- @param[out] status See `man 2 wait` for status information.
- @return Malloc'ed string
*/ -static char* exec_vp(char **args, uid_t uid, int redirect_stderr, int *status) +static char* exec_vp(char **args, uid_t uid, int redirect_stderr, int timeout_sec, int *status)
I guess timeout_sec can be const unsigned int here.
{ /* Nuke everything which may make setlocale() switch to non-POSIX locale: * we need to avoid having gdb output in some obscure language. @@ -143,7 +144,7 @@ static char* exec_vp(char **args, uid_t * Therefore we have a (largish) timeout, after which we kill the child. */ int t = time(NULL); /* int is enough, no need to use time_t */
- int endtime = t + 60;
int endtime = t + timeout_sec;
struct strbuf *buf_out = strbuf_new();
@@ -181,7 +182,7 @@ static char* exec_vp(char **args, uid_t return strbuf_free_nobuf(buf_out); }
-static char *get_backtrace(const char *pDebugDumpDir, const char *pDebugInfoDirs) +static char *get_backtrace(const char *pDebugDumpDir, const char *pDebugInfoDirs, int timeout_sec)
same here, const unsigned int
{ update_client(_("Generating backtrace"));
@@ -201,7 +202,7 @@ static char *get_backtrace(const char *p //http://sourceware.org/bugzilla/show_bug.cgi?id=9622 unsetenv("TERM"); // This is not necessary, and was observed to cause
- // environmant corruption (because we run in a thread?):
// environment corruption (because we run in a thread?): //putenv((char*)"TERM=dumb");
char *args[21];
@@ -276,7 +277,7 @@ static char *get_backtrace(const char *p while (1) { args[9] = xasprintf("%s backtrace %u%s", thread_apply_all, bt_depth, full);
bt = exec_vp(args, xatoi_u(uid), /*redirect_stderr:*/ 1, NULL);
bt = exec_vp(args, xatoi_u(uid), /*redirect_stderr:*/ 1, timeout_sec, NULL); if (bt&& (bt_depth<= 64 || strlen(bt)< 256*1024)) { free(args[9]);
@@ -332,7 +333,7 @@ static void GetIndependentBuildIdPC(cons } }
-static char* run_unstrip_n(const char *pDebugDumpDir) +static char* run_unstrip_n(const char *pDebugDumpDir, int timeout_sec)
again, const unsigned int?
{ dump_dir_t *dd = dd_init(); if (!dd_opendir(dd, pDebugDumpDir)) @@ -351,7 +352,7 @@ static char* run_unstrip_n(const char *p args[2] = (char*)"-n"; args[3] = NULL;
- char *out = exec_vp(args, xatoi_u(uid), /*redirect_stderr:*/ 0, NULL);
char *out = exec_vp(args, xatoi_u(uid), /*redirect_stderr:*/ 0, timeout_sec, NULL); free(uid);
free(args[1]);
@@ -541,7 +542,7 @@ string CAnalyzerCCpp::GetLocalUUID(const dd_close(dd);
string independentBuildIdPC;
- char *unstrip_n_output = run_unstrip_n(pDebugDumpDir);
- char *unstrip_n_output = run_unstrip_n(pDebugDumpDir, m_nGdbTimeoutSec); if (unstrip_n_output) GetIndependentBuildIdPC(unstrip_n_output, independentBuildIdPC); else
@@ -811,7 +812,7 @@ void CAnalyzerCCpp::CreateReport(const c VERB1 log(_("Skipping the debuginfo installation"));
/* Create and store backtrace. */
- char *backtrace_str = get_backtrace(pDebugDumpDir, m_sDebugInfoDirs.c_str());
- char *backtrace_str = get_backtrace(pDebugDumpDir, m_sDebugInfoDirs.c_str(), m_nGdbTimeoutSec); if (!backtrace_str) { backtrace_str = xstrdup("");
@@ -1109,6 +1110,11 @@ void CAnalyzerCCpp::SetSettings(const ma { m_nDebugInfoCacheMB = xatou(it->second.c_str()); }
- it = pSettings.find("GdbTimeoutSec");
- if (it != end)
- {
m_nGdbTimeoutSec = xatoi_u(it->second.c_str());
- } it = pSettings.find("InstallDebugInfo"); if (it == end) //compat, remove after 0.0.11 it = pSettings.find("InstallDebuginfo");
diff -x '*.po' -d -urpN abrt.0/lib/plugins/CCpp.h abrt.1/lib/plugins/CCpp.h --- abrt.0/lib/plugins/CCpp.h 2010-08-31 14:49:06.000000000 +0200 +++ abrt.1/lib/plugins/CCpp.h 2010-09-13 13:46:33.410765929 +0200 @@ -34,6 +34,7 @@ class CAnalyzerCCpp : public CAnalyzer bool m_bMemoryMap; bool m_bInstallDebugInfo; unsigned m_nDebugInfoCacheMB;
unsigned m_nGdbTimeoutSec;
Here it is unsigned.
std::string m_sOldCorePattern; std::string m_sDebugInfo; std::string m_sDebugInfoDirs;
Just went through the patch - reading it, not tried to apply.
RR
On Mon, 2010-09-13 at 15:21 +0200, Roman Rakus wrote:
On 09/13/2010 02:56 PM, Denys Vlasenko wrote:
diff -x '*.po' -d -urpN abrt.0/lib/plugins/CCpp.conf abrt.1/lib/plugins/CCpp.conf --- abrt.0/lib/plugins/CCpp.conf 2010-08-31 14:49:06.000000000 +0200 +++ abrt.1/lib/plugins/CCpp.conf 2010-09-13 14:53:53.659617932 +0200 @@ -11,6 +11,8 @@ SaveBinaryImage = no
# Generate backtrace Backtrace = yes +# How long to wait for gdb to finish. Default is 60 seconds. +# GdbTimeoutSec = 60
# Generate backtrace for crashes uploaded from remote machines. # Note that for reliable backtrace generation, your local machine diff -x '*.po' -d -urpN abrt.0/lib/plugins/CCpp.cpp abrt.1/lib/plugins/CCpp.cpp --- abrt.0/lib/plugins/CCpp.cpp 2010-09-07 18:57:31.000000000 +0200 +++ abrt.1/lib/plugins/CCpp.cpp 2010-09-13 13:51:15.750764456 +0200 @@ -49,7 +49,8 @@ CAnalyzerCCpp::CAnalyzerCCpp() : m_bBacktraceRemotes(false), m_bMemoryMap(false), m_bInstallDebugInfo(true),
- m_nDebugInfoCacheMB(4000)
m_nDebugInfoCacheMB(4000),
m_nGdbTimeoutSec(60) {}
static string create_hash(const char *pInput)
@@ -113,7 +114,7 @@ static string create_hash(const char *pI
- @param[out] status See `man 2 wait` for status information.
- @return Malloc'ed string
*/ -static char* exec_vp(char **args, uid_t uid, int redirect_stderr, int *status) +static char* exec_vp(char **args, uid_t uid, int redirect_stderr, int timeout_sec, int *status)
I guess timeout_sec can be const unsigned int here.
I understand why unsigned, but why const?
On 09/13/2010 06:44 PM, Denys Vlasenko wrote:
On Mon, 2010-09-13 at 15:21 +0200, Roman Rakus wrote:
On 09/13/2010 02:56 PM, Denys Vlasenko wrote:
diff -x '*.po' -d -urpN abrt.0/lib/plugins/CCpp.conf abrt.1/lib/plugins/CCpp.conf --- abrt.0/lib/plugins/CCpp.conf 2010-08-31 14:49:06.000000000 +0200 +++ abrt.1/lib/plugins/CCpp.conf 2010-09-13 14:53:53.659617932 +0200 @@ -11,6 +11,8 @@ SaveBinaryImage = no
# Generate backtrace Backtrace = yes +# How long to wait for gdb to finish. Default is 60 seconds. +# GdbTimeoutSec = 60
# Generate backtrace for crashes uploaded from remote machines. # Note that for reliable backtrace generation, your local machine diff -x '*.po' -d -urpN abrt.0/lib/plugins/CCpp.cpp abrt.1/lib/plugins/CCpp.cpp --- abrt.0/lib/plugins/CCpp.cpp 2010-09-07 18:57:31.000000000 +0200 +++ abrt.1/lib/plugins/CCpp.cpp 2010-09-13 13:51:15.750764456 +0200 @@ -49,7 +49,8 @@ CAnalyzerCCpp::CAnalyzerCCpp() : m_bBacktraceRemotes(false), m_bMemoryMap(false), m_bInstallDebugInfo(true),
- m_nDebugInfoCacheMB(4000)
m_nDebugInfoCacheMB(4000),
m_nGdbTimeoutSec(60) {}
static string create_hash(const char *pInput)
@@ -113,7 +114,7 @@ static string create_hash(const char *pI * @param[out] status See `man 2 wait` for status information. * @return Malloc'ed string */ -static char* exec_vp(char **args, uid_t uid, int redirect_stderr, int *status) +static char* exec_vp(char **args, uid_t uid, int redirect_stderr, int timeout_sec, int *status)
I guess timeout_sec can be const unsigned int here.
I understand why unsigned, but why const?
For the same reason you have for example `const char *'. But yes, it's not common. Nevertheless, unsigned seems reasonable for me.
On Tue, 2010-09-14 at 11:57 +0200, Roman Rakus wrote:
On 09/13/2010 06:44 PM, Denys Vlasenko wrote:
On Mon, 2010-09-13 at 15:21 +0200, Roman Rakus wrote:
On 09/13/2010 02:56 PM, Denys Vlasenko wrote:
diff -x '*.po' -d -urpN abrt.0/lib/plugins/CCpp.conf abrt.1/lib/plugins/CCpp.conf --- abrt.0/lib/plugins/CCpp.conf 2010-08-31 14:49:06.000000000 +0200 +++ abrt.1/lib/plugins/CCpp.conf 2010-09-13 14:53:53.659617932 +0200 @@ -11,6 +11,8 @@ SaveBinaryImage = no
# Generate backtrace Backtrace = yes +# How long to wait for gdb to finish. Default is 60 seconds. +# GdbTimeoutSec = 60
# Generate backtrace for crashes uploaded from remote machines. # Note that for reliable backtrace generation, your local machine diff -x '*.po' -d -urpN abrt.0/lib/plugins/CCpp.cpp abrt.1/lib/plugins/CCpp.cpp --- abrt.0/lib/plugins/CCpp.cpp 2010-09-07 18:57:31.000000000 +0200 +++ abrt.1/lib/plugins/CCpp.cpp 2010-09-13 13:51:15.750764456 +0200 @@ -49,7 +49,8 @@ CAnalyzerCCpp::CAnalyzerCCpp() : m_bBacktraceRemotes(false), m_bMemoryMap(false), m_bInstallDebugInfo(true),
- m_nDebugInfoCacheMB(4000)
m_nDebugInfoCacheMB(4000),
m_nGdbTimeoutSec(60) {}
static string create_hash(const char *pInput)
@@ -113,7 +114,7 @@ static string create_hash(const char *pI
- @param[out] status See `man 2 wait` for status information.
- @return Malloc'ed string
*/ -static char* exec_vp(char **args, uid_t uid, int redirect_stderr, int *status) +static char* exec_vp(char **args, uid_t uid, int redirect_stderr, int timeout_sec, int *status)
I guess timeout_sec can be const unsigned int here.
I understand why unsigned, but why const?
For the same reason you have for example `const char *'. But yes, it's not common.
Const-ness is useful in *pointers*: when I pass a parameter to f(char*), compiler checks that I do pass a non-const ptr to it, and complains otherwise, because (presumably) f can try writing to it and cause a SEGV or other damage. IOW: f1(char*) and f2(const char*) behave differently in some cases.
Const-ness of the parameters themselves is useless: both f1(int n) and f2(const int n) accept any integers, not only constants. There is no difference. IOW: gcc will not complain if you call f2(a + b).
Similarly, f1(type *p) and f2(type *const p) both can be called with any kind of pointer to type, not necessarily a constant pointer.
On 09/14/2010 01:07 PM, Denys Vlasenko wrote:
Const-ness is useful in*pointers*: when I pass a parameter to f(char*), compiler checks that I do pass a non-const ptr to it, and complains otherwise, because (presumably) f can try writing to it and cause a SEGV or other damage. IOW: f1(char*) and f2(const char*) behave differently in some cases.
Const-ness of the parameters themselves is useless: both f1(int n) and f2(const int n) accept any integers, not only constants. There is no difference. IOW: gcc will not complain if you call f2(a + b).
Similarly, f1(type *p) and f2(type *const p) both can be called with any kind of pointer to type, not necessarily a constant pointer.
You're right. Sorry for mess.
RR
crash-catcher@lists.fedorahosted.org