News:

Use the "Forum Search"
It may help you to find anything in the forum ;).

Compile error when compiling with musl

Started by ceeac, June 30, 2019, 11:53:42 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ceeac

OS: Alpine Linux

When compiling Simutrans with musl as the standard library, compilation fails with the following error message:

utils/cbuffer_t.cc:362:8: error: array type 'va_list' (aka '__builtin_va_list') is not assignable


Replacing the offending code by a call to va_copy fixes the error, however va_copy requires C99 (i.e. C++11).

Ters

There is some va_copy things there before. I guess they dropped the implementation specific macro when proper standard support came along, or never needed it to begin with.

One might surround the use of ca_copy with som #if __STDC_VERSION__ + 0 >= 199900L || __cplusplus + 0 >= 201103L stuff, just like the use of __va_copy is surrounded by an #ifdef. Maybe something like:


#if __STDC_VERSION__ + 0 >= 199900L || __cplusplus + 0 >= 201103L
      va_copy(args, ap);
#elseif defined(__va_copy)
      __va_copy(args, ap);
#else
      // HACK: this is undefined behavior but should work ... hopefully ...
      args = ap;
#endif

DrSuperGood

#2
The fact that "undefined behavior" is being relied on is not good. If the behaviour is undefined that macro branch should be removed.

Is the va_copy necessary? Since the function and its parents do not appear to use ap except for being copied. Maybe cbuffer_t::vprintf could pass ap to my_vsnprintf directly, avoiding the need to copy it. One would have to specify that cbuffer_t::vprintf calls va_arg on ap from an API perspective, but that is pretty standard/expected behaviour anyway.

Also where is the va_end for args? Since the function calling va_copy must also call va_end on the destination before the destination is re-used or the function returns.

Ters

I believe it has to make a copy because it starts over from the start every time it loops. (And it will loop if the buffer isn't big enough for the result.)

No platform I know of has anything meaningful to do in va_end, unless there is some debugging tools that can do something magic there to detect overruns. It is not much trouble to put it there, though. cbuffer_t::printf has it.

DrSuperGood

#4
Quote from: Ters on July 01, 2019, 02:45:48 PMI believe it has to make a copy because it starts over from the start every time it loops. (And it will loop if the buffer isn't big enough for the result.)
Ah I did not notice the loop. Maybe the approach should be restructured to avoid a loop if C++11 requirement is not intended?
Quote from: Ters on July 01, 2019, 02:45:48 PMNo platform I know of has anything meaningful to do in va_end, unless there is some debugging tools that can do something magic there to detect overruns.
It is still required by the C++ specification. If it actually does anything with a compiler is down to implementation details, but ultimately for maximum portability the C++ specification must be respected. After all this is why this topic was created, as undefined behaviour was relied upon.
Quote from: Ters on July 01, 2019, 02:45:48 PMIt is not much trouble to put it there, though. cbuffer_t::printf has it.
The copy made by va_copy must have va_end applied to it before the local variable goes out of scope within the function which called va_copy. This is required by the C++ specification.

Ters

Quote from: DrSuperGood on July 01, 2019, 06:41:24 PMAh I did not notice the loop. Maybe the approach should be restructured to avoid a loop if C++11 requirement is not intended?
I don't really see how. Even if you do try to calculate the length of the formatted result before calling vsnprintf or similar, that too might require accessing elements of the va_list in some cases. The only other option might be to depend on implementation defined behavior, which seems safe for GCC. I don't have Microsoft's compiler installed at this computer at the moment, so I'm not sure how its va_list works.

It is however a bit odd that a C library implementation like musl provides stdarg.h, since this seems inherently tied to the compiler. That stdarg.h (if I googled up the right one) doesn't care what standard you aim for, you get va_copy nonetheless. GCC comes bundled with stdarg.h as part of its freestanding C implementation (which does care about which standard you follow). That appears to be the case for Microsoft's compiler as well, as it is not part of the UCRT headers like stdio.h is.

Dwachs

Yes, va_end is missing. I do not know the reason for introducing the call to __va_copy anymore. I will implement Ters' suggestiong to fix the va_copy compiling issue.
Parsley, sage, rosemary, and maggikraut.

DrSuperGood

Quote from: Ters on July 02, 2019, 06:06:02 AMI do not know the reason for introducing the call to __va_copy anymore.
It was a compiler specific macro which acted as va_copy did around the time va_copy was proposed for standardization but before it was standardized.

In theory the most correct solution would be to out right use va_copy. C++11 is around 8 years old at this point and the reason va_copy was made standard was to allow for situations such as this.

Ters

The standardization of va_copy actually dates back to 1999. It just took 12 years for C++ to catch up.

Dwachs

Parsley, sage, rosemary, and maggikraut.

prissi

Not really, now one cannot compile with MSVC, since "#warning" is not a known preprocessor directive (only #error is).

DrSuperGood

Can confirm that MSVC no longer builds. The reason is because only newer versions of 2017 and 2019 support increasing the __cplusplus value and that might not be done by default. It defaults to reporting 1997 conformance lol.

MSVC 2017 supports va_copy so I think the solution would be to simply add a MSVC version macro check as well. At least until we remove all the macro mess and require that compilers support va_copy to build Simutrans.

Ters

Quote from: DrSuperGood on July 15, 2019, 06:05:45 PMIt defaults to reporting 1997 conformance lol.
I was a bit curious whether __cplusplus really was guaranteed to be a date, since that bit just came from some other bit of code I found somewhere, but I never suspected that someone would define it as the wrong date. That compilers might use just a value of 1 or something seemed more likely to me.

DrSuperGood

#13
Quote from: Ters on July 15, 2019, 07:49:59 PMI was a bit curious whether __cplusplus really was guaranteed to be a date, since that bit just came from some other bit of code I found somewhere, but I never suspected that someone would define it as the wrong date. That compilers might use just a value of 1 or something seemed more likely to me.
The issue is that MSVC was not fully C++ compliant. They were for 1997 C++ but not for anything newer. Major efforts have been made to try and make it C++ complaint, especially for recent standards, but it still is not 100% so. Hence Microsoft are edging on the side of caution and not declaring by default their compiler is compliant to new standards of C++ by default. In some more recent versions of 2017 and in all versions of 2019 one can force the macro to a higher C++ standard version which more closely matches what is mostly implemented but there are still some areas where MSVC deviates from the C++ standard or does not implement the standard fully.

In any case modern MSVC does support va_copy so should be using that code branch but currently cannot because it fails the macro.

EDIT:
Upon close inspection of the C and C++ standards it is required that va_copy be a macro. It will never be a function. Hence one can test for the existence of va_copy by testing if the macro exists. This removes the need for testing the C/C++ standard version since if it exists then chances are it is from some part of that standard. This fixes building in modern MSVC versions where the macro does exist and so should correctly be used.

For older compilers which lack the macro I have removed the non-standard pre-processor directive and instead placed a comment in line with the assignment. If such a compiler encounters an error with the assignment hopefully it will print the entire line allowing the comment to be seen and hence act like the warning did to some extent.

At some stage in the future when Simutrans is raised to a minimum of C++11 conformance all those conditional macros should be removed as lacking va_copy would then be an error. For now that unspecified behaviour hack is required to try and support old or non-conformant compilers.