1290170 - jsctypes-test.cpp:332:18: error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Werror,-Wvarargs]

Status: RESOLVED DUPLICATE of bug 1324248

(Core :: js-ctypes, defect)

Description

[Daniel Holbert [:dholbert]]

I'm trying to build with clang 3.9 (on Linux64), and I get these two build errors in jsctypes-test.cpp (from a build-warning being promoted to an error due to --enable-warnings-as-errors in my mozconfig):

ERROR #1:
{
$SRC/toolkit/components/ctypes/tests/jsctypes-test.cpp:332:18: error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Werror,-Wvarargs]

   va_start(list, n);
                  ^
$SRC/toolkit/components/ctypes/tests/jsctypes-test.cpp:328:27: note: parameter of type 'uint8_t' (aka 'unsigned char') is declared here

 test_sum_va_cdecl(uint8_t n, ...)
                           ^
}

ERROR #2:
{
$SRC/toolkit/components/ctypes/tests/jsctypes-test.cpp:344:18: error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Werror,-Wvarargs]
   va_start(list, n);
                  ^
$SRC/toolkit/components/ctypes/tests/jsctypes-test.cpp:340:34: note: parameter of type 'uint8_t' (aka 'unsigned char') is declared here
 test_count_true_va_cdecl(uint8_t n, ...)
                                  ^
}

For reference, this is in code that was added in bug 554790, here (using type "PRUint8" at that point in time, later autoconverted to uint8_t):
  https://hg.mozilla.org/mozilla-central/rev/faf4147ffe7f#l4.41

jorendorff, looks like you were involved on that bug.  Do you know if there's any significance to the fact that we were using 8-bit integers here? Can we just convert this to uint32_t instead? (That fixes the issue locally for me, but I don't understand this test well enough to know if that'd somehow break what the test is trying to test.)

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> jorendorff, looks like you were involved on that bug.  Do you know if
> there's any significance to the fact that we were using 8-bit integers here?
> Can we just convert this to uint32_t instead? (That fixes the issue locally
> for me, but I don't understand this test well enough to know if that'd
> somehow break what the test is trying to test.)

I'm guessing that change might break the test somehow, since the accompanying JS file explicitly mentions an 8-bit integer being associated with the first function here:
      sum_va = library.declare("test_sum_va_cdecl", ctypes.default_abi, ctypes.int32_t,
                               ctypes.uint8_t, "...");

The second function (test_count_true_va_cdecl) isn't mentioned at all in the JS file (or anywhere outside of its definition/declaration), actually, though -- maybe that one should just be removed?  (Or have the missing JS test code added?)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: strawman fix: s/uint8_t/uint32_t/ for these types

(For illustration purposes -- this fixes the issue for me, but I'm not sure if it's correct.)

Comment 3

[Daniel Holbert [:dholbert]]

Looks like this was separately filed (& hacked around) in bug 1324248.

I'll dupe & transfer dependencies to that bug.