Variadic functions improvements / unformatted functions

ZimM-LostPolygon commented 9 months ago

Added --generateexplodedvarargsfunctions option to generate variants of variadic functions with up to a number of explicit args, useful for languages without variadic function support (https://github.com/dearimgui/dear_bindings/issues/45)
Added --generateunformattedfunctions option to generate unformatted variants of format string supporting functions (fixed https://github.com/dearimgui/dear_bindings/issues/45)
Added --nogeneratedefaultargfunctions option to disable the non-Ex/Ex function split, useful for languages with default arg support (fixed https://github.com/dearimgui/dear_bindings/issues/9#issuecomment-1741719366)

The required changes were quite unintrusive, and the new output seems to resolve all the issues I had.

Remaining things to do:

[x] Add the new information to the JSON output
[x] Add some documentation

ShironekoBen commented 9 months ago

Just a very quick note to say thanks for the PR, and that I'm afraid it'll likely be the weekend before I get a chance to look at it properly, but as soon as I get a chance I'll give it a go!

ShironekoBen commented 9 months ago

I've spent a while playing with this and unfortunately I don't think the varargs mechanism works reliably in all situations. Basically, the problem is that the code is passing a ImGuiPrintableVarArg (a 64-bit structure), but assuming that it can be subsequently treated as though it was passed as a primitive of the appropriate type (since varargs uses whatever the standard calling convention is for arguments).

I think there are probably multiple situations in which this can go wrong - the most immediately obvious being that calling conventions generally expect floats/doubles to go into FPU registers. The x64 calling convention docs state that floating-point values go into XMM0 and so on whilst everything else goes into RCX/etc.

I wrote a little bit of code to test this out:

ImGui_Text("Float %f, %f", 1.0f, 2.0f);
ImGuiPrintableVarArg testArg;
testArg.val_double = 1.0f;
ImGuiPrintableVarArg testArgB;
testArgB.val_double = 2.0f;
ImGui_TextVA_2("Float test %f, %f", testArg, testArgB);
testArg.val_int32 = 123;
testArgB.val_int32 = 456;
ImGui_TextVA_2("Int test %d, %d", testArg, testArgB);

Interestingly, on MSVC/Windows/x64 this actually works correctly:

It appears that this is because of a compatibility feature - the calling convention notes say:

For floating-point values only, both the integer register and the floating-point register must contain the value, in case the callee expects the value in the integer registers.

...and it seems that in this case sprintf() is using the latter.

Looking at the code, the first case (calling ImGui_Text() directly) does this, setting both sets of registers:

00007FF79FC3AB9A  movsd       xmm2,mmword ptr [__real@4014000000000000 (07FF79FC4B8B8h)]  
00007FF79FC3ABA2  movq        r8,xmm2  
00007FF79FC3ABA7  movsd       xmm1,mmword ptr [__real@4010000000000000 (07FF79FC4B890h)]  
00007FF79FC3ABAF  movq        rdx,xmm1  
00007FF79FC3ABB4  lea         rcx,[string "Float test %f, %f" (07FF79FC9AE40h)]  
00007FF79FC3ABBB  call        ImGui_Text (07FF79FADFC08h)

Whilst the relevant part of ImGui_TextVA_2 does this:

00007FF6AC2D4400  mov         r8,qword ptr [arg2]  
00007FF6AC2D4407  mov         rdx,qword ptr [arg1]  
00007FF6AC2D440E  mov         rcx,qword ptr [fmt]  
00007FF6AC2D4415  call        ImGui::Text (07FF6AC180D6Fh)

...and is therefore only loading the integer registers, not the SSE ones.

On Clang/OSX/x64, on the other hand, printf() clearly uses the floating-point registers, because the same code gives this result:

It looks like this is possibly a Clang/LLVM vs MSVC difference, as you can see in this compiler explorer test - MSVC clearly generates code to set both sets of registers:

movsd   xmm2, QWORD PTR __real@4000000000000000
movq    r8, xmm2
movsd   xmm1, QWORD PTR __real@3ff0000000000000
movq    rdx, xmm1
lea     rcx, OFFSET FLAT:$SG5223
call    printf

...whilst Clang is happy to just set XMM1 and XMM2 and be on its way:

movsd   xmm0, qword ptr [rip + .LCPI0_0] # xmm0 = mem[0],zero
movsd   xmm1, qword ptr [rip + .LCPI0_1] # xmm1 = mem[0],zero
mov     al, 2
call    printf@PLT

So as it stands it doesn't look like this will work in the general case, I'm afraid.

I'm actually having a hard time coming up with plausible workarounds right now, too - even if the thunk code in ImGui_TextVA_2() and friends parsed the format string and understood what types it was dealing with, there isn't an easy way I can think of right now to put that data into the right places for the call to ImGui::Text() without the binding generator needing to have an understanding of the target ABI and emitting intrinsics or something to manually set the registers/etc (or, well, generating a thunk for every possible combination of parameter types, but that's going to generate a huge amount of code).

I'll keep pondering it in case something comes to mind, but any ideas on your side?

ZimM-LostPolygon commented 9 months ago

That's extremely unfortunate, but in hindsight, pretty expected, since we are completely in undefined behavior land. I can't think of any other reasonable approach, though...

Unless, maybe generating a thunk for every possible permutations isn't that terrible of an idea? If we limit ourselves to just int64 and double (so that int32 and float are upcasted during the call, and a void* is at most an int64 anyway), then, for 3 arguments, we'll have pretty reasonable 8 permutations:

double,double,double
double,double,int64
double,int64,double
double,int64,int64
int64,double,double
int64,double,int64
int64,int64,double
int64,int64,int64

4+ arguments would be impractical for sure.

Other than that, I guess we can always abandon the idea of supporting varargs and rely solely on the unformatted variant, with the formatting done by the caller. There really isn't that much of a need for varargs, it was more of a nice to have. The unformatted variants effectively achieve similar results.

zaafar commented 8 months ago

limiting the argument to 3 is not a great approach IMO since now user have to remember to use 1 method if arguments are less than 4 and another if they are equal or above 4. Also, argument type won’t match so that would be another issue user have to deal with.

At this point in time wouldn’t “generateunformattedfunctions” be enough?

ZimM-LostPolygon commented 8 months ago

Yeah, as I said, the unformatted variants are enough. Varargs support for languages without varargs would have been a nice option, but it's not worth the effort or the ugliness. I'll remove it.

ShironekoBen commented 8 months ago

Yeah, sadly I think that's the only practical way forward... I've been thinking about this more today but the only other "solution" I could come up with was basically to reimplement sprintf() in the wrapper layer, which would be both annoying to do and likely lead to all sorts of weird discrepancies in behaviour between the versions.

ZimM-LostPolygon commented 8 months ago

Removed varargs supports... Seems like changes are pretty minimal now.

ShironekoBen commented 8 months ago

Yeah, this looks pretty good to me now. If you don't think there's anything else you want to add then shall I go ahead and merge it?

ZimM-LostPolygon commented 8 months ago

Yup, feel free to merge!

ZimM-LostPolygon commented 8 months ago

Ah, one thing that just popped into my mind - what about ImStr support, do we need an unformatted variant of that? I'm a bit puzzled on the entire ImStr thing in general, since I couldn't find any real examples of what it is and how it is used in practice, so couldn't test it either.

ShironekoBen commented 8 months ago

ImStr is interesting because I think potentially it may result in format-string-less functions appearing in the main ImGui API. On a binding level since the basic idea is to allow languages that support string slicing to pass in partial strings without having the overhead of creating a new one (string_view in C++), it's kinda one of those features that is either really useful (if your target language allows that) or totally useless (if it doesn't!).

On that front, though, I suspect that it's likely to be mostly orthogonal to the stuff in this PR - my understanding is that there are relatively few situations in which C-style format strings and string_view style mechanics overlap, just due to the lineages of the features involved and their support. In broader terms though, it's probably worth taking a poke at support to get stuff working with the ImStr branch at some point, I think.

ShironekoBen commented 8 months ago

I've merged this in to mainline - thanks! I'll add a note to the changelog as well, but just to check - are you OK with me crediting your username there?

ZimM-LostPolygon commented 8 months ago

On that front, though, I suspect that it's likely to be mostly orthogonal to the stuff in this PR - my understanding is that there are relatively few situations in which C-style format strings and string_view style mechanics overlap

For the unformatted functions though, it does makes sense to have string slicing support though, same as for any other non format string supporting function. It would sure be useful for C#!

are you OK with me crediting your username there?

Of course!

dearimgui / dear_bindings

Variadic functions improvements / unformatted functions #46