Variadic function argument lists

[jayrm]

This update adds support for variadic function argument lists for all targets by adding the following:

• cva_list typedef (and __va_list_tag struct on 64-bit linux)
• cva_start( args, param )
• result = cva_arg( args, datatype )
• cva_end( args )
• cva_copy( dst, src )
• type id as datatype alias "modifier"
• tested on win32, win64, linux32 linux64

cva_list is a typedef symbol, and cva_start, cva_arg, cva_end, cva_copy are added as keywords. #undef can be used to remove any of these from the global namespace.

the alias "modifier" syntax was added to specify in fbc source code how data types are to be handled/emitted in the backends, in particular name mangling, and mapping to __builtin_va_list for gcc.

To implement the the cva_* forms:

• added new AST_NODECLASS_MACRO, and new AST_OP code for each cva_* macro
• added new EXPRCLASS_MACRO to C emitter
• added FB_DATATYPE_VA_LIST data type to handle the mappings to va_list types in the backends, though it is not exposed directly as a new datatype in AST/PARSER
• no changes to LLVM emitter, not tested.
• added quirk parsing for cva_* macros.

Experimental:

• added -z valist-as-ptr command line option. The cva_* expressions emitted for 32-bit gas backend should also work in 32-bit gcc backend. This command line option causes the expressions to be emitted even if backend is gcc.

Known issue:

• __builtin_va_list is typed as an struct array in gcc on linux 64bit, so to match mangling in c++ need to mangle in the "A1_" array type to the name.

Overall, I think it's a good starting point for varargs: more testing needed, of course. All the trivial types should work, though need to pay close attention to promotions and alignments.

