Do not pass non-POD type variables through variadic function

[krytarowski]

Summary

Cannot pass object of non-POD type 'const CMIUtilString' through variadic function.

This behavior is undefined according to C++11 5.2.2/7:

Passing a potentially-evaluated argument of class type having a non-trivial copy constructor, a non-trivial move contructor, or a non-trivial destructor, with no corresponding parameter, is conditionally-supported with implementation-defined semantics.

Replace SetErrorDescriptionn(errMsg); with SetErrorDescription(errMsg);

Original patch by Tobias Nygren (NetBSD).

Sponsored by <The NetBSD Foundation>

[joerg]

Should be "non-POD type" in the commit message.

[ki.stfu]

I don't know the point of this patch (probably it's something special for NetBSD? @emaste) but I'm okay with that.

[krytarowski]

In D29256#660072, @ki.stfu wrote:

I don't know the point of this patch (probably it's something special for NetBSD? @emaste) but I'm okay with that.

It's undefined (implementation defined) behavior.

C++11 5.2.2/7:

Passing a potentially-evaluated argument of class type having a non-trivial copy constructor, a non-trivial move contructor, or a non-trivial destructor, with no corresponding parameter, is conditionally-supported with implementation-defined semantics.

This patch was created to address compiler warning.

[ki.stfu]

In D29256#660159, @krytarowski wrote:

It's undefined (implementation defined) behavior.

C++11 5.2.2/7:

Passing a potentially-evaluated argument of class type having a non-trivial copy constructor, a non-trivial move contructor, or a non-trivial destructor, with no corresponding parameter, is conditionally-supported with implementation-defined semantics.

Interesting. Passing to what? I thought it means we shouldn't pass non-trivial types through variadic arguments (... expression), and in this case we don't do it because CMIUtilString is the type of the parameter vFormating which has a name.

Unfortunately I can't reproduce it on my Ubuntu using the following example:

#include <cstdarg>
#include <iostream>
 
struct Count {
    int value;
    explicit Count(int v) : value(v) { }
    // Make it non-trivial
    Count(const Count&) : value(0) { }
    Count(Count&&)      : value(0) { }
    ~Count() { if (value) value |= 1; }
};
 
int add_nums(Count count, ...) 
{
    int result = 0;
    va_list args;
    va_start(args, count);
    for (int i = 0; i < count.value; ++i) {
        result += va_arg(args, int);
    }
    va_end(args);
    return result;
}
 
int main() 
{
    std::cout << add_nums(Count(4), 25, 25, 50, 50) << '\n';
    return 0;
}

This patch was created to address compiler warning.

Could you tell me what compiler and platform do you use and provide the exact warning message?

[krytarowski]

Hmm, I'm not reproducing it right now either. It's a patch added a year ago. I've pinged Tobias.

[joerg]

#include <string>
#include <cstdarg>

void f(std::string msg, ...) {
  va_list ap;
  va_start(ap, msg);
}

