building ags4 compiler in gcc

[ericoporto]

So I am trying to make it possible to run the ags4 compiler as standalone.

The biggest issue I am having right now is actually being able to build it in GCC.

Edit: managed to fix a lot of issues and it actually builds now! I think I still may need to do some adjustments for clang and apple-clang in macOS.

[ericoporto]

Hey there's an error I am getting that isn't happening in the CI somehow - I am using Ubuntu 16.04 to use a compiler closer to the one we are using in debian. Anyway, I have a different compiler test causing SIGSEGV. If you have some time at some point later, please try building and running the tests on after building with GCC on Linux (or MinGW on Windows).

There is also a chance this compiler has issues with x64 builds in non MSVC.

[ivan-mogilko]

Which tests exactly cause sigsevs? There's this know problem, for instance: #2417, #2418

[ericoporto]

OK, I found what was causing. There was an error message for the modulo calculation that had a % sign that was supposed to be just a percent sign and not part of a format string, but no matter what it would eventually hit somewhere in the code that thought it should be a format string. I managed to fix by removing the percent sign altogether since escaping didn't help. Somehow this was not an issue in Windows.

There are places in the code that uses Long that are problematic. I don't know how to fix those, they are causing test errors. We should really never use long ever anywhere because of the way it behaves differently in different archs...

[ericoporto]

One thing that I won't do here, but it should be done at some point is to adjust the namespace use in the new compiler it is very inconsistent.

Also I think all the remaining errors are simply long usage. @ivan-mogilko could you take a look on what to replace all the remaining longs in the code on top of this branch? I will stop commiting because the remaining is just test errors for incorrect long usage. Edit: just did a minor fix for one last thing when building in macOS.

[ivan-mogilko]

There are places in the code that uses Long that are problematic. I don't know how to fix those, they are causing test errors. We should really never use long ever anywhere because of the way it behaves differently in different archs...

I can look tomorrow, but I'm pretty much certain that any long in compiler may be changed to int32, because ags bytecode is based on 32-bit signed ints.

[ericoporto]

I attempted to fix the long usage but it is still wrong (somewhere?) and the tests Bytecode1.LongMin1 and Compile0.EnumNegative are still failing. I will try again tomorrow.

[ivan-mogilko]

Unrelated to above, but when trying to build a x64 build in MSVS, compiler note this line:

sprintf(appendage, "^%d", _sym.FuncParamsCount(name_of_func) + 100 * _sym[name_of_func].FunctionD->IsVariadic);

the argument type here is size_t, but formatting uses %d, should be %zu, or this will break on 64-bit build.

[ivan-mogilko]

I might suggest to disable following tests temporarily:

• Bytecode1.Ternary5
• Compile0.Ternary02
• Compile1.CompileTimeConstant2