( ref: sf.net bug https://sourceforge.net/p/fbc/bugs/881/ )

I'll leave it to countingpine to comment on #dump & #odump pp statements. The debug information available from fbc is handy. I call the dump procs directly from gdb. But, having something builtin to fbc might let just about anyone investigate code that appears to be buggy.

[dkl]

Some big changes, good job. It's also how I thought cva_* should be implemented. I left some comments here and there; only the cva_arg() behaviour isn't entirely clear to me yet.

[dkl]

(unused var?)

[jayrm]

Yes, this will be removed.

[dkl]

(probably meant cva_end() instead of cva_start() in this comment)

[dkl]

(probably meant cva_copy() instead of cva_start() in this comment)

[dkl]

(It seems that the return value isn't used, so it's better to remove it to avoid confusion)

[jayrm]

Yes, since the ASTNODE passed is modified, will make this a sub procedure.

[dkl]

(maybe should do some error recovery in this case, at least delete expr)

[jayrm]

Looks like I was all over the place with the error handling. Error handling will be cleaned up through in next update.

[dkl]

(should delete expr1 in the error case here)

[dkl]

(should delete expr1 in the error case here)

[dkl]

(should delete expr1 + expr2 in the error case here)

[dkl]

Similar case for the ASSIGN here - cVarOrDeref() should be used to ensure expr1 is an lvalue

[jayrm]

Going to use CVarOrDeref thoughout.

[dkl]

Is it the same for all targets? (e.g. ARM?)

[jayrm]

I see, I didn't pay any attention to ARM. This should be a ANY alias "char" ptr for ARM 32-bit bit so the selection of builtin type needs to check the arch as well. My tests have been on x86 & x86_64 only for win/lin.

[jayrm]

This last update should address almost all the comments:

• cleaned up the error handling in parser-quirk-vafirst.bas
• added __va_list structure for AARCH64
• added check for ARM 32|64 bit targets and map the correct (I think) type
• added fbGetBackendValistType() to return the FB_CVA_LIST_TYPEDEF enum. Code for this was duplicated in 2 places.
• changed fbUseGccValistBuiltins(), it just checks the return of fbGetBackendValistType()

The cva_arg() behaviour:

For __builtin_va_arg in gcc, we just need to give it something that is compatible. So far, I have just been trying to give it appropriate types. For example in astLoadMACRO() there is some code to promote types. Kind of like what has to be done with astNewARG()/hCheckParam()/hCheckVarargParam(). Maybe this promotion can be shifted to the parser or emitter. Without promotion can get this in gcc emitted code:

char a = __builtin_va_arg( x, char );
...
a5.c: In function 'p1':
a5.c:8:32: warning: 'char' is promoted to 'int' when passed through '...'
  char a = __builtin_va_arg( x, char );
		   ~~~~~~~~~~~~~~~~~~~~~^~~~~~
a5.c:8:32: note: (so you should pass 'int' not 'char' to 'va_arg')
a5.c:8:32: note: if this code is reached, the program will abort

For the cva_* pointer expressions, I am basing the expressions from these plain old 32-bit (or lower) C defines:

#define __va_argsiz(t) (((sizeof(t) + sizeof(int) - 1) / sizeof(int)) * sizeof(int))
#define va_start(ap, param) ((ap) = ((va_list) (&param) + __va_argsiz(param)))
#define va_end(ap)	((void)0)
#define va_arg(ap, t) (((ap) = (ap) + __va_argsiz(t)), *((t*) (void*) ((ap) - __va_argsiz(t))))
#define va_copy(ap, src) ((dest) = (src))

So for va_arg(ap, t), it is implemented as an in-line assignment, followed by an expression. Simplified without all the stack aligning, assuming ap is a pointer type:

• given result := va_arg( ap, datatype )
• we want to return result of *(datatype*)ap
• we also want advance ap += sizeof(datatype), an intentional side effect of using va_arg()

Now if ap itself has side-effects, I don't know, should that kind of expression even be allowed? Honestly, I only ever used va_list types directly with va_* macros in C and have not needed to do anything more complicated than that. So maybe just need to check for side effects and disallow.

I tried a couple of tests, trying to come up with something that might make sense. Looks like other problems to fix:

[jayrm]

Well, I missed something, or there's a regression and I didn't have a test written.

On linux x86_64, the cva_list parameter type emitted is not correct when using an extern "C" function like vsprintf, it's a mismatch. Missing a pointer level indirection .. or something.

Will probably take a few days to sort it out.

[jayrm]

Made some progress, linux x86-64 still giving me some trouble with passing cva_list types as paramaters of varying levels of indirection, so I have not added those tests yet.

Otherwise, this last update:

dtype information was being discarded too early, so pointer levels didn't match

• hSolveValistType(): now using typeJoinDtOnly()
• symbGetRealParamDtype(), symbGetRealType(): now using symbGetFullType()
• cva_list type should get fully replaced by __builtin_va_list in backend, casts, parameters, variables, etc

mapping cva_list type in fbc to __builtin_va_list in gcc, (any ptr, fbc to struct{}[1], etc)

• hEmitType(): deref type to convert "any ptr" to "__builtin_va_list" (when a pointer type)
• exprNewVREG(): don't deref cva_list types, gcc will compain about the casts when used with _builtin_va* macros
• astNewMEM(): __builtin_memset doesn't like __builtin_va_list array type, force a cast to (void*)

stuff I couldn't fix, so I disabled:

• disallow using arguments with side effects with cva_*() macros (the dtype mangle modifer on the vreg gets lost somewhere when assigning the expression to a temporary)
• dissallow functions returning cva_list type byval or byref, could not get it to work on all platforms, using the results directly in a cva_* cause gcc warnings

stuff I tested, but haven't added tests

• call crt functions like vsprintf() seem to work

new problems:

• looks like va_list on AARCH64 is a struct only. But I have not tested.
• for checks, need a better (or more robust) way to quickly determine that a data type is a va_list type. the checks are made often, but currently only from a few places
• testing the va_list sometimes depends on what the default for the target/platform is.
• fbGetBackendValistType() should only be called to determine the default type, currently it's being used in a way I had not intended
• problem is if looking the UDT symbol only, can't tell if is supposed to map to a struct or a struct array
• the special checks for the va_list type are spread out and some code is duplicated

Some possible solutions:

• when declaring the cva_list type in fbc, examine the name/alias and save a symbstat or udt option for fast checking/choice
• symbstat: FB_SYMBSTATS_RTL_CONST is now available and could be renamed to indicate a va_list type
• thought maybe symbGetRealParamDtype(), symbGetRealType() could do the __builtin_va_list type substitution, just have to be careful it's not too early

[jayrm]

Last update cleans up some special cases but not all:

• fbGetBackendValistType() is now only called to determine the "cva_list" type to add by default for a given target
• only symbGetValistType() determines the va_list mapping and type from a dtype/symbol
• the helper defines symbIsBuiltinVaListType() & symbIsValistStructArray() call only symbGetValistType()

Remaining Hacks:

• to determine if cva_list is a struct or struct array, the id_alias name is checked, two options I can think of:
• maybe allow alias "__builtin_va_list[]" on the cva_list typepdef to indicate array struct. Assumes that UDT has extra attribs that can be set, and that refering to cva_list implies if the struct is an array type or not.
• or allow "* 1" fixed length array specifier, though I don't know how much work to have FBSYMBOL handle this in a generic and reusable way.

Remaining Work:

• functions returning struct array "cva_list" either byval or byref
• side effects in cva_arg() (to allow functions returning cva_list type to be used)
• if a symbol is typed by referring to "cva_list" typedef, set an attrib in the UDT to mark it as a va_list type

[jayrm]

I think this change could be usable, but also please read following:

The big change in a57b82a is that the full dtype is passed to the IR. I was struggling with remapping the types until this change. This change now allows IR-HLC to make more decisions about the symbol types. For IR-TAC, since it doesn't know what to do with the extra bits, they get filtered out after getting passed to EMIT.

• added ALIAS "__builtin_va_list[]" to indicate that the backend needs to handle the data type as a C array.
• Side effects with cva_arg() pointer expressions now handled, by making a temp variable copy of the expression
• In a695f8f , UDT options now have bits to indicate that the struct is the "__builtin_va_list" type. This is done by cloning the original struct and setting the modifiers. Had to increase the field from short to long

IR-HLC does a good job now of mapping fbc's cva_list tyepdef to __builtin_va_list. But there are 2 defects, I am thinking that since we are exposing gcc's _builtin_va* almost directly, we should follow what gcc expects and document the differences. I'm not sure it makes sense doing something else, since we would be introducing behaviour, that while it might appear to work, I don't think C spec guarantees that it must.

1. passing cva_list by value as a parameter on linux x86_64:. After being mapped to __builtin_va_list in gcc, argument passing semantics in gcc actually pass the array as a pointer. The pointer is passed by value but the array data is not. It's kind of like the old byval as string problem in fbc. If we are calling an external function, this is expected and we probably shouldn't make a temporary copy to pass to the function.
2. function returning cva_list (aka __builtin_va_list) by value on linux x86_64: C doesn't allow returning arrays directly so will get gcc throwing compile errors. We could return a struct instead, but gcc will generate warnings (error?) about casting to array types if used in an expression, so we probably have to do a byte-by-byte copy to a temp variable. We could maybe pass the function return in a hidden parameter.

[rversteegen]

(Firstly this is very cool, I'm happy you're working on this! Lack of varargs support in the gcc backend has been annoying)

Forgive my ignorance (I haven't read this PR in detail), but why do you want to support returning a va_list from a function? Because va_list can be an array it isn't allowed to return it by value in C anyway, as you said. But even if you could (depending on the platform), it seems the only use for that would to return a va_list from a function that was passed to it in the first place. (Also the man page states "Each invocation of va_copy() must be matched by a corresponding invocation of va_end() in the same function", so it can't even be a copy of a va_list that was passed in, it must be the same one.) Which appears to make returning a va_list rather useless, even though you may be free to allow things in FB that C doesn't.

BTW, I found this interesting reference to part of the C standard about passing va_lists by value vs by reference:
https://stackoverflow.com/a/3369762/1185152

[jayrm]

@rversteegen, thanks, I've probably read that SO question before, along with many others. See also https://stackoverflow.com/questions/10648337/

Yeah, I'm convinced that it should be an error in fbc too, at least for linux x86_64, or any platform using va_list typed as an array.

Practically, if we want to help users write platform independent code, and because of how the va_* API is specified for C, va_list should never be returned by value as a function return. It's only because of the underlying implementation on some platforms (win/lin 32bit & win 64bit) that I know it will work returned by value.

True, va_start|copy & va_end must be in same function, but I can't find any specific restriction on va_arg(). It should be expected: pass a va_list to a function, use va_arg() on it and pass back a modified va_list. It seems reasonable that va_list could be returned through a function return using byref or ptr on all platforms.

[rversteegen]

Well, according to the quote from the C standard in the SO answer I linked to: you can pass a va_list either by value or by reference to a function and use it inside that function (pass it to va_arg). If you passed it by value, you can't use the va_list anymore in the calling function (you must call va_end on it), while if you passed a pointer to it, you can continue to use it (call va_arg on it) in the calling function.
So sure, you could pass a pointer to va_list and pass back the same pointer, but the only conceivable use for that is to allow chaining of multiple function calls. I guess that is a real use, especially when macro madness is involved.

I'd suggest making returning cva_list (by value) an error on all platforms, not just those where its required.

Also, I know that you skipped llvm support, but it's interesting to see what llvm does.
https://llvm.org/docs/LangRef.html#int-varargs
In llvm, va_arg is actually a builtin instruction, while va_start, etc are intrinsic functions. The semantics don't seem to be described in detail, but it all seems to be practically identical to C. In particular, the structure of va_list is identical (as I suppose it must be). So there should be no problems implementing cva_list in future as long as you stick to what's allowed in C.

[dkl]

Looks good to me. This is way more difficult than I thought.

[dkl]

looks like exit select in the wrong place

[jayrm]

The first exit select is actually correct. The code immediately following was something I was trying to do but it didn't work: Here's what it should look like, with comments:

case FB_CVA_LIST_BUILTIN_C_STD
	if( astIsVAR( n ) ) then
		if( symbIsParamByval( astGetSymbol( n ) ) ) then

			'' The dtype & subtype tell us that this is a
			'' C struct array.  Only the pointer to the
			'' array is acutally passed by value.  Taking
			'' the address of this symbol actually gives
			'' us the address of the passed-by-value-pointer
			'' to the the array instead and we don't want that.
			'' We also can't, deref the symbol directly, 
			'' because C won't allow deref on an array.
			''					
			'' Give up and let gcc backend use the param var
			'' symbol name as-is.  The mangle modifier will
			'' let C backend know it's the va_list type, 
			'' and exprNewVREG() won't try to deref it.

			exit select
		end if
	end if

	'' for anything else, replace the dtype here and let backend
	'' handle it (though there are some casts in exprNewVREG that
	'' could be solved out, TODO).  Taking the address of array
	'' variable (in C) is the same address as just referencing
	'' the array name.  e.g. "int a[10]", a = &a = &(a[0]), but
	'' different pointer types

	'' cast( __builtin_va_list, expr )
	n->dtype = typeJoinDtOnly( n->dtype, FB_DATATYPE_VA_LIST )

In theory should work for any variable and skip any casting, not just byval param variables.

[dkl]

maybe astRemSideFx() can be re-used to build the assignment/temp var

[jayrm]

Yeah, I saw astRemSideFx(), astMakeRef, hGetExprRef, and this new code for cva_arg() all kind doing similar things. I'll have another look.

[dkl]

What's the purpose of the cloning here? I couldn't find anything still using the original symbol, so it seemed unused. In cMangleModifier() it makes more sense to me; it probably has to avoid marking the original type with the FB_UDTOPT_ISVALISTSTRUCT* flags because otherwise it would be emitted incorrectly.

[jayrm]

I cloned the symbol there so:

1. that the symbKeywordTypeInit() was doing exactly the same thing as if the typedefs were declared in an include file, and
2. that it was possible to refer to the original symbol '__va_list_tag.' and not have it map to the __builtin_va_list type.

I have just made a few tests, and see that this cloning doesn't work so great. I tried an earlier version where cMangleModifier back patched the original symbol, but that didn't seem right.

So a few more changes yet to come.

[jayrm]

Last update is coming, hopefully can bring this to a close this weekend:

• will remove the symbCloneSymbol() for the va_list structures. fbc not quite ready for this, and the amount of work is huge compared to the immediate gain. Using the mangle modifier on these types will simply back patch the original symbol. If properly documented, this is a fair compromise for getting the va_list type working.

• looking at astRemSideFx() to see if can make this more generic.

Following is a summary of what we've done here:

The dtype/subtype (FB_DATATYPE/FBSYMBOL) pair generally describes the data type for language elements to the AST. Many data types, like integer, pointers, floats, do not require a subtype, and the AST understands this. The current design is that the dtype/subtype pair is solved early in the translation of fbc source code to emitted code, for any given backend. This early resolution of types gives fbc some gains on speed when processing data and makes the implementation of the AST simpler. On the other hand, as a result, because fbc does not have a full "typedef" data type, we sacrifice some flexibility.

The mangle modifiers and FB_DATATYPE_VA_LIST data type implement a pseudo-typedef. The dtype/subtype pair is still simple enough that the current AST can operate, because the va_list type, on the AST side is based on types already available in fbc. The PARSER & AST can work, and a full solution for the va_list type is delayed until the backend. Because some dtypes might have a NULL subtype, we have to attach the mangle modifier to the dtype field itself to have it preserved. dtype might also have a subtype associated (as in STRUCT) in which case we have to attach the information to the struct subtype as well. In future, to expand the feature to a full typedef system, we likely want to have all dtypes to optionally have a subtype, just as a place to attached the extra information. But that's beyond what we're trying to accomplish here, and would require lots of work in PARSER & AST to accomodate.

What we have, is a VA_LIST-like type in fbc source that is based on data types fbc already knows about, so we can compute sizes and offsets, which the AST needs to know about. And we have enough information attached, that the backend can substitute it for a different type (__builtin_va_list) in the gcc backend.

For me, gcc linux x86_64 is the "difficult" one, compared to the other backend va_list types, since in gcc va_list is typed as a struct array. fbc doesn't have a data type for fixed length struct array. I am suggesting documenting the differences; fbc's PARSER/AST does not give exact translation to the copy/assign semantics of the va_list type in gcc. The main difference is when assigning by value or passing arguments by value of a struct array type. If we really want to preserve the assign or pass by value, can either refer to the array element directly on the backend, for example __builtin_va_list args1, args2; args1[0] = args2[0];, or create a wrapper on either front end or backend struct struct va_list_wrapper{ __builtin_va_list args; };. In either case though, we probably end up using the va_list type in a way that is not supported by the implementation (C standard), so it doesn't really matter if the semantics are exactly preserved in the translation.

Bottom line is, there is a lot more we could do with the va_list data type for consistency between front and back ends, but if it is documented properly, it won't matter, because there is a fairly specific way that cva_list, cva_arg, etc, can be used according to gcc and C standard.

For the -gen llvm backend, I need help from someone that knows more. In total I have probably only given about 50 hours in last year to making llvm back end work. Mostly trying to find a version of llvm-as that works with fbc. From what I have read, sounds like binary bitcode representation is backwards compatible, but the text representation (llvm-as) is not. I need help from someone that knows more about llvm than I do to get a starting point; choosing a specific minimum llvm version to target. Believe me, I have tried a dozen combinations of llvm version and platform, and currently I am of the opinion that our llvm emitter is out of date.

I think we have an immediate solution for va_list type, with llvm effort to be continued later, and we have some options for future development that won't break user code. Not perfect, but probably good enough to merge in.

Updates are coming...

[jayrm]

I've add or updated a number of comments that should explain how most of this works within the compiler source code. I think this is as far as I can go with this feature for now. To gain a little speed or flexibility in the compiler, next steps would be to look at reworking FBSYMBOL's attribs, but that's more in depth than I am prepared to go just now.

It's possible the astRemSideFx() and friends could be combined, or more cases added to handle more scenarios. There are a few bugs on sf.net related to side effect handling, so I expect it will get revisited when those bugs are looked at.

We should see the C variadic function feature immediately usable on all 32-bit targets, win64, linux-x86_64. ARM64 might work but it needs to be tested. For any unix and *bsd targets, may need to adjust the logic that selects the default cva_list type. The work around should be only to #undef cva_list type and simply write the needed cva_list typedef in fbc source code. When testing specific targets, may need to adjust specific tests based on results.

I think this pull request is ready to merge in.

[jayrm]

While I was working on the documentation for this feature (a few new pages for the wiki, though I have not uploaded any of them to the wiki), I was also reviewing the changes made in this pull request and have a couple of reservations, but only because next thing I want to do is put out a 1.06 release. This change for varargs makes some aggressive internal changes to the compiler, especially when dealing with types, and leaves a few loose ends on the TODO list, so it might be wiser to make this the first merge after a release, which will be soon. On the other hand, if I get the builds working mostly automatic, I could probably deal with re-release if something went horribly wrong due to this feature.

[jayrm]

Apologies for the rebase and force push. There was just enough conflict from when this PR was started that it made sense to rebase for latest fbc 1.07.0

Wiki pages added:
cva_list
cva_arg
cva_start
cva_end
cva_copy