Re: [libmicrohttpd] 0.9.62 doesn't build on Visual Studio

[Christian Grothoff]

On 12/10/18 7:14 PM, Jonathan McDougall wrote:
> On 12/10/18, Tim Rühsen <address@hidden> wrote:
>> On 12/10/18 2:27 PM, Christian Grothoff wrote:
>>> On 12/10/18 9:53 AM, Tim Rühsen wrote:
>>>> instead of keeping all projects at C89, you can also use Visual
>>>> Studio with LLVM/Clang or GCC
> 
> 0.9.62 does build with the clang toolset on Visual Studio after some
> changes to the project file. It's not something most users will have
> installed though.
> 
> Note also that VLAs are an _optional_ feature of C11 (see 6.7.6.2/4 and
> 6.10.8.3).
> 
>>> Well, we do know the max for all of the VLAs, so it would be trivial
>>> to change the code to use that value instead.  However, for the sake
>>> of keeping the stack shallow on embedded systems, I would prefer to
>>> do this conditionally only if the C compiler doesn't support VLAs.
> 
> Just how many bytes are we talking about? VLAs aren't free either, and
> the code generated to create the array might just be bigger than the
> number of bytes you save on allocation, as well as being generally less
> efficient than a simple array of constant size.

Sizeof SHA-256 * 4 in the larger case, sizeof SHA-1 * 4 in the smaller
case.  So ~128 bytes.  Besides, code size can be in ROM, stack is RAM.
Difficult to say which one is more precious ;-).

>>> Does the W32-crowd know whether AC_C_VARARRAYS and the resulting
>>> __STDC_NO_VLA__ works nicely with Visual Studio?
> 
> __STDC_NO_VLA__ is undefined in Visual Studio, regardless of the toolset
> selected. As for AC_C_VARARRAYS, the configure script isn't used on
> Windows at all. I guess __STDC_NO_VLA__ could be conditionally defined
> in w32/common/MHD_config.h.

Ah. So putting AC_C_VARARRAYS wouldn't help on W32/VS. That's exactly
what I wanted to know. Sure, we should still put that into configure.ac
for other platforms. but for W32 we need to define it in the
w32/common/MHD_config.h (as you suggest). Great.

>>> If so, we could
>>>
>>> #if __STDC_NO_VLA__
>>> #define VLA_LEN_DIGEST(n) (32*2+1)
>>> #else
>>> #define VLA_LEN_DIGEST(n) (n)
>>> #endif
> 
> Well, at this point I guess it becomes a matter of taste, but I'd be
> really uncomfortable with having such a difference in code between two
> platforms, especially since Windows is not exactly tested very often. If
> you go that route, at least put a check for `n < 32*2+1` to avoid buffer
> overflows.

Sounds reasonable.

>> You could also add a configure check for alloca and use that function
>> in case __STDC_NO_VLA__ is defined.
> 
> alloca() is not part of any standard, whether it's C, C++ or POSIX.
> Visual Studio had an _alloca(), but it's deprecated. It has a weird
> _malloca() that requires a weirder _freea().
> 
> alloca() also has a bunch of problems, like interfering with inlining
> on gcc and undefined behaviour on failure. Basically every single
> manpage I could find discourages its use.
> 
> I would personally go for constant size arrays, or malloc() if you
> can't. Keep it simple and portable.

Well, I don't want to go for malloc() as malloc() can fail and some of
those functions right now can _not_ fail. So malloc() would require
changing the signatures and clients to add extra error handling.

I agree on the alloca()-avoidance.

I'll see when I can find the time to fix this as discussed...

Thanks for the feedback!

Happy hacking!

Christian

signature.asc
Description: OpenPGP digital signature