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

RESOLVED DUPLICATE of bug 1324248

Status

()

Core
js-ctypes
RESOLVED DUPLICATE of bug 1324248
a year ago
10 months ago

People

(Reporter: dholbert, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox50 affected)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.)
(Reporter)

Updated

a year ago
Blocks: 1285668
(Reporter)

Updated

a year ago
Flags: needinfo?(jorendorff)
(Reporter)

Comment 1

a year ago
(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?)
(Reporter)

Comment 2

a year ago
Created attachment 8775669 [details] [diff] [review]
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.)
(Reporter)

Comment 3

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

I'll dupe & transfer dependencies to that bug.
No longer blocks: 1285668
Status: NEW → RESOLVED
Last Resolved: 10 months ago
No longer depends on: 554790
Resolution: --- → DUPLICATE
Duplicate of bug: 1324248
(Reporter)

Updated

10 months ago
Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.