compiled against libc++ gives:
test.cc:6:3: error: cannot pass object of non-POD type 'std::string' (aka 'basic_string<char, char_traits<char>,

allocator<char> >') through variadic function; call will abort at runtime [-Wnon-pod-varargs]

Joerg

[labath]

Interesting. Passing to what? I thought it means we shouldn't pass non-trivial types through variadic arguments (... expression), and in this case we don't do it because CMIUtilString is the type of the parameter vFormating which has a name.

Presumably the "undefinedness" comes from the fact that va_start uses the last non-... argument to locate the variadic args on the stack. And a non-POD type may cause the stack to be layed out in a way the the function does not expect.

[• tnn]

Attaching full error message as requested. Still reproducable on NetBSD's head-LLVM snapshot.

lldb-build.txt2 KBDownload

[krytarowski]

@ki.stfu do you still agree with this patch?

[ki.stfu]

In D29256#662167, @krytarowski wrote:

@ki.stfu do you still agree with this patch?

Please follow my comments and then we can go ahead.

tools/lldb-mi/MIDriver.cpp

512

SetErrorDescription accepts a const reference to CMIUtilString so it suits better than SetErrorDescriptionn (double n).

SetErrorDescription(errMsg);

Oh, certainly it should be renamed

[krytarowski]

Replace SetErrorDescriptionn(errMsg); with SetErrorDescription(errMsg);

tools/lldb-mi/MICmnBase.h

 class CMICmnBase {
   // Methods:
 public:
   /* ctor */ CMICmnBase();
 
   bool HaveErrorDescription() const;
   const CMIUtilString &GetErrorDescription() const;
   void SetErrorDescription(const CMIUtilString &vrTxt) const;
-  void SetErrorDescriptionn(const CMIUtilString vFormat, ...) const;
+  void SetErrorDescriptionn(const char *vFormat, ...) const;
   void SetErrorDescriptionNoLog(const CMIUtilString &vrTxt) const;
   void ClrErrorDescription() const;
 
   // Overrideable:
 public:
   /* dtor */ virtual ~CMICmnBase();
 
   // Attributes:
 protected:
   mutable CMIUtilString m_strMILastErrorDescription;
   bool m_bInitialized; // True = yes successfully initialized, false = no yet or
                        // failed
   CMICmnLog *m_pLog;   // Allow all derived classes to use the logger
   MIint m_clientUsageRefCnt; // Count of client using *this object so not
                              // shutdown() object to early
 };

tools/lldb-mi/MICmnBase.cpp

 //          and sent
 //          to the Logger.
 // Type:    Method.
 // Args:    vFormat - (R) Format string.
 //          ...     - (R) Variable number of CMIUtilString type objects.
 // Return:  None.
 // Throws:  None.
 //--
-void CMICmnBase::SetErrorDescriptionn(const CMIUtilString vFormat, ...) const {
+void CMICmnBase::SetErrorDescriptionn(const char *vFormat, ...) const {
   va_list args;
   va_start(args, vFormat);
   CMIUtilString strResult = CMIUtilString::FormatValist(vFormat, args);
   va_end(args);
 
   SetErrorDescription(strResult);
 }

tools/lldb-mi/MIDriver.cpp

bool CMIDriver::StartWorkerThreads() {
   // Grab the thread manager
   CMICmnThreadMgrStd &rThreadMgr = CMICmnThreadMgrStd::Instance();
 
   // Start the event polling thread
   if (bOk && !rThreadMgr.ThreadStart<CMICmnLLDBDebugger>(m_rLldbDebugger)) {
     const CMIUtilString errMsg = CMIUtilString::Format(
         MIRSRC(IDS_THREADMGR_ERR_THREAD_FAIL_CREATE),
         CMICmnThreadMgrStd::Instance().GetErrorDescription().c_str());
-    SetErrorDescriptionn(errMsg);
+    SetErrorDescription(errMsg);
     return MIstatus::failure;
   }
 
   return bOk;
 }
 
 //++
 //------------------------------------------------------------------------------------

tools/lldb-mi/MIUtilString.h

 //--
 class CMIUtilString : public std::string {
   // Typedefs:
 public:
   typedef std::vector<CMIUtilString> VecString_t;
 
   // Static method:
 public:
-  static CMIUtilString Format(const CMIUtilString vFormating, ...);
+  static CMIUtilString Format(const char *vFormating, ...);
   static CMIUtilString FormatBinary(const MIuint64 vnDecimal);
   static CMIUtilString FormatValist(const CMIUtilString &vrFormating,
                                     va_list vArgs);
   static bool IsAllValidAlphaAndNumeric(const char *vpText);
   static bool Compare(const CMIUtilString &vrLhs, const CMIUtilString &vrRhs);
   static CMIUtilString ConvertToPrintableASCII(const char vChar,
                                                bool bEscapeQuotes = false);
   static CMIUtilString ConvertToPrintableASCII(const char16_t vChar16,

tools/lldb-mi/MIUtilString.cpp

 // object is
 //          created and returned.
 // Type:    Static method.
 // Args:    vFormat       - (R) Format string data instruction.
 //          ...           - (R) Var list args of any type.
 // Return:  CMIUtilString - Number of splits found in the string data.
 // Throws:  None.
 //--
-CMIUtilString CMIUtilString::Format(const CMIUtilString vFormating, ...) {
+CMIUtilString CMIUtilString::Format(const char *vFormating, ...) {
   va_list args;
   va_start(args, vFormating);
   CMIUtilString strResult = CMIUtilString::FormatPriv(vFormating, args);
   va_end(args);
 
   return strResult;
 }