Debug Framework changes for dump routines and custom assert #300

anilsadineni · 2019-08-26T10:47:06Z

These changes along with changes in sonic-utilities are needed to test the framework

The code is inline with design from:
sonic-net/SONiC#398

These changes along with changes in sonic-utilities are needed to test the framework The code is inline with design from: sonic-net/SONiC#398

These changes go along with sonic-net/sonic-swss-common#300

qiluo-msft · 2019-09-25T23:47:49Z

common/debugframework.h

+        UNSPECIFIED
+    };
+    static void setAssertAction(std::string val);
+    static void custom_assert(bool exp, AssertAction act, const char *func, const unsigned line);


custom_assert [](start = 16, length = 13)

customAssert #Closed

qiluo-msft · 2019-09-25T23:50:59Z

common/debugframework.h

+
+#ifdef assert
+#undef assert
+#define assert(exp) Debugframework::custom_assert(exp, Debugframework::AssertAction::UNSPECIFIED, __PRETTY_FUNCTION__, __LINE__)


assert [](start = 8, length = 6)

Why re-define standard assert macro? Originally assert will only take effect when build in DEBUG mode, and totally optimized in RELEASE mode. Your use case is different. You may choose another name.

The basic idea here is that without having to change the code where assert has been used, bring customAssert and have this enabled even in production builds. Based on the configuration either log or collect more info or abort shall be action when assert condition is met and hence standard assert is re-defined.

I am worried about the side-effect. Assume there is a third party header file, which uses assert intensively for DEBUG build. If someone include this header before third party one, then there will be overhead to RELEASE build.

In reply to: 328907805 [](ancestors = 328907805)

I am not sure of the overhead to release build you mention. What would otherwise have no effect in release build will now have some outcome when assert condition is hit. Is that what you refer?

The outcome of assert is now controlled using a default or configuration command. The idea is to not miss out on assert but have some insight into it and the level of overhead is controlled. So I am not sure if we can term that overhead or side-effect, this is by design the desired effect for better debug ability.

qiluo-msft · 2019-09-25T23:52:46Z

common/debugframework.h

+
+typedef void (*DumpInfoFunc)(std::string, KeyOpFieldsValuesTuple);
+
+class Debugframework 


Debugframework [](start = 6, length = 14)

DebugFramework #Closed

changed this in lot of places..

qiluo-msft · 2019-09-25T23:54:17Z

common/debugframework.cpp

+}
+
+Debugframework::Debugframework() {
+    // Read .ini configuration file to init


Read .ini configuration file to init [](start = 7, length = 36)

Is the comment related to code? #Closed

It is now deadcode. The original plan was to have a init config file but since this is library and not daemon it did not make lot of sense to have a config file. Shall remove.

qiluo-msft · 2019-09-25T23:58:56Z

common/debugframework.cpp

+
+Debugframework::~Debugframework() {
+    //m_registeredComps.clear();
+    //m_configParams.clear();


remove dead code #Closed

qiluo-msft · 2019-09-25T23:59:34Z

common/debugframework.cpp

+    //m_configParams.clear();
+}
+
+void Debugframework::linkWithFrameworkNoThread(std::string &componentName)


std::string & [](start = 47, length = 13)

const std::string & or std::string
Many more below. #Closed

qiluo-msft · 2019-09-26T00:00:49Z

common/debugframework.cpp

+    std::thread t(Debugframework::runnableThread, componentName, funcPtr);
+    t.detach();
+
+}


remove extra empty line #Closed

qiluo-msft · 2019-09-26T00:01:58Z

common/schema.h

