On 12/10/18 7:14 PM, Jonathan McDougall wrote: > On 12/10/18, Tim Rühsen <[email protected]> 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
