[ffi] Varargs should give an error on uint8, int8, uint16, int16 and float

[dcharkes]

Passing 8 bit or 16 bit ints in varargs is undefined behavior.

Clang will alert us:

Second argument to 'va_arg' is of promotable type 'uint16_t' (aka 'unsigned short'); this va_arg has undefined behavior because arguments will be promoted to 'int'clang(-Wvarargs)

But we allow this in dart:ffi, leading to undefined behavior.

We should consider adding an error message.

Discovered on:

• CI on windows is flaky native#1191

[vaishnavi-2901]

hey
can work on it, please assign this task to me

[dcharkes]

hey can work on it, please assign this task to me

Hi @vaishnavi-2901! That's great!

Please start with https://github.com/dart-lang/sdk/blob/main/CONTRIBUTING.md

A PR for this issue will involve adding error messages both to the CFE (the compiler used when doing dart ...) and analyzer (used in the IDE for giving error messages).

Some pointers to point you in the right direction:

• Analyzer
  • https://dart-review.googlesource.com/c/sdk/+/278343 analyzer implementation for varargs, not including any error messages.
  • https://dart-review.googlesource.com/c/sdk/+/336040 How error messages are defined.
  • You can see in the ffi_verifier.dart file in the first PR how to emit error messages from the analyzer.
• CFE
  • https://dart-review.googlesource.com/c/sdk/+/276921/49/pkg/vm/lib/transformations/ffi/common.dart -> This is where the error checking for the FFI is done.
• Tests
  • The tests should be in here. tests/ffi/static_checks/vmspecific_static_checks_varargs_test.dart Note that this test needs to be converted from //# 1: compile-time error to tests/ffi/static_checks/vmspecific_static_checks_test.dart style with // [cfe].

It's probably easiest to

1. First convert the test
2. Then make an analyzer PR
3. And then in a third PR try to tackle the CFE.

[trivedikavya]

hey sir can i work on it ..?

[dcharkes]

@vaishnavi-2901 are you still working on this?

[trivedikavya]

Dear respected sir let she work and let me also work on it. as I am beginning the open source contribution first you will check the request of both and then you will see what to do
I have one Question sir just tell me in which file I have to contribute .

[codesculpture]

Hi @dcharkes , as you said clang just warn and promote Int8 Int16 (also Uint's) to 32bit data,

But in dart side you are suggesting to throw error. Am not sure whether someone will report this or ask this, "Clang is allowing (just warning) but why dart is throwing error for varargs for 8bit, 16bit data"

(am not familiar with ffi eco system, just wondering some program work (warn) when u compile natively (clang or gcc), but same program ends in error when runs by dart, is or will this bother someone ?)

Sorry if this is too dumb.

[dcharkes]

That has to do with the philosophy of C compilers: They warn and don't error on undefined behavior. (For sanity sake, we usually pass -Werror to the C compiler to turn all warnings into errors. And also -Wall to get all warnings. We don't want undefined behavior.) As for the philosophy why C compilers do this, you find plenty of discussions on StackOverflow and friends.

In Dart we have a slightly different approach. We want to prevent programs from compiling that will crash. Now on the dart:ffi front this becomes slightly ambiguous because we have a Dart philosophy but then call into C code which has a different philosophy. We tend to take the most conservative of the two philosophies on the boundary. Sometimes we end up being too conservative and users file bugs to loosen the restrictions. But we can always loosen restrictions later as a non-breaking change. Introducing restrictions causes a breaking change.

For this specific bug. We saw undefined behavior leading to mangled enum and integer values and crashes. See the bug that is referenced in the top.

[codesculpture]

I think this is a bit off topic, but do you have any idea why variadic dint allow < 4 byte data such as short or char ?

Because i wondered if its about the size then we can create custom data with any size we want using struct,

For ex:

This code throw warning (eventually exit when executing this code, i compiled with gcc),

short a = va_arg(arg, short)

But if we have,

struct Short {
      short a;
};

...

struct a = va_arg(arg, struct Short)

....

The above don't have any warning and successfully executing (actually interpreting short)

Am not sure is this valid, but am sure it is not affect anything here dart (thats why i mentioned it is off topic)

Reason for this question is i just imagined C will reject any data type which is less than 4 byte (including struct) inside va_arg, so i thought we here in dart should also do that check. But C is not doing that, so we can also skip that.

[dcharkes]

Reason for this question is i just imagined C will reject any data type which is less than 4 byte

C just happily accepts things, even if it's undefined behavior. And the "best" part: the undefined behavior might "work" with some C compilers on some hardware, but not with other compilers. So you think your code is correct by testing it on one system and it not crashing, but it ends up crashing on another system.

P.S. If you want to explore what various compilers do: https://godbolt.org/ You can play around with adding fields to the struct or changing the type and seeing how that changes the generated machine code.