Do not pass non-POD type variables through variadic function
ClosedPublic

Authored by krytarowski on Sat, Jan 28, 4:16 PM.

Details

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>

Diff Detail

Repository
rL LLVM
krytarowski created this revision.Sat, Jan 28, 4:16 PM
joerg added a comment.Sat, Jan 28, 4:24 PM

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

krytarowski retitled this revision from Do not pass non-trivial type variables through variadic function to Do not pass non-POD type variables through variadic function.Sat, Jan 28, 4:48 PM
krytarowski edited the summary of this revision. (Show Details)
ki.stfu accepted this revision.Sun, Jan 29, 9:39 PM

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

This revision is now accepted and ready to land.Sun, Jan 29, 9:39 PM

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.

krytarowski edited the summary of this revision. (Show Details)Mon, Jan 30, 2:42 AM

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?

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

#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 accepted this revision.Mon, Jan 30, 5:59 PM

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 added a subscriber: tnn.Tue, Jan 31, 1:26 PM

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

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

@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 updated this revision to Diff 86644.Wed, Feb 1, 8:10 AM
krytarowski edited the summary of this revision. (Show Details)

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

krytarowski closed this revision.Wed, Feb 1, 8:14 AM