Finish porting parsers to windows

[uucidl]

This pull request is mainly deactivating or fixing warnings that MSVC would emit on the parser code.

The only significant change is to choice's regex constructor implementation, where the gotos array used to be allocated on the stack as a variable-length array. It is now heap allocated and freed before returning.

Let me know if anything needs to be changed.

In my next effort I will port the registry.c file which is the only remaining part of the library which is currently not building on windows (because it uses a unix only tree library)

[pesco]

The only significant change is to choice's regex constructor implementation, where the gotos array used to be allocated on the stack as a variable-length array. It is now heap allocated and freed before returning.

Unfortunately, that's not permissible since the RVM compilation uses longjmp for error handling. See for instance the h_rvm_insert_insn function. The jump will bypass any cleanup at the end of the function.

I'm trying to see why the change is necessary at all - didn't we have a working build on Windows before? Variable-length arrays are in C99.

[uucidl]

Alright, good that you jumped in. I'll have a closer look.

As for VLA Support: The MSVC compiler is not a C99 compiler. It's first a C++ compiler and only a C compiler as an afterthought. So it does not support (unless via a flag I'm not aware off) variable length arrays. Didn't they get removed from C11 in the end? Or at least made optional?

[uucidl]

As for hammer working previously on windows I do not know. I can say with reasonable certainty that it was certainly not compiling with MSVC and not with readily available libraries.

Maybe someone did a compilation on cygwin w/ gcc, I believe that could work although not really "native."

[abiggerhammer]

Hammer was definitely not compiling with MSVC before. I tried, and @uucidl picked it up and has gotten farther than I did.

[pesco]

As for hammer working previously on windows I do not know. I can say with reasonable certainty that it was certainly not compiling with MSVC and not with readily available libraries.

Then I'm confused - what has AppVeyor been building? E.g. https://ci.appveyor.com/project/abiggerhammer/hammer/build/1.0.61/job/7w4lovd5h8p45sdp

As for VLAs missing, that's a shame. The obvious alternative is alloca() - does it have that?

[uucidl]

@pesco I've introduced AppVeyor at the beginning of my porting effort, since I could not do everything in one go.

The idea is to have MSVC build against a known working set of source files in order to give feedback to contributors when they would break portability to windows.

Along the way when I can dedicate time to it I create PRs which add new files to the set of compiled files on Windows.

[uucidl]

@pesco MSVC has:

• _alloca https://msdn.microsoft.com/en-us/library/wb1s57t5.aspx
• _malloca https://msdn.microsoft.com/en-us/library/5471dc8s.aspx (this one however requires a pair _malloca/_freea)

I do not know if either of theses work fine with setjmp/longjmp (we should hope); but they are candidates

(I usually do my open source work on sundays)

[pesco]

Ah, I see; sorry for the confusion. _alloca looks like the right thing. In this case I would personally favor a preprocessor check for MSVC in that one function over the alternative of changing the code to something "portable" via some macro abstraction, but I guess it's a matter of taste.

[uucidl]

Hi, I've updated this PR and now use alloca on windows to allocate the array on the stack (choice.c) The travis build failed on two configurations.

I find it unlikely that's due to this patch, the error look like an environment problem. So maybe you can re-run it.

[uucidl]

Hi again. The build above is only failing in one run (gcc dotnet) and as far as I can see the main difference is the gcc version used. For the failing run it is gcc 4.8 and for the working runs on master it is gcc 4.6.

The error message says:

build/opt/src/backends/regex.c:209:75: error: argument 'trace' might be clobbered by 'longjmp' or 'vfork' [-Werror=clobbered]
 HParseResult *run_trace(HAllocator *mm__, HRVMProg *orig_prog, HRVMTrace *trace, const uint8_t *input, int len) {
                                                                           ^
cc1: all warnings being treated as errors

It's not even on a file I touched!

I don't see any serious problem with the code in backends/regex, I'm wondering if the warning is to be dealt with in any way.

[uucidl]

@pesco have a look at parsers/choice.c ; let me know if the solution I found for the VLA support is not to your liking.

[pesco]

This should be fine. I'll pass the ball to @abiggerhammer or @thequux to "sign off" and merge it. :)

For the failing run it is gcc 4.8 and for the working runs on master it is gcc 4.6. [longjmp clobber]

Eh, I've seen this before. I'm pretty sure the warning is bogus; it appears with GCC 4.8 only to disappear again in later versions.

[uucidl]

I've done some more work on this pull request. Now the hammer lib compiles in its entirety on windows, when using the tools\windows\build.bat script.

Examples that are portable (those that don't rely on posix) compile now.

So from the code point of view the port to windows is complete.

This PR is a firm proposal of mine for inclusion. The compilation failure above is again the setjmp/longjmp issue that we talked about earlier, which only shows up in the dotnet build, for some reason.

[abiggerhammer]

Many thanks! Apologies for taking so damn long to get this merged, too.

We're still seeing this compile problem with the dotnet bindings. I bumped the Travis-CI config to Ubuntu Trusty, and 4.8.4 is working fine (for those builds that pass; I should have gotten everything working before opening #163, mea culpa).

162 was reported yesterday and is the same issue, so we'll continue investigating it over there.