Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Variadic functions improvements / unformatted functions #46

Merged
merged 9 commits into from
Oct 16, 2023

Conversation

ZimM-LostPolygon
Copy link
Contributor

@ZimM-LostPolygon ZimM-LostPolygon commented Oct 1, 2023


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

Remaining things to do:

  • Add the new information to the JSON output
  • Add some documentation

… function split, useful for languages with default arg support

Added --generateexplodedvarargsfunctions option to generate variants of variadic functions with up to a number of explicit args, useful for languages without variadic function support
Added --generateunformattedfunctions option to generate unformatted variants of format string supporting functions
@ZimM-LostPolygon ZimM-LostPolygon changed the title Add .vscode to .gitignore Variadic functions improvements\ Oct 1, 2023
@ZimM-LostPolygon ZimM-LostPolygon changed the title Variadic functions improvements\ Variadic functions improvements Oct 1, 2023
@ZimM-LostPolygon ZimM-LostPolygon changed the title Variadic functions improvements Variadic functions improvements / unformatted functions Oct 1, 2023
@ShironekoBen
Copy link
Collaborator

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
Copy link
Collaborator

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:

image

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:

image

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
Copy link
Contributor Author

ZimM-LostPolygon commented Oct 7, 2023

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
Copy link
Contributor

zaafar commented Oct 8, 2023

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@ShironekoBen
Copy link
Collaborator

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
Copy link
Contributor Author

Yup, feel free to merge!

@ZimM-LostPolygon
Copy link
Contributor Author

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
Copy link
Collaborator

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 ShironekoBen merged commit bce4b58 into dearimgui:main Oct 16, 2023
@ShironekoBen
Copy link
Collaborator

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
Copy link
Contributor Author

ZimM-LostPolygon commented Oct 16, 2023

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!

@ZimM-LostPolygon ZimM-LostPolygon deleted the varargs-improvements branch August 26, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants