Closed Bug 1285649 Opened 3 years ago Closed 3 years ago

dom/bindings/BindingUtils.cpp:93:16: error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: dholbert, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

I tried building with clang 3.9 today (on Linux64), and I got this build error (from a build-warning being promoted to an error due to --enable-warnings-as-errors in my mozconfig)

{
$SRC/dom/bindings/BindingUtils.cpp:93:16: error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Werror,-Wvarargs]
  va_start(ap, aErrorNumber);
               ^

$SRC/dom/bindings/BindingUtils.cpp:90:48: note: parameter of type 'const mozilla::dom::ErrNum' is declared here
ThrowErrorMessage(JSContext* aCx, const ErrNum aErrorNumber, ...)
                                               ^
}

hg blame points to ms2ger (specifically this line of one of his patches):
 https://hg.mozilla.org/mozilla-central/rev/1ed8454e7090#l1.39

Hence, tagging him to take a look if he could.
Flags: needinfo?(Ms2ger)
Blocks: 1285668
See also https://bugzilla.mozilla.org/show_bug.cgi?id=1282795#c7 where Waldo fixed a similar error in the JS shell.
Thank you! I kept trying to find that bug and failed.

I'm not sure what the best solution would be in this case.
Flags: needinfo?(Ms2ger) → needinfo?(jwalden+bmo)
This compiles through at least a few translation units that see the signature (and I think call it), so it might do the trick?  In any case it was a bit dumb to have an OOL symbol here, templates really were the smarter/easier/simpler solution.
Attachment #8772494 - Flags: review?(nfroyd)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8772494 [details] [diff] [review]
Change the ellipsis function to a template function, eliminate the middleman, restore world peace

Review of attachment 8772494 [details] [diff] [review]:
-----------------------------------------------------------------

A question about the inline-ness and a strong complaint about the size impact of this change below.

::: dom/bindings/ErrorResult.h
@@ +71,5 @@
> +const JSErrorFormatString*
> +GetErrorMessage(void* aUserRef, const unsigned aErrorNumber);
> +
> +template<typename... Ts>
> +inline bool

What if this |inline| declaration doesn't get respected?  Aren't we essentially back in the same boat?

@@ +74,5 @@
> +template<typename... Ts>
> +inline bool
> +ThrowErrorMessage(JSContext* aCx, const ErrNum aErrorNumber, Ts&&... aArgs)
> +{
> +  JS_ReportErrorNumber(aCx, GetErrorMessage, nullptr,

I'm not super-excited about adding these constant arguments to every single call site for code-bloat reasons: we already have ~7k calls to ThrowErrorMessage in generating bindings, and that number is only ever going to go up.  Back of the envelope calculations say this change is ~60K extra bytes of text on x86-64 Linux (lea X(%rip), $REG1 / xor $REG2, $REG2 for loading these two arguments is nine bytes) just for error cases, which IMHO isn't worth it.  I think the numbers are actually roughly comparable on x86, and possibly even worse on ARM...

I know this is subject to my |inline| complaint above, but what about:

namespace detail {

bool
ThrowErrorMessage(JSContext* aCx, const unsigned aErrorNumber, ...);

}

template<typename... Ts>
inline bool
ThrowErrorMessage(JSContext* aCx, const ErrNum aErrorNumber, Ts&&... aArgs)
{
  detail::ThrowErrorMessage(aCx, static_cast<const unsigned>(aErrorNumber),
                            mozilla::Forward<Ts>(aArgs)...);
}

Does that solve the warnings?
Attachment #8772494 - Flags: review?(nfroyd)
Attached patch Alterna-patchSplinter Review
We really need to get rid of those two stupid arguments.

detail is ambiguous between mozilla::dom::detail and mozilla::detail without the change to that one file.  Haven't built a whole tree, but this is fairly plausible as a version of your approach.
Attachment #8772518 - Flags: review?(nfroyd)
Attachment #8772494 - Attachment is obsolete: true
Comment on attachment 8772518 [details] [diff] [review]
Alterna-patch

Review of attachment 8772518 [details] [diff] [review]:
-----------------------------------------------------------------

WFM.  Is the AtomicRefCountedWithFinalize.h change necessary here, or is that included from some other patch?
Attachment #8772518 - Flags: review?(nfroyd) → review+
We've been using binding_detail because of that ambiguity thing, for what it's worth.  So we have mozilla::dom::binding_detail and mozilla:detail.
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a2627378a0d
Replace an ellipsis function, whose last non-ellipsis argument was subject to integer promotions, with a variadic template function so as not to invoke C++ undefined behavior, because that would be Bad.  NOT REVIEWED YET
(In reply to Pulsebot from comment #8)
> Pushed by jwalden@mit.edu:
> [...] NOT REVIEWED YET

Looks like you forgot to update the commit message to note r=froydnj. Maybe worth a backout + reland? (with DONTBUILD to avoid incurring extra treeherder build/test cycles)
Flags: needinfo?(jwalden+bmo)
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e17ed0283ffd
Replace an ellipsis function, whose last non-ellipsis argument was subject to integer promotions, with a variadic template function so as not to invoke C++ undefined behavior, because that would be Bad.  r=froydnj
Hmm.  I'm kinda not sure it's worth the trouble (and especially not if I forget DONTBUILD, bah), but eh.  Redone.
Flags: needinfo?(jwalden+bmo)
https://hg.mozilla.org/mozilla-central/rev/e17ed0283ffd
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.