From 4a5bcf0f7a069e4f97493d0c881ddac2f049a80a Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Wed, 5 Dec 2012 20:02:30 -0500 Subject: [PATCH 01/15] Rework debugging check in BReferenceable. - If a BReferenceable object is deleted with a non-zero reference count, we now test to see if the object was allocated on the stack. If so, we don't flag a warning if the reference count is currently 1. --- src/kits/support/Referenceable.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/kits/support/Referenceable.cpp b/src/kits/support/Referenceable.cpp index 6b2ee363b0..39cc641f33 100644 --- a/src/kits/support/Referenceable.cpp +++ b/src/kits/support/Referenceable.cpp @@ -27,8 +27,18 @@ BReferenceable::BReferenceable() BReferenceable::~BReferenceable() { #ifdef DEBUG - if (fReferenceCount > 1) - debugger("Deleted object which still had references.\n"); + if (fReferenceCount != 0) { + // Simple heuristic to test if this object was allocated + // on the stack: check if this is within 1KB in either + // direction of the current stack address, and the reference + // count is 1. If so, we don't flag a warning since that would + // imply the object was allocated/destroyed on the stack + // without any references being acquired or released. + char test; + int64 testOffset = (int64)this - (int64)&test; + if (testOffset < -1024 || testOffset > 1024 || fReferenceCount != 1) + debugger("Deleted referenceable object with non-zero ref count."); + } #endif } From 8326ad45bb970b97b541bb13ccad1404b1289dac Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Wed, 5 Dec 2012 20:04:34 -0500 Subject: [PATCH 02/15] Destroy created file via reference release if necessary. --- src/apps/debugger/dwarf/DwarfManager.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/apps/debugger/dwarf/DwarfManager.cpp b/src/apps/debugger/dwarf/DwarfManager.cpp index 5d02dfed71..b6fb6bfadb 100644 --- a/src/apps/debugger/dwarf/DwarfManager.cpp +++ b/src/apps/debugger/dwarf/DwarfManager.cpp @@ -41,13 +41,14 @@ DwarfManager::LoadFile(const char* fileName, DwarfFile*& _file) if (file == NULL) return B_NO_MEMORY; + BReference fileReference(file, true); status_t error = file->Load(fileName); if (error != B_OK) { - delete file; return error; } fFiles.Add(file); + fileReference.Detach(); // we keep the initial reference for ourselves file->AcquireReference(); From 95b3c61c4bea18edbf3e9597749b2ad0cdf1ef1b Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Wed, 5 Dec 2012 20:05:14 -0500 Subject: [PATCH 03/15] DebugReportGenerator shouldn't inherit from BReferenceable. --- src/apps/debugger/controllers/DebugReportGenerator.cpp | 2 -- src/apps/debugger/controllers/DebugReportGenerator.h | 5 +---- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/apps/debugger/controllers/DebugReportGenerator.cpp b/src/apps/debugger/controllers/DebugReportGenerator.cpp index 12b4fa25dd..996e67f631 100644 --- a/src/apps/debugger/controllers/DebugReportGenerator.cpp +++ b/src/apps/debugger/controllers/DebugReportGenerator.cpp @@ -32,8 +32,6 @@ DebugReportGenerator::DebugReportGenerator(::Team* team) : BLooper("DebugReportGenerator"), - BReferenceable(), - fTeam(team), fArchitecture(team->GetArchitecture()), fTeamDataSem(-1) diff --git a/src/apps/debugger/controllers/DebugReportGenerator.h b/src/apps/debugger/controllers/DebugReportGenerator.h index 1267586aee..1ea4dff8e0 100644 --- a/src/apps/debugger/controllers/DebugReportGenerator.h +++ b/src/apps/debugger/controllers/DebugReportGenerator.h @@ -7,7 +7,6 @@ #include -#include #include "Team.h" @@ -19,9 +18,7 @@ class Team; class Thread; -class DebugReportGenerator : public BLooper, public BReferenceable, - public Team::Listener -{ +class DebugReportGenerator : public BLooper, public Team::Listener { public: DebugReportGenerator(::Team* team); ~DebugReportGenerator(); From 969609f0efcb2c841027e33762f0c2dd83e8e494 Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Wed, 5 Dec 2012 20:06:08 -0500 Subject: [PATCH 04/15] Fix double free. --- src/apps/debugger/controllers/TeamDebugger.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/apps/debugger/controllers/TeamDebugger.cpp b/src/apps/debugger/controllers/TeamDebugger.cpp index 3e92cb12d0..6c8f43ecd2 100644 --- a/src/apps/debugger/controllers/TeamDebugger.cpp +++ b/src/apps/debugger/controllers/TeamDebugger.cpp @@ -284,7 +284,6 @@ TeamDebugger::~TeamDebugger() delete fMemoryBlockManager; fReportGenerator->Lock(); fReportGenerator->Quit(); - delete fReportGenerator; delete fWorker; delete fTeam; delete fFileManager; From cb44a2a6ef3db2d924be5a56708cbea99ff13e5e Mon Sep 17 00:00:00 2001 From: Alexander von Gluck IV Date: Thu, 6 Dec 2012 03:59:38 +0000 Subject: [PATCH 05/15] swpipe: Fix memory pointer scope issue * Store stVisual in each context --- src/add-ons/opengl/swpipe/GalliumContext.cpp | 24 +++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/add-ons/opengl/swpipe/GalliumContext.cpp b/src/add-ons/opengl/swpipe/GalliumContext.cpp index 3a9a22911a..6e91cd08e0 100644 --- a/src/add-ons/opengl/swpipe/GalliumContext.cpp +++ b/src/add-ons/opengl/swpipe/GalliumContext.cpp @@ -73,10 +73,14 @@ hgl_viewport(struct gl_context* glContext, GLint x, GLint y, } -static void -hgl_fill_st_visual(st_visual* stVisual, gl_config* glVisual) +static st_visual* +hgl_fill_st_visual(gl_config* glVisual) { - memset(stVisual, 0, sizeof(*stVisual)); + struct st_visual* stVisual = CALLOC_STRUCT(st_visual); + if (!stVisual) { + ERROR("%s: Couldn't allocate st_visual\n", __func__); + return NULL; + } // Determine color format if (glVisual->redBits == 8) { @@ -129,6 +133,8 @@ hgl_fill_st_visual(st_visual* stVisual, gl_config* glVisual) if (glVisual->haveDepthBuffer || glVisual->haveStencilBuffer) stVisual->buffer_mask |= ST_ATTACHMENT_DEPTH_STENCIL_MASK; + + return stVisual; } @@ -292,11 +298,10 @@ GalliumContext::CreateContext(Bitmap *bitmap) TRACE("stencilBits :\t%d\n", glVisual->stencilBits); // Convert Mesa calculated visual into state tracker visual - struct st_visual stVisual; - hgl_fill_st_visual(&stVisual, glVisual); + context->stVisual = hgl_fill_st_visual(glVisual); - context->draw = new GalliumFramebuffer(&stVisual); - context->read = new GalliumFramebuffer(&stVisual); + context->draw = new GalliumFramebuffer(context->stVisual); + context->read = new GalliumFramebuffer(context->stVisual); if (!context->draw || !context->read) { ERROR("%s: Problem allocating framebuffer!\n", __func__); @@ -312,7 +317,7 @@ GalliumContext::CreateContext(Bitmap *bitmap) memset(&attribs, 0, sizeof(attribs)); attribs.options.force_glsl_extensions_warn = false; attribs.profile = ST_PROFILE_DEFAULT; - attribs.visual = stVisual; + attribs.visual = *context->stVisual; attribs.major = 1; attribs.minor = 0; //attribs.flags |= ST_CONTEXT_FLAG_DEBUG; @@ -418,6 +423,9 @@ GalliumContext::DestroyContext(context_id contextID) if (fContext[contextID]->draw) delete fContext[contextID]->draw; + if (fContext[contextID]->stVisual) + FREE(fContext[contextID]->stVisual); + if (fContext[contextID]->manager) FREE(fContext[contextID]->manager); From a34020ba21777c21f57e3bb765d083b38ae9ad1e Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Thu, 6 Dec 2012 17:57:14 -0500 Subject: [PATCH 06/15] A bit more fine-tuning to BReferenceable debugging. - Rework quick stack range check as suggested by Ingo. - If the ref count is > 1 we invoke the debugger unconditionally. - If equal to 1, we first perform a quick heuristic check to see if the var might be on the stack. If we can't conclusively determine that is, we make certain by comparing to the thread's actual stack range. --- src/kits/support/Referenceable.cpp | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/kits/support/Referenceable.cpp b/src/kits/support/Referenceable.cpp index 39cc641f33..1aaedec5d9 100644 --- a/src/kits/support/Referenceable.cpp +++ b/src/kits/support/Referenceable.cpp @@ -27,7 +27,8 @@ BReferenceable::BReferenceable() BReferenceable::~BReferenceable() { #ifdef DEBUG - if (fReferenceCount != 0) { + bool enterDebugger = false; + if (fReferenceCount == 1) { // Simple heuristic to test if this object was allocated // on the stack: check if this is within 1KB in either // direction of the current stack address, and the reference @@ -35,10 +36,22 @@ BReferenceable::~BReferenceable() // imply the object was allocated/destroyed on the stack // without any references being acquired or released. char test; - int64 testOffset = (int64)this - (int64)&test; - if (testOffset < -1024 || testOffset > 1024 || fReferenceCount != 1) - debugger("Deleted referenceable object with non-zero ref count."); - } + size_t testOffset = (addr_t)this - (addr_t)&test; + if (testOffset > 1024 || -testOffset > 1024) { + // might still be a stack object, check the thread's + // stack range to be sure. + thread_info info; + status_t result = get_thread_info(find_thread(NULL), &info); + if (result != B_OK || this < info.stack_base + || this > info.stack_end) { + enterDebugger = true; + } + } + } else if (fReferenceCount != 0) + enterDebugger = true; + + if (enterDebugger) + debugger("Deleted referenceable object with non-zero ref count."); #endif } From 0c8935498e54b1698d6e1173d37a5c370a012991 Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Thu, 6 Dec 2012 17:59:49 -0500 Subject: [PATCH 07/15] Fix one more case of deleting instead of reference releasing. --- src/apps/debugger/value/value_nodes/BListValueNode.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/apps/debugger/value/value_nodes/BListValueNode.cpp b/src/apps/debugger/value/value_nodes/BListValueNode.cpp index 91ea46f2b6..bd0073f931 100644 --- a/src/apps/debugger/value/value_nodes/BListValueNode.cpp +++ b/src/apps/debugger/value/value_nodes/BListValueNode.cpp @@ -246,29 +246,28 @@ BListValueNode::ResolvedLocationAndValue(ValueLoader* valueLoader, if (strcmp(member->Name(), "fObjectList") == 0) { error = baseType->ResolveDataMemberLocation(member, *location, memberLocation); + BReference locationRef(memberLocation, true); if (error != B_OK) { TRACE_LOCALS( "BListValueNode::ResolvedLocationAndValue(): " "failed to resolve location of header member: %s\n", strerror(error)); - delete memberLocation; return error; } error = valueLoader->LoadValue(memberLocation, valueType, false, fDataLocation); - delete memberLocation; if (error != B_OK) return error; } else if (strcmp(member->Name(), "fItemCount") == 0) { error = baseType->ResolveDataMemberLocation(member, *location, memberLocation); + BReference locationRef(memberLocation, true); if (error != B_OK) { TRACE_LOCALS( "BListValueNode::ResolvedLocationAndValue(): " "failed to resolve location of header member: %s\n", strerror(error)); - delete memberLocation; return error; } @@ -280,7 +279,6 @@ BListValueNode::ResolvedLocationAndValue(ValueLoader* valueLoader, BVariant listSize; error = valueLoader->LoadValue(memberLocation, valueType, false, listSize); - delete memberLocation; if (error != B_OK) return error; From 690a7f3d47063a25241d33554c48810c6eed3183 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dariusz=20Knoci=C5=84ski?= Date: Thu, 6 Dec 2012 22:50:57 +0100 Subject: [PATCH 08/15] BStringList: Fix compare_private_data[ignore_case]() Signed-off-by: Ingo Weinhold --- src/kits/support/StringList.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/kits/support/StringList.cpp b/src/kits/support/StringList.cpp index 900322d967..7b453d825a 100644 --- a/src/kits/support/StringList.cpp +++ b/src/kits/support/StringList.cpp @@ -17,16 +17,16 @@ static int compare_private_data(const void* a, const void* b) { - return BString::Private::StringFromData((char*)a).Compare( - BString::Private::StringFromData((char*)b)); + return BString::Private::StringFromData(*(char**)a).Compare( + BString::Private::StringFromData(*(char**)b)); } static int compare_private_data_ignore_case(const void* a, const void* b) { - return BString::Private::StringFromData((char*)a).ICompare( - BString::Private::StringFromData((char*)b)); + return BString::Private::StringFromData(*(char**)a).ICompare( + BString::Private::StringFromData(*(char**)b)); } @@ -367,7 +367,7 @@ BStringList::Flatten(void* buf, ssize_t size) const if (size < FlattenedSize()) return B_NO_MEMORY; - + int32 count = CountStrings(); for (int32 i = 0; i < count; i++) { BString item = StringAt(i); @@ -403,7 +403,7 @@ BStringList::Unflatten(type_code code, const void* buffer, ssize_t size) } return B_OK; -} +} void From 715c56d45435a373b9f581960fd352506a348922 Mon Sep 17 00:00:00 2001 From: John Scipione Date: Thu, 6 Dec 2012 19:16:04 -0500 Subject: [PATCH 09/15] Style fixes only, no functional changes intended including converting a while loop into a for loop. --- src/apps/terminal/AppearPrefView.cpp | 8 ++++---- src/apps/terminal/Colors.cpp | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/apps/terminal/AppearPrefView.cpp b/src/apps/terminal/AppearPrefView.cpp index 7da4e58cd4..4d2a291561 100644 --- a/src/apps/terminal/AppearPrefView.cpp +++ b/src/apps/terminal/AppearPrefView.cpp @@ -291,6 +291,7 @@ AppearancePrefView::MessageReceived(BMessage* msg) _EnableCustomColors(true); else _EnableCustomColors(false); + _ChangeColorScheme(newScheme); modified = true; } @@ -393,14 +394,13 @@ AppearancePrefView::_SetCurrentColorScheme(BMenuField* field) const char* currentSchemeName = NULL; - color_scheme** schemes - = const_cast(gPredefinedColorSchemes); - while (*schemes) { + for (color_scheme** schemes + = const_cast(gPredefinedColorSchemes); + *schemes != NULL; schemes++) { if (gCustomColorScheme == **schemes) { currentSchemeName = (*schemes)->name; break; } - schemes++; } for (int32 i = 0; i < fColorSchemeField->Menu()->CountItems(); i++) { diff --git a/src/apps/terminal/Colors.cpp b/src/apps/terminal/Colors.cpp index 7125514330..1eae1d6417 100644 --- a/src/apps/terminal/Colors.cpp +++ b/src/apps/terminal/Colors.cpp @@ -14,7 +14,7 @@ // Standard colors -const rgb_color kBlack= { 0, 0, 0, 255 }; +const rgb_color kBlack = { 0, 0, 0, 255 }; const rgb_color kGreen = { 0, 255, 0, 255 }; const rgb_color kWhite = { 255, 255, 255, 255 }; const rgb_color kYellow = { 255, 255, 0, 255 }; From 72bf9c4e480784cde000b41982f3afb6f7d00e2a Mon Sep 17 00:00:00 2001 From: John Scipione Date: Thu, 6 Dec 2012 19:34:12 -0500 Subject: [PATCH 10/15] Make Default color scheme match default settings... So that it gets selected, well, by default. Before Custom was being selected initially. --- src/apps/terminal/Colors.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apps/terminal/Colors.cpp b/src/apps/terminal/Colors.cpp index 1eae1d6417..161727dbac 100644 --- a/src/apps/terminal/Colors.cpp +++ b/src/apps/terminal/Colors.cpp @@ -24,7 +24,7 @@ const struct color_scheme kColorSchemeDefault = { B_TRANSLATE("Default"), kBlack, kWhite, - kWhite, + kBlack, kBlack, kWhite, kBlack From 5ddf877be64a8d6aa12e6f856d3b748c5ba045d4 Mon Sep 17 00:00:00 2001 From: John Scipione Date: Thu, 6 Dec 2012 19:38:15 -0500 Subject: [PATCH 11/15] More style fixes, return true or false directly --- src/apps/terminal/Colors.cpp | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/apps/terminal/Colors.cpp b/src/apps/terminal/Colors.cpp index 161727dbac..7d89c64a99 100644 --- a/src/apps/terminal/Colors.cpp +++ b/src/apps/terminal/Colors.cpp @@ -85,27 +85,24 @@ struct color_scheme gCustomColorScheme = { }; const color_scheme* gPredefinedColorSchemes[] = { - &kColorSchemeDefault, - &kColorSchemeBlue, - &kColorSchemeMidnight, - &kColorSchemeProfessional, - &kColorSchemeRetro, - &kColorSchemeSlate, - &gCustomColorScheme, - NULL + &kColorSchemeDefault, + &kColorSchemeBlue, + &kColorSchemeMidnight, + &kColorSchemeProfessional, + &kColorSchemeRetro, + &kColorSchemeSlate, + &gCustomColorScheme, + NULL }; bool color_scheme::operator==(const color_scheme& scheme) { - if (text_fore_color == scheme.text_fore_color + return text_fore_color == scheme.text_fore_color && text_back_color == scheme.text_back_color && cursor_fore_color == scheme.cursor_fore_color && cursor_back_color == scheme.cursor_back_color && select_fore_color == scheme.select_fore_color - && select_back_color == scheme.select_back_color) - return true; - - return false; + && select_back_color == scheme.select_back_color; } From 992c7397cf62bda71c2dfdee0c49575820f6bbd8 Mon Sep 17 00:00:00 2001 From: John Scipione Date: Fri, 7 Dec 2012 01:08:34 -0500 Subject: [PATCH 12/15] Remove ugly const_cast, thanks Ingo --- src/apps/terminal/AppearPrefView.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/apps/terminal/AppearPrefView.cpp b/src/apps/terminal/AppearPrefView.cpp index 4d2a291561..06b11900f4 100644 --- a/src/apps/terminal/AppearPrefView.cpp +++ b/src/apps/terminal/AppearPrefView.cpp @@ -394,8 +394,7 @@ AppearancePrefView::_SetCurrentColorScheme(BMenuField* field) const char* currentSchemeName = NULL; - for (color_scheme** schemes - = const_cast(gPredefinedColorSchemes); + for (const color_scheme** schemes = gPredefinedColorSchemes; *schemes != NULL; schemes++) { if (gCustomColorScheme == **schemes) { currentSchemeName = (*schemes)->name; From d2091d8876b95f8677532d2c608beb92a69bf9b6 Mon Sep 17 00:00:00 2001 From: John Scipione Date: Fri, 7 Dec 2012 01:38:29 -0500 Subject: [PATCH 13/15] Change the cursor from white to yellow for blue theme matching the text color. The white was a little off. --- src/apps/terminal/Colors.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apps/terminal/Colors.cpp b/src/apps/terminal/Colors.cpp index 7d89c64a99..90672f3d99 100644 --- a/src/apps/terminal/Colors.cpp +++ b/src/apps/terminal/Colors.cpp @@ -35,7 +35,7 @@ const struct color_scheme kColorSchemeBlue = { kYellow, { 0, 0, 139, 255 }, kBlack, - kWhite, + kYellow, kBlack, { 0, 139, 139, 255 }, }; From 587caece753c32f82382534c2f1ed28de4998601 Mon Sep 17 00:00:00 2001 From: John Scipione Date: Fri, 7 Dec 2012 02:35:09 -0500 Subject: [PATCH 14/15] Revert "Make Default color scheme match default settings..." This reverts commit 72bf9c4e480784cde000b41982f3afb6f7d00e2a. I fixed this the wrong way. --- src/apps/terminal/Colors.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apps/terminal/Colors.cpp b/src/apps/terminal/Colors.cpp index 90672f3d99..56cbbea212 100644 --- a/src/apps/terminal/Colors.cpp +++ b/src/apps/terminal/Colors.cpp @@ -24,7 +24,7 @@ const struct color_scheme kColorSchemeDefault = { B_TRANSLATE("Default"), kBlack, kWhite, - kBlack, + kWhite, kBlack, kWhite, kBlack From 8ccd1d0386e635ed0ef5aaf17bb0f3d237aa4bfb Mon Sep 17 00:00:00 2001 From: John Scipione Date: Fri, 7 Dec 2012 02:39:14 -0500 Subject: [PATCH 15/15] Make the default cursor fore color white --- src/apps/terminal/PrefHandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apps/terminal/PrefHandler.cpp b/src/apps/terminal/PrefHandler.cpp index 41f96bf0db..7c120f7db4 100644 --- a/src/apps/terminal/PrefHandler.cpp +++ b/src/apps/terminal/PrefHandler.cpp @@ -44,7 +44,7 @@ static const pref_defaults kTermDefaults[] = { { PREF_TEXT_FORE_COLOR, " 0, 0, 0" }, { PREF_TEXT_BACK_COLOR, "255, 255, 255" }, - { PREF_CURSOR_FORE_COLOR, " 0, 0, 0" }, + { PREF_CURSOR_FORE_COLOR, "255, 255, 255" }, { PREF_CURSOR_BACK_COLOR, " 0, 0, 0" }, { PREF_SELECT_FORE_COLOR, "255, 255, 255" }, { PREF_SELECT_BACK_COLOR, " 0, 0, 0" },