Re: [PATCH v2 2/4] Add wrappers for some Python APIs

[Tom Tromey <tom@tromey.com>]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

>> +static inline gdbpy_borrowed_ref
>> +gdbpy_dict_get_item_with_error (gdbpy_borrowed_ref dict,
>> +				gdbpy_borrowed_ref key)
>> +{
>> +  PyObject *result = PyDict_GetItemWithError (dict, key);
>> +  if (result == nullptr)
>> +    throw gdb_python_exception ();
>> +  return result;

Andrew> PyDict_GetItemWithError will return NULL *without* an exception set if
Andrew> KEY is not found in DICT.  This will then trigger an assert in the
Andrew> gdb_python_exception constructor. Because of the *cough* lack of
Andrew> comments, it's unclear what the function's intended behaviour is, but
Andrew> this feels like a bug.

Thanks for catching this, it is indeed a bug.

I added small comments to most of the functions, and then expanded on
them where I thought it made sense.  In most cases I figured the big
comment at the top of the file covers the details.

I changed this particular function to return gdbpy_opt_borrowed_ref in
the case where PyDict_GetItemWithError returns NULL without setting the
exception.

>> +static inline gdbpy_ref<>
>> +gdbpy_unicode_from_format (const char *fmt, ...)
>> +{
>> +  va_list args;
>> +  va_start (args, fmt);
>> +  gdbpy_ref<> result (PyUnicode_FromFormatV (fmt, args));
>> +  if (result == nullptr)
>> +    throw gdb_python_exception ();
>> +  va_end (args);

Andrew> If an exception is thrown in this case then we fail to call va_end,
Andrew> which I believe is technically undefined behaviour (though in reality
Andrew> it's probably harmless), still, we should fix this.

I used a template function here and elsewhere in this file.

Note that this discovery means that many other uses of va_end in gdb are
already wrong.

>> +template<typename... Arg>
>> +[[noreturn]] void
>> +gdbpy_err_format (gdbpy_borrowed_ref type, const char *fmt, Arg... args)
>> +{
>> +  PyErr_Format (type, fmt, std::forward<Arg> (args)...);

Andrew> This isn't really a request for a change, more a question for my own
Andrew> education.  My understanding is that std::forward is intended to be used
Andrew> with forwarding reference arguments, which ARGS is not.

Yeah, I agree with your analysis.  We don't need perfect forwarding here
so I've removed the std::forward.

Tom