@@ -44,6 +44,11 @@ namespace swss {
 #define APP_SFLOW_TABLE_NAME              "SFLOW_TABLE"
 #define APP_SFLOW_SESSION_TABLE_NAME      "SFLOW_SESSION_TABLE"
 #define APP_SFLOW_SAMPLE_RATE_TABLE_NAME  "SFLOW_SAMPLE_RATE_TABLE"
+#define APP_DEBUGFM_CONFIG_TABLE_NAME     "DEBUGFM_CONFIG_TABLE"


APP [](start = 8, length = 3)

Maybe the tables should be moved to ConfigDB. #Closed

This was discussed in the design review meeting. This is really only on a trigger and hence considered to place it in the APP_DB.

qiluo-msft · 2019-09-26T18:39:26Z

common/debugframework.h

+
+typedef void (*DumpInfoFunc)(std::string, KeyOpFieldsValuesTuple);
+
+class Debugframework 


[](start = 20, length = 1)

an extra trailing blank #Closed

qiluo-msft · 2019-09-26T18:39:42Z

common/debugframework.h

+class Debugframework 
+{
+public:
+    static Debugframework  &getInstance();       /* To have only one instance aka singleton */


[](start = 26, length = 1)

an extra blank

qiluo-msft · 2019-09-26T18:39:58Z

common/debugframework.h

+    static Debugframework  &getInstance();       /* To have only one instance aka singleton */
+
+    //typedef std::function< void (std::string componentName, KeyOpFieldsValuesTuple args)> DumpInfoFunc;
+


remove dead code #Closed

qiluo-msft · 2019-09-26T18:43:28Z

common/debugframework.cpp

+#include "notificationconsumer.h"
+#include "notificationproducer.h"
+
+#define ACTIONS_CONFIG_FILE "/etc/sonic/dbgFm.ini"


ACTIONS_CONFIG_FILE [](start = 8, length = 19)

How is the macro used? #Closed

Same as above. With library doesnt make lot of sense. Shall remove this deadcode.

qiluo-msft · 2019-09-26T19:33:01Z

common/debugframework.cpp

+                   try{
+                       (*funcPtr)(componentName, entry);
+                   }catch(...){
+                       throw "Registered dump routine returned error";


returned error [](start = 54, length = 14)

threw an exception #Closed

This is already in the context of debug routines and additional exception handling is an overhead and hence not changing this for now.

qiluo-msft · 2019-09-26T19:34:13Z

common/debugframework.cpp

+                   }
+
+                   /* Inform done with same key */
+                   FieldValueTuple fvResp("RESPONSE", "SUCCESS");


SUCCESS [](start = 55, length = 7)

~~When exception thrown, still SUCCESS?~~ #Closed

Oops! response failure in the catch is missed. Shall correct it

Sorry, I gave the wrong comment. In your second commit, any code after throw will not run, so it is dead code. Original code should be good. #Closed

qiluo-msft · 2019-09-26T19:37:01Z

common/debugframework.cpp

+    {
+        // open a file in append mode and keep it open
+        std::fstream *outfile;
+        std::string options("/var/log/" + component + "_debug.log");


/var/log/ [](start = 29, length = 9)

Don't use fixed path, you should have a configuration for it.

The post scripts are TBD and for now there is nothing in configDB. When the post scripts (TBD) are in place this needs to allign with those scripts. Shall defer this till that point.

qiluo-msft · 2019-09-26T21:08:55Z

common/debugframework.cpp

+    }
+    else
+    {
+        char buffer[0x1000];


0x1000 [](start = 20, length = 6)

Better to allocate on heap #Closed

This is short lived and called several times and trying to get from heap might be really costly.

qiluo-msft · 2019-09-26T21:12:15Z

common/debugframework.cpp

+        if (itr != dbgfm.m_compFds.end())
+        {
+            f = itr->second;
+            *f << buffer << "\n";


"\n" [](start = 28, length = 4)

If you need flush, use endl #Closed

qiluo-msft · 2019-09-26T21:14:20Z

common/debugframework.cpp

+                std::string comp("all");
+                std::string args("DETAIL:FULL;");
+                invokeTrigger(comp, args);
+            }


break ? #Closed

As you see comment in line 427 break is not added intentionally, dump is exhaustive info and that should include backtrace as well

qiluo-msft

As comments.
The c++ source files should be well formatted like all existing files.

kcudnik · 2019-09-30T20:30:29Z

common/debugframework.cpp

+
+void Debugframework::linkWithFramework(std::string &componentName, const DumpInfoFunc funcPtr)
+{
+    if (funcPtr == NULL)


missing SWSS_LOG_ENTER() in many functions

Added for all relevant functions except in small utility functions that get called repeatedly

kcudnik · 2019-09-30T20:31:21Z

common/debugframework.cpp

+        std::fstream *outfile;
+        std::string options("/var/log/" + component + "_debug.log");
+        try{
+            outfile = new std::fstream();


why this is a pointer ? not actual object? on s tack ?

This is created during begin and kept open while the components dumps info using multiple writes and in the end this is closed and freed.

kcudnik · 2019-09-30T20:34:03Z

common/debugframework.h

+#ifndef NO_RET_TYPE
+#define NO_RET_TYPE [[noreturn]]
+#endif
+    NO_RET_TYPE static void runnableThread(std::string componentName, const DumpInfoFunc funcPtr);   


if this is thread function, threads should be able to quit loop in nice way, for example in dtr there should be join thread and break run flag in thread so norturn flag would not be needed

This comes into picture if the component registers with a thread option and the thread stays until the component exits so this really never returns and hence the noreturn flag.

kcudnik · 2019-09-30T20:34:50Z

common/debugframework.h

+    Debugframework &operator=(const Debugframework&);
+
+    static void listenDoneEvents(std::string componentName);
+    static void relayTrigger(std::string &componentName, KeyOpFieldsValuesTuple args);  


all paramters in functions should be defined 1 per line and with prefixes In _Inout or Out

I am new to project and tried to stay consistent with other files in the dir.

kcudnik · 2019-11-06T20:09:59Z

common/debugframework.cpp

+                        (*funcPtr)(componentName, entry);
+                    }catch(...){
+                        throw "Registered dump routine returned error";
+                        fvResp.second = "FAILURE";


will be never executed,
also please log error message if catch was catched

kcudnik · 2019-11-06T20:10:43Z

common/debugframework.cpp

+    return;
+}
+
+void DebugFramework::dumpWrite(const std::string &component, const char *fmt, ...)


one param per line, also add In /Out etc prefixes

Are you referring to SAL annotation style of doxygen style? As mentioned in response to your earlier comment, I tried to stay consistent with the other files in the directory and elsewhere in the project. Can you please point me to any other file which followed your suggestion for me to be clear. Thanks,

kcudnik · 2019-11-06T20:11:13Z

common/debugframework.cpp

+        {
+            std::fstream *out = itr->second;
+            out->close();
+            delete out;


use shared pointers in map/set, to not explicitly delete them

Good suggestion and optimization. However for now there are no components using this framework. Once the usage picks up, we need to revisit the file naming conventions, post scripts, uploads and based on that these changes might be moot. For now this is not in usual operations and only during debug and hence this might not be a lot of overhead even without the suggested optimization.

kcudnik · 2019-11-06T20:11:50Z

common/debugframework.cpp

+}
+
+DebugFramework::PostAction DebugFramework::getPostAction()
+{


missing SWSS_LOG_ENTER(); on every function

I have added to all the functions which does meaningful work other than helper functions and utility functions which will be called multiple times. Please check my comment above when I changed to address your review comments

kcudnik · 2019-11-06T20:12:15Z

common/debugframework.cpp

+        case BTRACE:
+            {
+                std::FILE *fp = fopen(ASSERT_FILE, "w");
+#define SIZE 100


please define at the file beginning and add more meaningfull name

qiluo-msft · 2019-11-06T22:49:23Z

common/debugframework.cpp

+    table.set(component, fieldValues);
+}
+
+NO_RET_TYPE void DebugFramework::runnableThread(const std::string componentName, const DumpInfoFunc funcPtr)


const std::string [](start = 48, length = 17)

const std::string & #Closed

qiluo-msft · 2019-11-06T22:50:58Z

common/debugframework.cpp

+    }  // end while loop
+}
+
+void DebugFramework::invokeTrigger(const std::string componentName, std::string argList)


const std::string [](start = 35, length = 17)

const std::string & #Closed

qiluo-msft · 2019-11-06T22:51:35Z

common/debugframework.cpp

+    t.detach();
+}
+
+void DebugFramework::listenDoneEvents(const std::string componentName)


const std::string [](start = 38, length = 17)

const std::string & #Closed

Debug Framework changes for dump routines and custom assert

24730e7

These changes along with changes in sonic-utilities are needed to test the framework The code is inline with design from: sonic-net/SONiC#398

anilsadineni added a commit to anilsadineni/sonic-utilities that referenced this pull request Aug 26, 2019

Utility functions and scripts for Debug Framework

ff143fb

These changes go along with sonic-net/sonic-swss-common#300

anilsadineni mentioned this pull request Aug 26, 2019

Utility functions and scripts for Debug Framework sonic-net/sonic-utilities#618

Open

lguohan requested a review from qiluo-msft August 29, 2019 16:22

lguohan assigned qiluo-msft Aug 29, 2019

qiluo-msft requested a review from kcudnik September 25, 2019 23:40

qiluo-msft reviewed Sep 25, 2019

View reviewed changes

qiluo-msft reviewed Sep 26, 2019

View reviewed changes

kcudnik reviewed Sep 30, 2019

View reviewed changes

bhaveshdell previously approved these changes Oct 18, 2019

View reviewed changes

Address review comments for pull/398

7448ff9

kcudnik reviewed Nov 6, 2019

View reviewed changes

qiluo-msft reviewed Nov 6, 2019

View reviewed changes

Address review comments-2

759f3d2

qiluo-msft dismissed bhaveshdell’s stale review via 759f3d2 July 29, 2021 03:33


		typedef void (*DumpInfoFunc)(std::string, KeyOpFieldsValuesTuple);

		class Debugframework

		static Debugframework &getInstance(); /* To have only one instance aka singleton */

		//typedef std::function< void (std::string componentName, KeyOpFieldsValuesTuple args)> DumpInfoFunc;

Debug Framework changes for dump routines and custom assert #300

Are you sure you want to change the base?

Debug Framework changes for dump routines and custom assert #300

Conversation

anilsadineni commented Aug 26, 2019

qiluo-msft Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

kcudnik Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcudnik Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 25, 2019 •

edited

Loading

qiluo-msft Sep 25, 2019 •

edited

Loading

qiluo-msft Sep 25, 2019 •

edited

Loading

qiluo-msft Sep 25, 2019 •

edited

Loading

qiluo-msft Sep 25, 2019 •

edited

Loading

qiluo-msft Sep 26, 2019 •

edited

Loading

qiluo-msft Sep 26, 2019 •

edited

Loading

qiluo-msft Sep 26, 2019 •

edited

Loading

qiluo-msft Sep 26, 2019 •

edited

Loading

qiluo-msft Sep 26, 2019 •

edited

Loading

qiluo-msft Sep 26, 2019 •

edited

Loading

qiluo-msft Sep 26, 2019 •

edited

Loading

qiluo-msft Nov 6, 2019 •

edited

Loading

qiluo-msft Sep 26, 2019 •

edited

Loading

qiluo-msft Sep 26, 2019 •

edited

Loading

qiluo-msft Sep 26, 2019 •

edited

Loading

kcudnik Sep 30, 2019 •

edited

Loading

kcudnik Sep 30, 2019 •

edited

Loading

qiluo-msft Nov 6, 2019 •

edited

Loading

qiluo-msft Nov 6, 2019 •

edited

Loading

qiluo-msft Nov 6, 2019 •

edited

Loading