From: Alex Rousskov Date: Wed, 10 Jan 2018 15:45:43 +0000 (-0700) Subject: Report exception locations and exception-related polish (#119) X-Git-Tag: M-staged-PR71~16 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ebaabe74128fe480b35ece2d46816f2c6fd7f888;p=thirdparty%2Fsquid.git Report exception locations and exception-related polish (#119) Without location, many exceptions look identical: A growing number of Must(entry != NULL) and Must(request) complicate triage. The location info was already stored in TextException but was not reported. Reporting exception location on a separate line makes admin-visible FATAL/ERROR/WARNING messages easier to comprehend, and their primary text becomes more "stable", which is good for documentation. Also, some future exceptions will probably report multiple details, possibly even context details collected as a low-level exception bubbles up to its high-level handling/reporting code. Also simplified/optimized TextException: * TextException now reuses std::runtime_error message memory management code, including its CoW optimizations/guarantees. * Debug and TextException code now share the source location reporting code (including Squid build prefix elision) in base/Here.{cc,h}. Also simplified and polished SBuf-related exceptions, removing a few: * Removed InvalidParamException as unused. * Replaced SBufTooBigException with generic exceptions. SBufTooBigException was misused (by SBuf::plength) and not useful. No need to create a whole class just to parameterize an object! * Replaced OutOfBoundsException with a generic exception. OutOfBoundsException was not very useful (see SBufTooBigException). It was used by one test case, that did not justify adding a whole class. Also added SWALLOW_EXCEPTIONS() API to protect any code that may throw unwanted exceptions. Reworked a few destructors after Must() changes made it easier for GCC v6 to detect (and warn about) throwing code: * Polished Ipc::Forwarder cleanup sequence. For Forwarders, I see no reason to split/duplicate swanSong() functionality via a cleanup() method. The swanSong() API exists so that job destructors do not need to make confusing virtual method calls! * Hid the AsyncJob destructor because all jobs should be "automatically" deleted by the internal job code that guarantees a swanSong() call. * Removed a bad (pair-less) StoreEntry::unregisterAbort() call from Mgr::Forwarder destructor, possibly left behind in or around 51ea090. * Removed ctor/dtor entrance debugging from the classes affected by the "throwing destructor issue". AsyncJob covers that debugging need. * TODO: Adjust all throwing destructors. --- diff --git a/scripts/calc-must-ids.pl b/scripts/calc-must-ids.pl index 0442618521..4cef8dff31 100755 --- a/scripts/calc-must-ids.pl +++ b/scripts/calc-must-ids.pl @@ -30,9 +30,10 @@ while ($file = shift @ARGV) { } sub FileNameHash { -# Please keep in sync this function with the FileNameHash function in -# src/base/TextException.cc file my($name) = @_; + + # Keep in sync with FileNameHash() in src/base/Here.cc! + $name =~ s/.*\///g; my($i) = 0; my($j) =0; @@ -41,26 +42,33 @@ sub FileNameHash for($j=0; $j < @na; $j++) { $n = $n ^ (271 * ord($na[$j])); } - $i = $n ^ ($j *271); - - # Currently 18bits of a 32 bit integer used for filename hash - # (max hash=262143), and 14 bits for storing line number - $i = $i % 262143; - return $i; + return $n ^ ($j *271); } sub ComputeMustIds { my($file) = @_; - my($hash) = FileNameHash($file); + + # Keep in sync with SourceLocation::id() in src/base/Here.cc! + + my $fullHash = &FileNameHash($file); + my $hash = $fullHash % 0x3FFFF; + if(!open(IN, "<$file")) { printf STDERR "error opening file $file. Ignore ..."; return; } while() { my($line) = $_; + + next if $line =~ /^\s*#/; # ignore simple single-line C++ macros + $line =~ s@//.*@@; # strip simple // comments + $line =~ s@/[*].*?[*]/@@; # strip simple single-line /* comments */ + my($id); - if ( $line =~ /^\s*Must\s*\(/ || $line =~ /^\s*throw\s*TexcHere\s*\(/){ + if ($line =~ /\bMust2?\s*\(/ || # Must(...) and Must2(...) + $line =~ /\bTexcHere\s*\(/ || # TexcHere(...) + $line =~ /\bHere\s*\(\s*\)/) { # Here() $line =~ s/^\s*//; $id= ($hash <<14) | ($. & 0x3FFF); $id += ERR_DETAIL_EXCEPTION_START; diff --git a/src/Debug.h b/src/Debug.h index c8571e504a..e1186f3f55 100644 --- a/src/Debug.h +++ b/src/Debug.h @@ -11,6 +11,7 @@ #ifndef SQUID_DEBUG_H #define SQUID_DEBUG_H +#include "base/Here.h" // XXX should be mem/forward.h once it removes dependencies on typedefs.h #include "mem/AllocatorProxy.h" @@ -111,9 +112,6 @@ void StopUsingDebugLog(); /// a hack for low-level file descriptor manipulations in ipcCreate() void ResyncDebugLog(FILE *newDestination); -size_t BuildPrefixInit(); -const char * SkipBuildPrefix(const char* path); - /* Debug stream * * Unit tests can enable full debugging to stderr for one @@ -127,7 +125,7 @@ const char * SkipBuildPrefix(const char* path); std::ostream &_dbo = Debug::Start((SECTION), _dbg_level); \ if (_dbg_level > DBG_IMPORTANT) { \ _dbo << (SECTION) << ',' << _dbg_level << "| " \ - << SkipBuildPrefix(__FILE__)<<"("<<__LINE__<<") "<<__FUNCTION__<<": "; \ + << Here() << ": "; \ } \ _dbo << CONTENT; \ Debug::Finish(); \ diff --git a/src/DiskIO/IpcIo/IpcIoFile.cc b/src/DiskIO/IpcIo/IpcIoFile.cc index 00dd1beae1..a6a21d4b3e 100644 --- a/src/DiskIO/IpcIo/IpcIoFile.cc +++ b/src/DiskIO/IpcIo/IpcIoFile.cc @@ -78,13 +78,14 @@ IpcIoFile::IpcIoFile(char const *aDb): IpcIoFile::~IpcIoFile() { - if (diskId >= 0) { - const IpcIoFilesMap::iterator i = IpcIoFiles.find(diskId); - // XXX: warn and continue? - Must(i != IpcIoFiles.end()); - Must(i->second == this); - IpcIoFiles.erase(i); - } + SWALLOW_EXCEPTIONS({ + if (diskId >= 0) { + const auto i = IpcIoFiles.find(diskId); + Must(i != IpcIoFiles.end()); + Must(i->second == this); + IpcIoFiles.erase(i); + } + }); } void diff --git a/src/FadingCounter.cc b/src/FadingCounter.cc index f629c6d423..31f588686f 100644 --- a/src/FadingCounter.cc +++ b/src/FadingCounter.cc @@ -8,6 +8,7 @@ #include "squid.h" #include "base/TextException.h" +#include "Debug.h" #include "FadingCounter.h" #include "SquidTime.h" diff --git a/src/SquidString.h b/src/SquidString.h index 305f07108f..f18ab8c030 100644 --- a/src/SquidString.h +++ b/src/SquidString.h @@ -12,6 +12,7 @@ #define SQUID_STRING_H #include "base/TextException.h" +#include "Debug.h" #include diff --git a/src/adaptation/icap/ServiceRep.cc b/src/adaptation/icap/ServiceRep.cc index 89677a84d4..9261569397 100644 --- a/src/adaptation/icap/ServiceRep.cc +++ b/src/adaptation/icap/ServiceRep.cc @@ -49,9 +49,11 @@ Adaptation::Icap::ServiceRep::ServiceRep(const ServiceConfigPointer &svcCfg): Adaptation::Icap::ServiceRep::~ServiceRep() { - delete theIdleConns; - Must(!theOptionsFetcher); - delete theOptions; + SWALLOW_EXCEPTIONS({ + delete theIdleConns; + Must(!theOptionsFetcher); + delete theOptions; + }); } void diff --git a/src/base/AsyncJob.h b/src/base/AsyncJob.h index a07bdc0f29..3c0be8c60b 100644 --- a/src/base/AsyncJob.h +++ b/src/base/AsyncJob.h @@ -35,7 +35,6 @@ public: public: AsyncJob(const char *aTypeName); - virtual ~AsyncJob(); /// starts a freshly created job (i.e., makes the job asynchronous) static Pointer Start(AsyncJob *job); @@ -64,6 +63,9 @@ public: virtual void callException(const std::exception &e); protected: + // external destruction prohibited to ensure swanSong() is called + virtual ~AsyncJob(); + const char *stopReason; ///< reason for forcing done() to be true const char *typeName; ///< kid (leaf) class name, for debugging AsyncCall::Pointer inCall; ///< the asynchronous call being handled, if any diff --git a/src/base/Here.cc b/src/base/Here.cc new file mode 100644 index 0000000000..0a7efe9b8c --- /dev/null +++ b/src/base/Here.cc @@ -0,0 +1,93 @@ +/* + * Copyright (C) 1996-2018 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#include "squid.h" +#include "base/Here.h" + +#include + +/* File name hashing helpers */ + +// Build prefix is the file system path leading to Squid src/ source directory. +// It is "." for in-tree builds but may be lengthy and sensitive otherwise. + +/// \returns the build prefix length or, if estimation is not possible, zero +static size_t +BuildPrefixLength() +{ + // The hard-coded tail must be kept in sync with this file actual name! + const char *tail = "src/base/Here.cc"; + const char *full = __FILE__; + + // Disable heuristic if it does not work. + if (strstr(full, tail) == 0) + return 0; + + return strlen(full) - strlen(tail); +} + +/// \returns filename portion without the build prefix +static const char * +SkipBuildPrefix(const char* path) +{ + static const size_t ToSkip = BuildPrefixLength(); + return path + ToSkip; +} + +/// quickly computes a (weak) hash of a file name +static SourceLocationId +FileNameHash(const char *path) +{ + // Keep in sync with FileNameHash() in scripts/calc-must-ids.pl! + + const char *name = strrchr(path, '/'); + if (name) + ++name; // skip '/' + else + name = path; + + uint32_t hash = 0; + uint32_t iterations = 0; + while (*name) { + ++iterations; + hash ^= 271 * static_cast(*name); + ++name; + } + return hash ^ (iterations * 271); +} + +/* SourceLocation */ + +SourceLocationId +SourceLocation::id() const +{ + const auto fnameHashFull = fileNameHashCacher(fileName, &FileNameHash); + // 32 bits = 18 bits for the filename hash + 14 bits for the line number. + // Keep in sync with ComputeMustIds() in scripts/calc-must-ids.pl. + const auto fnameHash = fnameHashFull % 0x3FFFF; + return (fnameHash << 14) | (lineNo & 0x3FFF); +} + +std::ostream & +SourceLocation::print(std::ostream &os) const +{ + if (fileName) { + os << SkipBuildPrefix(fileName); + + // TODO: Use more common and search-friendlier fileName:lineNo: format. + if (lineNo > 0) + os << '(' << lineNo << ')'; + } + if (context) { + if (fileName) + os << ' '; + os << context; + } + return os; +} + diff --git a/src/base/Here.h b/src/base/Here.h new file mode 100644 index 0000000000..7ece4e253d --- /dev/null +++ b/src/base/Here.h @@ -0,0 +1,77 @@ +/* + * Copyright (C) 1996-2018 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#ifndef SQUID_BASE_HERE_H +#define SQUID_BASE_HERE_H + +#include + +/// source code location of the caller +#define Here() SourceLocation(__FUNCTION__, __FILE__, __LINE__) + +/// semi-uniquely identifies a source code location; stable across Squid runs +typedef uint32_t SourceLocationId; + +/// returns a hash of a file name +typedef SourceLocationId FileNameHasher(const char *fileName); + +/// a caching proxy for `hasher` results +typedef SourceLocationId FileNameHashCacher(const char *fileName, FileNameHasher hasher); + +static FileNameHashCacher UnitFileNameHashCacher; + +/// a source code location that is cheap to create, copy, and store +class SourceLocation +{ +public: + SourceLocation(const char *aContext, const char *aFileName, const int aLineNo): + context(aContext), + fileName(aFileName), + lineNo(aLineNo), + fileNameHashCacher(&UnitFileNameHashCacher) + {} + + /// \returns our location identifier + SourceLocationId id() const; + + /// describes location using a compact but human-friendly format + std::ostream &print(std::ostream &os) const; + + const char *context; ///< line-independent location description + const char *fileName; ///< source file name, often relative to build path + int lineNo; ///< line number inside the source file name (if positive) + +private: + SourceLocationId calculateId(FileNameHasher) const; + FileNameHashCacher *fileNameHashCacher; +}; + +inline std::ostream & +operator <<(std::ostream &os, const SourceLocation &location) +{ + return location.print(os); +} + +/// SourceLocation::id() speed optimization hack: Caches `hasher` results. The +/// cache capacity is one filename hash. Each translation unit gets one cache. +static SourceLocationId +UnitFileNameHashCacher(const char *fileName, FileNameHasher hasher) +{ + static SourceLocationId cachedHash = 0; + static const char *hashedFilename = 0; + // Each file #included in a translation unit has its own __FILE__ value. + // Keep the cache fresh (and the result correct). + if (hashedFilename != fileName) { // cheap pointer comparison + hashedFilename = fileName; + cachedHash = hasher(fileName); + } + return cachedHash; +} + +#endif + diff --git a/src/base/Makefile.am b/src/base/Makefile.am index 303dc1dd34..a6ee09775d 100644 --- a/src/base/Makefile.am +++ b/src/base/Makefile.am @@ -13,21 +13,23 @@ noinst_LTLIBRARIES = libbase.la libbase_la_SOURCES = \ AsyncCall.cc \ AsyncCall.h \ + AsyncCallQueue.cc \ + AsyncCallQueue.h \ AsyncCbdataCalls.h \ - AsyncJob.h \ AsyncJob.cc \ + AsyncJob.h \ AsyncJobCalls.h \ - AsyncCallQueue.cc \ - AsyncCallQueue.h \ ByteCounter.h \ CbcPointer.h \ CbDataList.h \ - CharacterSet.h \ CharacterSet.cc \ + CharacterSet.h \ EnumIterator.h \ - File.h \ File.cc \ + File.h \ HardFun.h \ + Here.cc \ + Here.h \ InstanceId.h \ Lock.h \ LookupTable.h \ diff --git a/src/base/RunnersRegistry.cc b/src/base/RunnersRegistry.cc index ceb72c79b6..af6d072970 100644 --- a/src/base/RunnersRegistry.cc +++ b/src/base/RunnersRegistry.cc @@ -9,6 +9,7 @@ #include "squid.h" #include "base/RunnersRegistry.h" #include "base/TextException.h" +#include "Debug.h" #include /// a collection of unique runners, in no particular order diff --git a/src/base/TextException.cc b/src/base/TextException.cc index 96f4a01057..f197e237ad 100644 --- a/src/base/TextException.cc +++ b/src/base/TextException.cc @@ -8,93 +8,72 @@ #include "squid.h" #include "base/TextException.h" -#include "Debug.h" #include "sbuf/SBuf.h" -#include "util.h" -TextException::TextException() -{ - message=NULL; - theFileName=NULL; - theLineNo=0; - theId=0; -} +#include +#include +#include -TextException::TextException(const TextException& right) : - message((right.message?xstrdup(right.message):NULL)), theFileName(right.theFileName), theLineNo(right.theLineNo), theId(right.theId) -{ -} +/// a standard CoW string; avoids noise and circular dependencies of SBuf +typedef std::runtime_error WhatString; -TextException::TextException(const char *aMsg, const char *aFileName, int aLineNo, unsigned int anId): - message(aMsg?xstrdup(aMsg):NULL), theFileName(aFileName), theLineNo(aLineNo), theId(anId) -{} +/// a collection of strings indexed by pointers to their creator objects +typedef std::unordered_multimap WhatStrings; + +/// requested what() strings of alive TextException objects +static WhatStrings *WhatStrings_ = nullptr; -TextException::TextException(SBuf msg, const char *aFileName, int aLineNo, unsigned int anId): - TextException(msg.c_str(), aFileName, aLineNo, anId) +TextException::TextException(SBuf message, const SourceLocation &location): + TextException(message.c_str(), location) {} TextException::~TextException() throw() { - if (message) xfree(message); + if (WhatStrings_) + WhatStrings_->erase(this); // there only if what() has been called } -TextException& TextException::operator=(const TextException &right) +std::ostream & +TextException::print(std::ostream &os) const { - if (this==&right) return *this; - if (message) xfree(message); - message=(right.message?xstrdup(right.message):NULL); - theFileName=right.theFileName; - theLineNo=right.theLineNo; - theId=right.theId; - return *this; + os << std::runtime_error::what() << "\n" << + " exception location: " << where << "\n"; + // TODO: error_detail: " << (ERR_DETAIL_EXCEPTION_START+id()) << "\n"; + return os; } -const char *TextException::what() const throw() +const char * +TextException::what() const throw() { - /// \todo add file:lineno - return message ? message : "TextException without a message"; -} + std::ostringstream os; + print(os); + const WhatString result(os.str()); -unsigned int TextException::FileNameHash(const char *fname) -{ - const char *s = NULL; - unsigned int n = 0; - unsigned int j = 0; - unsigned int i = 0; - s = strrchr(fname, '/'); - - if (s) - ++s; - else - s = fname; + // extend result.c_str() lifetime to this object lifetime + if (!WhatStrings_) + WhatStrings_ = new WhatStrings; + // *this could change, but we must preserve old results for they may be used + WhatStrings_->emplace(std::make_pair(this, result)); - while (*s) { - ++j; - n ^= 271 * (unsigned) *s; - ++s; - } - i = n ^ (j * 271); - /*18bits of a 32 bit integer used for filename hash (max hash=262143), - and 14 bits for storing line number (16k max). - If you change this policy remember to update the FileNameHash function - in the scripts/calc-must-ids.pl script - */ - return i % 262143; + return result.what(); } -void Throw(const char *message, const char *fileName, int lineNo, unsigned int id) +std::ostream & +CurrentException(std::ostream &os) { - - // or should we let the exception recepient print the exception instead? - - if (fileName) { - debugs(0, 3, fileName << ':' << lineNo << ": exception" << - (message ? ": " : ".") << (message ? message : "")); + if (std::current_exception()) { + try { + throw; // re-throw to recognize the exception type + } + catch (const std::exception &ex) { + os << ex.what(); + } + catch (...) { + os << "[unknown exception type]"; + } } else { - debugs(0, 3, "exception" << - (message ? ": " : ".") << (message ? message : "")); + os << "[no active exception]"; } - - throw TextException(message, fileName, lineNo, id); + return os; } diff --git a/src/base/TextException.h b/src/base/TextException.h index b72d829f71..924e093d0b 100644 --- a/src/base/TextException.h +++ b/src/base/TextException.h @@ -9,88 +9,75 @@ #ifndef SQUID__TEXTEXCEPTION_H #define SQUID__TEXTEXCEPTION_H -// Origin: xstd/TextException +#include "base/Here.h" #include class SBuf; -static unsigned int FileNameHashCached(const char *fname); - -// simple exception to report custom errors -// we may want to change the interface to be able to report system errors - -class TextException: public std::exception +/// an std::runtime_error with thrower location info +class TextException: public std::runtime_error { public: - TextException(); - TextException(const char *aMessage, const char *aFileName = 0, int aLineNo = -1, unsigned int anId =0); - TextException(SBuf aMessage, const char *aFileName = 0, int aLineNo = -1, unsigned int anId =0); - TextException(const TextException& right); - virtual ~TextException() throw(); + TextException(const char *message, const SourceLocation &location): + std::runtime_error(message), + where(location) + {} - // unique exception ID for transaction error detail logging - unsigned int id() const { return theId; } + TextException(SBuf message, const SourceLocation &location); - virtual const char *what() const throw(); + TextException(const TextException &) = default; + TextException(TextException &&) = default; + TextException& operator=(const TextException &) = default; - TextException& operator=(const TextException &right); + /* std::runtime_error API */ + virtual ~TextException() throw() override; + virtual const char *what() const throw() override; -public: - char *message; // read-only + /// same-location exceptions have the same ID + SourceLocationId id() const { return where.id(); } -protected: - /// a small integer hash value to semi-uniquely identify the source file - static unsigned int FileNameHash(const char *fname); + /// dumps the exception text into the stream + std::ostream &print(std::ostream &) const; - // optional location information - const char *theFileName; - int theLineNo; - unsigned int theId; + /// code location related to the exception; usually the thrower location + SourceLocation where; - friend unsigned int FileNameHashCached(const char *fname); + // TODO: Add support for arbitrary (re)thrower-supplied details: + // std::tuple details; }; -//inline -//ostream &operator <<(ostream &os, const TextException &exx) { -// return exx.print(os); -//} - -/// caches the result of FileNameHash() for each translation unit -static unsigned int -FileNameHashCached(const char *fname) -{ - static const char *lastFname = 0; - static int lastHash = 0; - // __FILE__ changes when we #include files - if (lastFname != fname) { // cheap pointer comparison - lastFname = fname; - lastHash = TextException::FileNameHash(fname); +/// prints active (i.e., thrown but not yet handled) exception +std::ostream &CurrentException(std::ostream &); + +/// legacy convenience macro; it is not difficult to type Here() now +#define TexcHere(msg) TextException((msg), Here()) + +/// Like assert() but throws an exception instead of aborting the process +/// and allows the caller to specify a custom exception message. +#define Must2(condition, message) \ + do { \ + if (!(condition)) { \ + const TextException Must_ex_((message), Here()); \ + debugs(0, 3, Must_ex_.what()); \ + throw Must_ex_; \ + } \ + } while (/*CONSTCOND*/ false) + +/// Like assert() but throws an exception instead of aborting the process. +#define Must(condition) Must2((condition), "check failed: " #condition) + +/// Reports and swallows all exceptions to prevent compiler warnings and runtime +/// errors related to throwing class destructors. Should be used for most dtors. +#define SWALLOW_EXCEPTIONS(code) \ + try { \ + code \ + } catch (...) { \ + debugs(0, DBG_IMPORTANT, "BUG: ignoring exception;\n" << \ + " bug location: " << Here() << "\n" << \ + " ignored exception: " << CurrentException); \ } - return lastHash; -} - -/// Avoids "defined but not used" warnings for FileNameHashCached -class FileNameHashCacheUser -{ - bool use(void *ptr=NULL) { return ptr != (void*)&FileNameHashCached; } -}; - -#if !defined(TexcHere) -# define TexcHere(msg) TextException((msg), __FILE__, __LINE__, \ - (FileNameHashCached(__FILE__)<<14) | (__LINE__ & 0x3FFF)) -#endif - -void Throw(const char *message, const char *fileName, int lineNo, unsigned int id); - -// Must(condition) is like assert(condition) but throws an exception instead -#if !defined(Must) -# define Must(cond) ((cond) ? \ - (void)0 : \ - (void)Throw(#cond, __FILE__, __LINE__, \ - (FileNameHashCached(__FILE__)<<14) | (__LINE__ & 0x3FFF))) -#endif #endif /* SQUID__TEXTEXCEPTION_H */ diff --git a/src/base/YesNoNone.h b/src/base/YesNoNone.h index 78527039f0..de4de55ca2 100644 --- a/src/base/YesNoNone.h +++ b/src/base/YesNoNone.h @@ -10,6 +10,7 @@ #define SQUID_YESNONONE_H_ #include "base/TextException.h" +#include "Debug.h" // TODO: generalize / template to non-boolean option types // and make YesNoNone the boolean instance of the template diff --git a/src/debug.cc b/src/debug.cc index 5c6d076d8e..e112dea1e8 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -845,29 +845,6 @@ Debug::Finish() // else it was a static topContext from Debug::Start() } -size_t -BuildPrefixInit() -{ - // XXX: This must be kept in sync with the actual debug.cc location - const char *ThisFileNameTail = "src/debug.cc"; - - const char *file=__FILE__; - - // Disable heuristic if it does not work. - if (!strstr(file, ThisFileNameTail)) - return 0; - - return strlen(file)-strlen(ThisFileNameTail); -} - -const char* -SkipBuildPrefix(const char* path) -{ - static const size_t BuildPrefixLength = BuildPrefixInit(); - - return path+BuildPrefixLength; -} - /// print data bytes using hex notation void Raw::printHex(std::ostream &os) const diff --git a/src/ipc/Forwarder.cc b/src/ipc/Forwarder.cc index d151b60177..e5aff1ba2b 100644 --- a/src/ipc/Forwarder.cc +++ b/src/ipc/Forwarder.cc @@ -27,20 +27,13 @@ Ipc::Forwarder::Forwarder(Request::Pointer aRequest, double aTimeout): AsyncJob("Ipc::Forwarder"), request(aRequest), timeout(aTimeout) { - debugs(54, 5, HERE); } Ipc::Forwarder::~Forwarder() { - debugs(54, 5, HERE); - Must(request->requestId == 0); - cleanup(); -} - -/// perform cleanup actions -void -Ipc::Forwarder::cleanup() -{ + SWALLOW_EXCEPTIONS({ + Must(request->requestId == 0); + }); } void @@ -79,7 +72,6 @@ Ipc::Forwarder::swanSong() DequeueRequest(request->requestId); request->requestId = 0; } - cleanup(); } bool diff --git a/src/ipc/Forwarder.h b/src/ipc/Forwarder.h index 12993e9099..2acdafadee 100644 --- a/src/ipc/Forwarder.h +++ b/src/ipc/Forwarder.h @@ -45,7 +45,6 @@ protected: virtual void swanSong(); virtual bool doneAll() const; - virtual void cleanup(); ///< perform cleanup actions virtual void handleError(); virtual void handleTimeout(); virtual void handleException(const std::exception& e); diff --git a/src/main.cc b/src/main.cc index e4310e3aab..91ed2d4b86 100644 --- a/src/main.cc +++ b/src/main.cc @@ -1350,26 +1350,6 @@ mainInitialize(void) configured_once = 1; } -/// describes active (i.e., thrown but not yet handled) exception -static std::ostream & -CurrentException(std::ostream &os) -{ - if (std::current_exception()) { - try { - throw; // re-throw to recognize the exception type - } - catch (const std::exception &ex) { - os << ex.what(); - } - catch (...) { - os << "[unknown exception type]"; - } - } else { - os << "[no active exception]"; - } - return os; -} - static void OnTerminate() { diff --git a/src/mgr/Forwarder.cc b/src/mgr/Forwarder.cc index 4a1afbc35b..7041a7213c 100644 --- a/src/mgr/Forwarder.cc +++ b/src/mgr/Forwarder.cc @@ -46,19 +46,17 @@ Mgr::Forwarder::Forwarder(const Comm::ConnectionPointer &aConn, const ActionPara Mgr::Forwarder::~Forwarder() { - debugs(16, 5, HERE); - Must(httpRequest != NULL); - Must(entry != NULL); - - HTTPMSGUNLOCK(httpRequest); - entry->unregisterAbort(); - entry->unlock("Mgr::Forwarder"); - cleanup(); + SWALLOW_EXCEPTIONS({ + Must(entry); + entry->unlock("Mgr::Forwarder"); + Must(httpRequest); + HTTPMSGUNLOCK(httpRequest); + }); } /// closes our copy of the client HTTP connection socket void -Mgr::Forwarder::cleanup() +Mgr::Forwarder::swanSong() { if (Comm::IsConnOpen(conn)) { if (closer != NULL) { @@ -68,6 +66,7 @@ Mgr::Forwarder::cleanup() conn->close(); } conn = NULL; + Ipc::Forwarder::swanSong(); } void diff --git a/src/mgr/Forwarder.h b/src/mgr/Forwarder.h index e419048795..cd71539563 100644 --- a/src/mgr/Forwarder.h +++ b/src/mgr/Forwarder.h @@ -38,7 +38,7 @@ public: protected: /* Ipc::Forwarder API */ - virtual void cleanup(); ///< perform cleanup actions + virtual void swanSong(); virtual void handleError(); virtual void handleTimeout(); virtual void handleException(const std::exception& e); diff --git a/src/mgr/IntParam.cc b/src/mgr/IntParam.cc index b304df0070..c77ed0af03 100644 --- a/src/mgr/IntParam.cc +++ b/src/mgr/IntParam.cc @@ -10,6 +10,7 @@ #include "squid.h" #include "base/TextException.h" +#include "Debug.h" #include "ipc/TypedMsgHdr.h" #include "mgr/IntParam.h" diff --git a/src/sbuf/Exceptions.cc b/src/sbuf/Exceptions.cc deleted file mode 100644 index d95fa6b386..0000000000 --- a/src/sbuf/Exceptions.cc +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Copyright (C) 1996-2018 The Squid Software Foundation and contributors - * - * Squid software is distributed under GPLv2+ license and includes - * contributions from numerous individuals and organizations. - * Please see the COPYING and CONTRIBUTORS files for details. - */ - -#include "squid.h" -#include "sbuf/Exceptions.h" -#include "sbuf/OutOfBoundsException.h" -#include "sbuf/SBuf.h" - -OutOfBoundsException::OutOfBoundsException(const SBuf &throwingBuf, - SBuf::size_type &pos, - const char *aFileName, int aLineNo) - : TextException(NULL, aFileName, aLineNo), - theThrowingBuf(throwingBuf), - accessedPosition(pos) -{ - SBuf explanatoryText("OutOfBoundsException"); - if (aLineNo != -1) - explanatoryText.appendf(" at line %d", aLineNo); - if (aFileName != NULL) - explanatoryText.appendf(" in file %s", aFileName); - explanatoryText.appendf(" while accessing position %d in a SBuf long %d", - pos, throwingBuf.length()); - message = xstrdup(explanatoryText.c_str()); -} - -OutOfBoundsException::~OutOfBoundsException() throw() -{ } - -InvalidParamException::InvalidParamException(const char *aFilename, int aLineNo) - : TextException("Invalid parameter", aFilename, aLineNo) -{ } - -SBufTooBigException::SBufTooBigException(const char *aFilename, int aLineNo) - : TextException("Trying to create an oversize SBuf", aFilename, aLineNo) -{ } - diff --git a/src/sbuf/Exceptions.h b/src/sbuf/Exceptions.h deleted file mode 100644 index 20d838df03..0000000000 --- a/src/sbuf/Exceptions.h +++ /dev/null @@ -1,34 +0,0 @@ -/* - * Copyright (C) 1996-2018 The Squid Software Foundation and contributors - * - * Squid software is distributed under GPLv2+ license and includes - * contributions from numerous individuals and organizations. - * Please see the COPYING and CONTRIBUTORS files for details. - */ - -#ifndef SQUID_SBUFEXCEPTIONS_H -#define SQUID_SBUFEXCEPTIONS_H - -#include "base/TextException.h" - -/** - * Exception raised when call parameters are not valid - * \todo move to an Exceptions.h? - */ -class InvalidParamException : public TextException -{ -public: - explicit InvalidParamException(const char *aFilename = 0, int aLineNo = -1); -}; - -/** - * Exception raised when an attempt to resize a SBuf would cause it to reserve too big - */ -class SBufTooBigException : public TextException -{ -public: - explicit SBufTooBigException(const char *aFilename = 0, int aLineNo = -1); -}; - -#endif /* SQUID_SBUFEXCEPTIONS_H */ - diff --git a/src/sbuf/Makefile.am b/src/sbuf/Makefile.am index 30a04a0120..6c5ae898d8 100644 --- a/src/sbuf/Makefile.am +++ b/src/sbuf/Makefile.am @@ -15,14 +15,11 @@ libsbuf_la_SOURCES = \ Algorithms.h \ DetailedStats.cc \ DetailedStats.h \ - Exceptions.cc \ - Exceptions.h \ forward.h \ List.cc \ List.h \ MemBlob.cc \ MemBlob.h \ - OutOfBoundsException.h \ SBuf.cc \ SBuf.h \ Stats.cc \ diff --git a/src/sbuf/OutOfBoundsException.h b/src/sbuf/OutOfBoundsException.h deleted file mode 100644 index 0c7d2f65d6..0000000000 --- a/src/sbuf/OutOfBoundsException.h +++ /dev/null @@ -1,31 +0,0 @@ -/* - * Copyright (C) 1996-2018 The Squid Software Foundation and contributors - * - * Squid software is distributed under GPLv2+ license and includes - * contributions from numerous individuals and organizations. - * Please see the COPYING and CONTRIBUTORS files for details. - */ - -#ifndef _SQUID_SRC_OUTOFBOUNDSEXCEPTION_H -#define _SQUID_SRC_OUTOFBOUNDSEXCEPTION_H - -#include "base/TextException.h" -#include "sbuf/SBuf.h" - -/** - * Exception raised when the user is going out of bounds when accessing - * a char within the SBuf - */ -class OutOfBoundsException : public TextException -{ -public: - OutOfBoundsException(const SBuf &buf, SBuf::size_type &pos, const char *aFileName = 0, int aLineNo = -1); - virtual ~OutOfBoundsException() throw(); - -protected: - SBuf theThrowingBuf; - SBuf::size_type accessedPosition; -}; - -#endif /* _SQUID_SRC_OUTOFBOUNDSEXCEPTION_H */ - diff --git a/src/sbuf/SBuf.cc b/src/sbuf/SBuf.cc index a6d201a7aa..d72fa02216 100644 --- a/src/sbuf/SBuf.cc +++ b/src/sbuf/SBuf.cc @@ -11,8 +11,6 @@ #include "base/RefCount.h" #include "Debug.h" #include "sbuf/DetailedStats.h" -#include "sbuf/Exceptions.h" -#include "sbuf/OutOfBoundsException.h" #include "sbuf/SBuf.h" #include "util.h" @@ -159,8 +157,7 @@ SBuf::rawAppendFinish(const char *start, size_type actualSize) debugs(24, 8, id << " finish appending " << actualSize << " bytes"); size_type newSize = length() + actualSize; - if (newSize > min(maxSize,store_->capacity-off_)) - throw SBufTooBigException(__FILE__,__LINE__); + Must2(newSize <= min(maxSize,store_->capacity-off_), "raw append overflow"); len_ = newSize; store_->size = off_ + newSize; } @@ -277,6 +274,7 @@ SBuf::vappendf(const char *fmt, va_list vargs) #else sz = vsnprintf(space, spaceSize(), fmt, vargs); #endif + Must2(sz >= 0, "vsnprintf() output error"); /* check for possible overflow */ /* snprintf on Linux returns -1 on output errors, or the size @@ -288,14 +286,9 @@ SBuf::vappendf(const char *fmt, va_list vargs) requiredSpaceEstimate = sz*2; // TODO: tune heuristics space = rawSpace(requiredSpaceEstimate); sz = vsnprintf(space, spaceSize(), fmt, vargs); - if (sz < 0) // output error in vsnprintf - throw TextException("output error in second-go vsnprintf",__FILE__, - __LINE__); + Must2(sz >= 0, "vsnprintf() output error despite increased buffer space"); } - if (sz < 0) // output error in either vsnprintf - throw TextException("output error in vsnprintf",__FILE__, __LINE__); - // data was appended, update internal state len_ += sz; @@ -863,17 +856,6 @@ SBuf::toUpper() ++stats.caseChange; } -/** - * checks whether the requested 'pos' is within the bounds of the SBuf - * \throw OutOfBoundsException if access is out of bounds - */ -void -SBuf::checkAccessBounds(size_type pos) const -{ - if (pos >= length()) - throw OutOfBoundsException(*this, pos, __FILE__, __LINE__); -} - /** re-allocate the backing store of the SBuf. * * If there are contents in the SBuf, they will be copied over. @@ -886,8 +868,7 @@ void SBuf::reAlloc(size_type newsize) { debugs(24, 8, id << " new size: " << newsize); - if (newsize > maxSize) - throw SBufTooBigException(__FILE__, __LINE__); + Must(newsize <= maxSize); MemBlob::Pointer newbuf = new MemBlob(newsize); if (length() > 0) newbuf->append(buf(), length()); diff --git a/src/sbuf/SBuf.h b/src/sbuf/SBuf.h index 5512341a2f..be943649cb 100644 --- a/src/sbuf/SBuf.h +++ b/src/sbuf/SBuf.h @@ -12,9 +12,9 @@ #define SQUID_SBUF_H #include "base/InstanceId.h" +#include "base/TextException.h" #include "Debug.h" #include "globals.h" -#include "sbuf/Exceptions.h" #include "sbuf/forward.h" #include "sbuf/MemBlob.h" #include "sbuf/Stats.h" @@ -233,7 +233,7 @@ public: /** random-access read to any char within the SBuf. * - * \throw OutOfBoundsException when access is out of bounds + * \throw std::exception when access is out of bounds * \note bounds is 0 <= pos < length(); caller must pay attention to signedness */ char at(size_type pos) const {checkAccessBounds(pos); return operator[](pos);} @@ -242,7 +242,7 @@ public: * * \param pos the position to be overwritten * \param toset the value to be written - * \throw OutOfBoundsException when pos is of bounds + * \throw std::exception when pos is of bounds * \note bounds is 0 <= pos < length(); caller must pay attention to signedness * \note performs a copy-on-write if needed. */ @@ -407,11 +407,10 @@ public: /** Get the length of the SBuf, as a signed integer * * Compatibility function for printf(3) which requires a signed int - * \throw SBufTooBigException if the SBuf is too big for a signed integer + * \throw std::exception if buffer length does not fit a signed integer */ int plength() const { - if (length()>INT_MAX) - throw SBufTooBigException(__FILE__, __LINE__); + Must(length() <= INT_MAX); return static_cast(length()); } @@ -426,7 +425,7 @@ public: * After the reserveSpace request, the SBuf is guaranteed to have at * least minSpace bytes of unused backing store following the currently * used portion and single ownership of the backing store. - * \throw SBufTooBigException if the user tries to allocate too big a SBuf + * \throw std::exception if the user tries to allocate a too big SBuf */ void reserveSpace(size_type minSpace) { Must(minSpace <= maxSize); @@ -440,7 +439,7 @@ public: * minCapacity bytes of total buffer size, including the currently-used * portion; it is also guaranteed that after this call this SBuf * has unique ownership of the underlying memory store. - * \throw SBufTooBigException if the user tries to allocate too big a SBuf + * \throw std::exception if the user tries to allocate a too big SBuf */ void reserveCapacity(size_type minCapacity); @@ -652,7 +651,7 @@ private: void cow(size_type minsize = npos); - void checkAccessBounds(size_type pos) const; + void checkAccessBounds(const size_type pos) const { Must(pos < length()); } /** Exports a writable pointer to the SBuf internal storage. * \warning Use with EXTREME caution, this is a dangerous operation. @@ -669,7 +668,7 @@ private: * buffer ownership. It is instead optimized for a one writer * (appender), many readers scenario by avoiding unnecessary * copying and allocations. - * \throw SBufTooBigException if the user tries to allocate too big a SBuf + * \throw std::exception if the user tries to allocate a too big SBuf */ char *rawSpace(size_type minSize); diff --git a/src/sbuf/forward.h b/src/sbuf/forward.h index ded38d844f..7c4f28b7f6 100644 --- a/src/sbuf/forward.h +++ b/src/sbuf/forward.h @@ -19,10 +19,6 @@ class SBufIterator; class SBufReverseIterator; class SBufReservationRequirements; -class OutOfBoundsException; -class InvalidParamException; -class SBufTooBigException; - class SBufStats; typedef std::list SBufList; diff --git a/src/snmp/Forwarder.cc b/src/snmp/Forwarder.cc index 066ab1e394..29c59b1d12 100644 --- a/src/snmp/Forwarder.cc +++ b/src/snmp/Forwarder.cc @@ -35,7 +35,7 @@ Snmp::Forwarder::Forwarder(const Pdu& aPdu, const Session& aSession, int aFd, /// removes our cleanup handler of the client connection socket void -Snmp::Forwarder::cleanup() +Snmp::Forwarder::swanSong() { if (fd >= 0) { if (closer != NULL) { @@ -44,6 +44,7 @@ Snmp::Forwarder::cleanup() } fd = -1; } + Ipc::Forwarder::swanSong(); } /// called when the client socket gets closed by some external force diff --git a/src/snmp/Forwarder.h b/src/snmp/Forwarder.h index dfc9279654..9980230833 100644 --- a/src/snmp/Forwarder.h +++ b/src/snmp/Forwarder.h @@ -34,7 +34,7 @@ public: protected: /* Ipc::Forwarder API */ - virtual void cleanup(); ///< perform cleanup actions + virtual void swanSong(); virtual void handleTimeout(); virtual void handleException(const std::exception& e); diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 1fd0dc0a9e..7180b7ee34 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -1116,7 +1116,7 @@ Ssl::SSL_add_untrusted_cert(SSL *ssl, X509 *cert) untrustedStack = sk_X509_new_null(); if (!SSL_set_ex_data(ssl, ssl_ex_index_ssl_untrusted_chain, untrustedStack)) { sk_X509_pop_free(untrustedStack, X509_free); - throw TextException("Failed to attach untrusted certificates chain"); + throw TextException("Failed to attach untrusted certificates chain", Here()); } } sk_X509_push(untrustedStack, cert); diff --git a/src/tests/stub_debug.cc b/src/tests/stub_debug.cc index ee0c566288..adc9428342 100644 --- a/src/tests/stub_debug.cc +++ b/src/tests/stub_debug.cc @@ -97,12 +97,6 @@ void Debug::parseOptions(char const *) {} -const char* -SkipBuildPrefix(const char* path) -{ - return path; -} - Debug::Context *Debug::Current = nullptr; Debug::Context::Context(const int aSection, const int aLevel): diff --git a/src/tests/stub_libmgr.cc b/src/tests/stub_libmgr.cc index d41235142f..7777291991 100644 --- a/src/tests/stub_libmgr.cc +++ b/src/tests/stub_libmgr.cc @@ -96,7 +96,7 @@ void Mgr::CountersAction::dump(StoreEntry* entry) STUB //Mgr::Forwarder::Forwarder(int aFd, const ActionParams &aParams, HttpRequest* aRequest, StoreEntry* anEntry) STUB //Mgr::Forwarder::~Forwarder() STUB //protected: -void Mgr::Forwarder::cleanup() STUB +//void Mgr::Forwarder::swanSong() STUB void Mgr::Forwarder::handleError() STUB void Mgr::Forwarder::handleTimeout() STUB void Mgr::Forwarder::handleException(const std::exception& e) STUB diff --git a/src/tests/testSBuf.h b/src/tests/testSBuf.h index c3f61d1da5..d6ff58e9c9 100644 --- a/src/tests/testSBuf.h +++ b/src/tests/testSBuf.h @@ -11,7 +11,7 @@ #include -#include "sbuf/OutOfBoundsException.h" +#include "base/TextException.h" /* * test the SBuf functionalities @@ -30,7 +30,7 @@ class testSBuf : public CPPUNIT_NS::TestFixture CPPUNIT_TEST( testAppendStdString ); CPPUNIT_TEST( testAppendf ); CPPUNIT_TEST( testSubscriptOp ); - CPPUNIT_TEST_EXCEPTION( testSubscriptOpFail, OutOfBoundsException ); + CPPUNIT_TEST_EXCEPTION( testSubscriptOpFail, TextException ); CPPUNIT_TEST( testComparisons ); CPPUNIT_TEST( testConsume ); CPPUNIT_TEST( testRawContent ); diff --git a/src/tests/testSBufList.h b/src/tests/testSBufList.h index 551f578004..c9466c49f8 100644 --- a/src/tests/testSBufList.h +++ b/src/tests/testSBufList.h @@ -11,8 +11,6 @@ #include -#include "sbuf/OutOfBoundsException.h" - class testSBufList : public CPPUNIT_NS::TestFixture { CPPUNIT_TEST_SUITE( testSBufList ); diff --git a/test-suite/mem_hdr_test.cc b/test-suite/mem_hdr_test.cc index 7412dfae7c..2a6d3a5dab 100644 --- a/test-suite/mem_hdr_test.cc +++ b/test-suite/mem_hdr_test.cc @@ -9,7 +9,6 @@ /* DEBUG: section 19 Store Memory Primitives */ #include "squid.h" -#include "base/TextException.h" #include "Generic.h" #include "mem_node.h" #include "stmem.h" @@ -17,13 +16,6 @@ #include #include -// required on some platforms -unsigned int -TextException::FileNameHash(const char *) -{ - return 0; -} - void testLowAndHigh() { diff --git a/tools/Makefile.am b/tools/Makefile.am index 01b06c9371..a315847ad4 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -34,6 +34,9 @@ test_tools.cc: $(top_srcdir)/test-suite/test_tools.cc stub_debug.cc: $(top_srcdir)/src/tests/stub_debug.cc cp $(top_srcdir)/src/tests/stub_debug.cc $@ +Here.cc: $(top_srcdir)/src/base/Here.cc + cp $(top_srcdir)/src/base/Here.cc $@ + MemBuf.cc: $(top_srcdir)/src/MemBuf.cc cp $(top_srcdir)/src/MemBuf.cc $@ @@ -54,7 +57,7 @@ STUB.h: $(top_srcdir)/src/tests/STUB.h # globals.cc is needed by test_tools.cc. # Neither of these should be disted from here. TESTSOURCES= test_tools.cc -CLEANFILES += test_tools.cc MemBuf.cc stub_debug.cc time.cc stub_cbdata.cc stub_libmem.cc STUB.h +CLEANFILES += test_tools.cc Here.cc MemBuf.cc stub_debug.cc time.cc stub_cbdata.cc stub_libmem.cc STUB.h ## Test Scripts EXTRA_DIST += helper-ok-dying.pl helper-ok.pl @@ -66,6 +69,7 @@ DEFAULT_CACHEMGR_CONFIG = $(sysconfdir)/cachemgr.conf libexec_PROGRAMS = cachemgr$(CGIEXT) cachemgr__CGIEXT__SOURCES = cachemgr.cc \ + Here.cc \ MemBuf.cc \ stub_cbdata.cc \ stub_debug.cc \ diff --git a/tools/squidclient/Makefile.am b/tools/squidclient/Makefile.am index 7b3810869e..295036dbc4 100644 --- a/tools/squidclient/Makefile.am +++ b/tools/squidclient/Makefile.am @@ -14,6 +14,7 @@ DISTCLEANFILES = LDADD = \ $(top_builddir)/src/ip/libip.la \ + $(top_builddir)/src/base/libbase.la \ $(top_builddir)/lib/libmiscencoding.la \ $(top_builddir)/lib/libmiscutil.la \ $(COMPAT_LIB) \