These tests are known to cause incorrect memory access (#2418), which hypothetically may lead to other glitches.

[ericoporto]

So I commented the said tests but I still am getting an error. I also adjusted the mentioned appendage section and the tests kept failing. I will look for more possible printf like errors.

Unfortunately there is a ton of narrowing conversion warnings all through the code... I noticed also the defines for the headers are the same as the old compiler.

[ericoporto]

The failing test Bytecode1.LongMin1 looks like this

https://github.com/adventuregamestudio/ags/blob/2bb36fe514f0a80f125bfc2db3da1d828e825787/Compiler/test2/cc_bytecode_test_1.cpp#L3296-L3310

I will try playing with agsdiss tomorrow to see if I find a light...

[ericoporto]

Btw, the pipelines that aren't failing in other branches aren't failing because of this PR, they just weren't building and running the tests at all before in the CI because they just weren't built in CMake at all. This is the first time these are being built using GCC and Clang. You can see the CMake test that is built using MSVC still makes all tests pass.

[ivan-mogilko]

There has to be a difference in how C++ compilers treat type conversions somewhere, same code works in 64-bit msvc, but results in integer overflow in others.

[ericoporto]

I found it! There was a LONG_MIN I forgot to replace! There is still ONE test that fails , the Scan.ConsecutiveStringLiterals1, but only fails in gcc and doesn't in Apple Clang builds.

[ivan-mogilko]

There is still ONE test that fails , the Scan.ConsecutiveStringLiterals1, but only fails in gcc and doesn't in Apple Clang builds.

Is there any way to know which exact assertion fails? There are 4 of them in this test, but logs only have a message that the test failed.

[ivan-mogilko]

Unrelated, but searching the logs for warnings in compiler tests, I found this:

/ags/Compiler/script2/cc_symboltable.h:481:126: warning: no return statement in function returning non-void [-Wreturn-type] inline Symbol *FindStructComponent(Symbol strct, Symbol component) const { FindStructComponent(strct, component, strct); }

Too good this function is not used anywhere... i thought this kind of mistake should be treated as error by compilers...

[ericoporto]

So I understand exactly why the test Scan.ConsecutiveStringLiterals1 is failing, I actually don't understand why it should work... the issue is this.

In the ReadInStringLit, this is the function that globs something "ABC" "DEF" into a single "ABCDEF" string literal.

When it hits the second " it will try to see if there is only whitespace followed by another string. But ags has a special string literal that is a line with a " __NEWSCRIPTSTART_scriptname";, and so it has a minimal code that does the disambiguation. Here is the code

https://github.com/adventuregamestudio/ags/blob/2bb36fe514f0a80f125bfc2db3da1d828e825787/Compiler/script2/cs_scanner.cpp#L602-L608

Now this code is using standard c++ stream code here, and notice the _eofReached here. In this test the "__NEWSCRIPTSTART_" prefix is longer than the remaining characters that are there ("expialidocious"). So what happens is _eofReached is set but once things are undone, it's not unset. Because it remains set, once the while loop loops, it breaks and throws the error message: Input ended within a string literal.

I don't understand why this is working in Windows or macOS.

[ivan-mogilko]

If I remember correctly, in standard lib eof() returns true only when a stream tries to read again when being past the last char. It is not yet set when it's read everything out first time. https://en.cppreference.com/w/cpp/io/basic_ios/eof

So it reads them all out, gets to the end of stream, but according to the standard eof flag should not be set just yet.

could be there's a mistake in particular version of gcc in regards to how it sets eof flag?

[ericoporto]

there's a mistake in particular version

I don't think it's a particular version because it reproduces in my old gcc 5.4, and in the new GCC on the Ubuntu 24.04 on CI and in the MinGW (also GCC) on the Windows CI - check the actions log here. So it happens with different gccs.

I guess these issues are the things we bypass when we use our own streams that are in ags. I am not sure how to fix this yet.

[ivan-mogilko]

Hmm, no, I seem to be wrong here, maybe I misunderstood certain things.

If I do this: _inputStream.seekg(-5, std::iostream::end); right before the _inputStream.get(tbuffer, kNewSectionLitPrefixSize + 1u, 0);

then it will always return eof == true.

[ivan-mogilko]

So, here's my proposal for a test:

pos_before_skip = _inputStream.tellg();
//<------------------------------------------------
_inputStream.seekg(0, std::iostream::end);
auto pos_end = _inputStream.tellg();
size_t has_count = pos_end - pos_before_skip;
_inputStream.seekg(pos_before_skip);
assert(has_count >= kNewSectionLitPrefixSize + 1u);
//<------------------------------------------------

This asserts that the stream contains enough chars to read "kNewSectionLitPrefixSize + 1u". This assertion never failed for me on MSVC, it always contained at least "kNewSectionLitPrefixSize + 1u" chars. What will happen on GCC?

EDIT: although I am not saying that the code is originally correct..... tbh I am confused by this situation, because string literals may be small. What does it do in that case?...

[ericoporto]

The assert didn't fail. Here is a print from that code copy pasted and ran in the CLion IDE (you can see the values, the test still failed after too)

Note we are reading using istream.get. Below is the header I have on my Ubuntu 16.04 computer.

```C++ /** * @brief Simple multiple-character extraction. * @param __s Pointer to an array. * @param __n Maximum number of characters to store in @a __s. * @param __delim A "stop" character. * @return *this * * Characters are extracted and stored into @a __s until one of the * following happens: * * - @c __n-1 characters are stored * - the input sequence reaches EOF * - the next character equals @a __delim, in which case the character * is not extracted * * If no characters are stored, failbit is set in the stream's error * state. * * In any case, a null character is stored into the next location in * the array. * * @note This function is not overloaded on signed char and * unsigned char. */ __istream_type& get(char_type* __s, streamsize __n, char_type __delim); ```

I need to sleep now and I will look at this tomorrow. Also thanks for the input on the other PR, those are easy to adjust I should be able to adjust that one tomorrow.

[ivan-mogilko]

Yes I had same values. I forgot to tell to test _eofReached after this though, it will be interesting if it sets eof in this case, or there's another reason it appears set.

On a side note, this line in the test must be safeguarded by checking if strings is not empty:

EXPECT_STREQ("Supercalifragilisticexpialidocious", &string_collector.strings[0] + value);

because if scanner fails for any reason, this leads to a index out of range exception.

[ericoporto]

Not super sure on the last commit but it makes all tests pass. I think this is enough for now - I think more tests are needed for this grouping of string tokens.

I thought about removing the _eofReached completely and let only a simple eof check exist but I don't quite get what is happening there entirely so I prefer to leave it in a way someone can find it weird enough to do a better fix...

This PR is NOT meant to fix the standalone compiler issue. Now that I can get it to build and play with it, I will rethink on how to make the existent front end to properly glue with it, but the existing placeholder code should at least be there to make it easier to run it. I want to work on a next PR that actually has both the proper code for the standalone compiler plus builds both it and the other one in the CI and saves their binaries.

[ivan-mogilko]

I think we should ask @fernewelten to check out this problem with eof; since it's his code, he might know better how is it intended to work.

[ericoporto]

@fernewelten can you check the mentioned eof() issue? I explain it here here and put a small fix in https://github.com/adventuregamestudio/ags/pull/2508/commits/3da2faee00986eb9c5f0dc9a1d3da7714830eb99 and https://github.com/adventuregamestudio/ags/pull/2508/commits/ff2eb6d8f27eee4b45cfa833802b6c5a774822d6 .

[ivan-mogilko]

I think I'll merge this now, as all the relevant tests pass, and I was able to recompile and run a large existing project after these changes too. The fix to the "eof" check is contained in a single place of code and is easy to see; it may be revisited later. I will add a small comment there explaining what was found.