Am 20.03.2019 um 19:27 schrieb William F Pokorny:

> I can confirm the v38 issue. Also running Ubuntu 18.04 though g++7 vs 
> Warren's 8.
> 
> It seems the v3.8 branch (commit 74b3ebe) needs a space ahead of the 
> macro #end statement to work as follows:
> 
> #macro LoadThing(filePath)
>      #include filePath
>   #end                 // <-- Add extra space before #end and OK
> 
> LoadThing("zoomBangCrash.inc")
> 
> and whenever the include file has an empty line, C++ comment style line, 
> C++ comment line and then a declare line. The following for 
> zoomBangCrash.inc, less the line numbers, is also a v3.8 parse issue if 
> there is no space ahead of the main file macro #end:
> 
> 01
> 02 //
> 03 //
> 04 #declare A=1;

Can't reproduce on Windows; neither with DOS line endings (CR+LF) nor 
with Unix line endings (LF).

I'm running a slightly modified version, but if any of those 
modifications make any difference, I wouldn't know which ones.

Do you have the time and energy to investigate this further?

Post a reply to this message

On 3/21/19 3:18 PM, clipka wrote:
> Am 20.03.2019 um 19:27 schrieb William F Pokorny:
> 
...
> 
> Can't reproduce on Windows; neither with DOS line endings (CR+LF) nor 
> with Unix line endings (LF).
> 
> I'm running a slightly modified version, but if any of those 
> modifications make any difference, I wouldn't know which ones.
> 
> Do you have the time and energy to investigate this further?

Not today with a family visit later, but I'll put it on the list to play 
with the issue more this weekend.

Bill P.

Post a reply to this message

On 3/22/19 6:44 AM, William F Pokorny wrote:
> On 3/21/19 3:18 PM, clipka wrote:
>> Am 20.03.2019 um 19:27 schrieb William F Pokorny:
>>
> ...
>>
>> Can't reproduce on Windows; neither with DOS line endings (CR+LF) nor 
>> with Unix line endings (LF).
>>
>> I'm running a slightly modified version, but if any of those 
>> modifications make any difference, I wouldn't know which ones.
>>
>> Do you have the time and energy to investigate this further?
> 
> Not today with a family visit later, but I'll put it on the list to play 
> with the issue more this weekend.
> 
> 

That was painful to run down, but changing Parser::IsEndOfInvokedMacro() 
in the file source/parser/parser_tokenizer.cpp as in the attached file 
fixes the issue.

Have to admit I'm not sure even now I completely understand what was 
happening (how it could happen perhaps). We seem to have been comparing 
structures in the original - which I don't think c++ does by default?

return (Cond_Stack.back().PMac->endPosition ==
CurrentFilePosition()) && ...

If this true though, why no compile warning or error? In any case, once 
in a while on the structure compare some error got thrown and caught by 
the parser code and no message was set which is why we get the unhelpful 
generic something wrong message. Mostly calls to the compare above 
seemed to work - including ones prior to the failing one for the test 
case - returning false always.

I'm tempted to add a throw on a true test for the re-coded version then 
run a bunch of stuff to see if it true ever really happens. Have we been 
doing something like comparing pointers (and so false) always? But, I've 
already spent a day on this and I don't feel like spending additional 
time.

For the particular test cases you can also hard code that function to 
return false always and things work because that test is always false 
for it.

Lastly, I did try a debug -Og compile thinking maybe we had some 
compiler issue here, but it failed in the same way.

Anyone who better understands what happened / how direct struct compares 
could sort of seem to work, please feel free to jump in and explain.

Bill P.

Post a reply to this message

Attachments:
Download 'isendofinvokedmacro_update.txt' (1 KB)

Am 24.03.2019 um 15:59 schrieb William F Pokorny:

> That was painful to run down, but changing Parser::IsEndOfInvokedMacro() 
> in the file source/parser/parser_tokenizer.cpp as in the attached file 
> fixes the issue.

Well, that's a lead I might investigate. But as you don't seem to be 
confidently understanding what's going on, I'll be going with the 
presumption that this is not a proper fix but just a patch.

> Have to admit I'm not sure even now I completely understand what was 
> happening (how it could happen perhaps). We seem to have been comparing 
> structures in the original - which I don't think c++ does by default?
> 
> return (Cond_Stack.back().PMac->endPosition ==
> CurrentFilePosition()) && ...
> 
> If this true though, why no compile warning or error?

That's easily explained: The data is of type `LexemePosition`, which is 
a struct defining an overloaded comparison operator.

(In C++, structs and classes are the same category of beasts. The only 
semantic difference between `struct` and `class` is the default member 
and base class access: `struct` implies `public` by default, while 
`class` implies `private` by default. According to the language 
standard, it is even perfectly valid to pre-declare a type as `struct` 
and later define it as `class` or vice versa.)

The `LexemePosition` type holds a file offset, as well as a line and 
column number.

Comparison is done by file offset, but the code also features an 
assertion, which tests whether the comparison by line and column yields 
the same result. If the assertion fails, debug builds bomb (i.e. core 
dump or break into a debugger), and non-debug builds throw an exception 
to trigger a parse error.

(Usually such assertion tests are only enabled in debug builds, but in 
the parser they're currently deliberately enabled in all builds, because 
I don't fully trust my current understanding of the parser and the 
refactoring work I have based on that understanding.)

If `Cond_Stack.back().PMac->endPosition == CurrentFilePosition()` throws 
an exception, it means either of two things:

(A) The positions compared are from two different files, which means the 
calling code has failed to check whether it is even in the right file.

(B) The line/column tracking is buggy, and yields different positions in 
different situations.


> In any case, once 
> in a while on the structure compare some error got thrown and caught by 
> the parser code and no message was set which is why we get the unhelpful 
> generic something wrong message.

This is the default error message for failed assertions. Unfortunately 
somewhere along the chain of error message handling the location 
information for the assertion seems to be lost, the original exception 
should include the nformation that it was triggered in `parsertypes.h` 
line 62 (in my version of the file at any rate), maybe even that it was 
in function `LexemePosition::operator==` (depending on compiler). Maybe 
it's worth investigating and fixing that.

Post a reply to this message

Am 25.03.2019 um 23:55 schrieb clipka:

> If `Cond_Stack.back().PMac->endPosition == CurrentFilePosition()` throws 
> an exception, it means either of two things:
> 
> (A) The positions compared are from two different files, which means the 
> calling code has failed to check whether it is even in the right file.
> 
> (B) The line/column tracking is buggy, and yields different positions in 
> different situations.


Well, looking at the code of `IsEndOfInvokedMacro()` again, it 
_strongly_ reeks of scenario (A):

     return (Cond_Stack.back().PMac->endPosition ==
                 CurrentFilePosition()) &&
            (Cond_Stack.back().PMac->source.fileName ==
                 mTokenizer.GetInputStreamName());

should probably be swapped around, like so:

     return (Cond_Stack.back().PMac->source.fileName ==
                 mTokenizer.GetInputStreamName()) &&
            (Cond_Stack.back().PMac->endPosition ==
                 CurrentFilePosition());

so that the `LexemePosition` comparison operator is only invoked if the 
file names match.


I guess you got really lucky in your test (and the OP unlucky in their 
use case), in that you happen to have created the following scenario:

- A macro is invoked.
- That macro includes a different file.
- That included file happens to have a `#` (e.g. `#declare`) at the same 
file offset as the `#end` of the macro.
- However, the `#` happens to be in a different line and/or column.


In such a scenario, encountering the `#`, the parser would first test 
whether this is the `#end` of the macro it has been waiting for, calling 
`IsEndOfInvokedMacro()`.

`IsEndOfInvokedMacro()` in turn would checks whether the current file 
position matches that of the macro's `#end` by calling 
`LexemePostition::operator==`.

`LexemePostition::operator==` in turn would find that the file position 
does match. It would then verify that the match also reflects in line 
and column, and find that they differ, and report this inconsistency via 
an Exception.

`IsEndOfInvokedMacro()` wouldn't even get a chance to test whether it is 
looking at the right file, because its author (duh, I wonder who that 
stupid git might have been...) chose to do the two tests in the wrong order.


Now aside from the stupid git who authored `IsEndOfInvokedMacro()` 
there's someone else to blame for the situation, namely whoever thought 
it was a good idea to keep the stack of include files separate from the 
stack of pending "conditions". If they were using one and the same 
structure, having `INVOKING_MACRO_COND` on top of the stack would imply 
that we're in the file with the `#end`, and the file name test would be 
redundant.

I think it should be possible to merge the include stack into the 
"condition" stack without adverse effects, but I haven't finished work 
on that yet.


I should note that I haven't tested this hypothesis and fix for the 
error yet; I intend to do that as soon as I find the time, but of course 
you're welcome to beat me to it.

Post a reply to this message

On 3/25/19 7:41 PM, clipka wrote:
> Am 25.03.2019 um 23:55 schrieb clipka:
> 
>> If `Cond_Stack.back().PMac->endPosition == CurrentFilePosition()` 
>> throws an exception, it means either of two things:
>>
>> (A) The positions compared are from two different files, which means 
>> the calling code has failed to check whether it is even in the right 
>> file.
>>
>> (B) The line/column tracking is buggy, and yields different positions 
>> in different situations.
> 
> 
> Well, looking at the code of `IsEndOfInvokedMacro()` again, it 
> _strongly_ reeks of scenario (A):
> 
>      return (Cond_Stack.back().PMac->endPosition ==
>                  CurrentFilePosition()) &&
>             (Cond_Stack.back().PMac->source.fileName ==
>                  mTokenizer.GetInputStreamName());
> 
> should probably be swapped around, like so:
> 
>      return (Cond_Stack.back().PMac->source.fileName ==
>                  mTokenizer.GetInputStreamName()) &&
>             (Cond_Stack.back().PMac->endPosition ==
>                  CurrentFilePosition());
> 
> so that the `LexemePosition` comparison operator is only invoked if the 
> file names match.
> 
> 
> I guess you got really lucky in your test (and the OP unlucky in their 
> use case), in that you happen to have created the following scenario:
> 
> - A macro is invoked.
> - That macro includes a different file.
> - That included file happens to have a `#` (e.g. `#declare`) at the same 
> file offset as the `#end` of the macro.
> - However, the `#` happens to be in a different line and/or column.
> 
> 
> In such a scenario, encountering the `#`, the parser would first test 
> whether this is the `#end` of the macro it has been waiting for, calling 
> `IsEndOfInvokedMacro()`.
> 
> `IsEndOfInvokedMacro()` in turn would checks whether the current file 
> position matches that of the macro's `#end` by calling 
> `LexemePostition::operator==`.
> 
> `LexemePostition::operator==` in turn would find that the file position 
> does match. It would then verify that the match also reflects in line 
> and column, and find that they differ, and report this inconsistency via 
> an Exception.
> 
> `IsEndOfInvokedMacro()` wouldn't even get a chance to test whether it is 
> looking at the right file, because its author (duh, I wonder who that 
> stupid git might have been...) chose to do the two tests in the wrong 
> order.
> 
> 
> Now aside from the stupid git who authored `IsEndOfInvokedMacro()` 
> there's someone else to blame for the situation, namely whoever thought 
> it was a good idea to keep the stack of include files separate from the 
> stack of pending "conditions". If they were using one and the same 
> structure, having `INVOKING_MACRO_COND` on top of the stack would imply 
> that we're in the file with the `#end`, and the file name test would be 
> redundant.
> 
> I think it should be possible to merge the include stack into the 
> "condition" stack without adverse effects, but I haven't finished work 
> on that yet.
> 
> 
> I should note that I haven't tested this hypothesis and fix for the 
> error yet; I intend to do that as soon as I find the time, but of course 
> you're welcome to beat me to it.

Thanks for the detailed explanations and, though I'm busy with RL 
starting in a little while today, I think you've figured it out.

I know from looking at the values the position was matching and the line 
and column were not. Further, when I look at my test cases (all some 
pretty close variation of Warren's original) the failing ones all have 
the # of the declare in just the right spot as you suggested. I'd not 
recognized this was so.

My patch, then, just avoided the assertion in the 
LexemePosition::operator== I suppose. I also didn't know the parser 
assertions were today on always and I wasn't thinking an assert could 
cause the exception.

Quick additional question. Is it the --enable-debug configure flag that 
- normally - turns on the pov asserts? I've never verified this 
assumption of mine by creating a pov assert in the code I know should 
fail.

Bill P.

Post a reply to this message

Am 26.03.2019 um 11:46 schrieb William F Pokorny:

> Quick additional question. Is it the --enable-debug configure flag that 
> - normally - turns on the pov asserts? I've never verified this 
> assumption of mine by creating a pov assert in the code I know should fail.

To be honest, I haven't a clue. I'm not sure what the `--enable-debug` 
flag does exactly (it's some standard mechanism in the Automake tools), 
nor whether and how assertions are enabled or disabled in Unix builds.

I'm pretty sure that `--enable-debug` does enable the creation of debug 
information in the binaries, so that a debugging tool can correlate 
binary code addresses with original source code lines, so that you can 
e.g. set breakpoints and step through the code line by line with a 
debugger. I also wouldn't be surprised if it turns off optimizations by 
default, because those tend to re-order instructions and make a 
line-by-line correlation impossible. Whether it does anything beyond 
that I do not know. It might dictate whether `NDEBUG` is defined or not, 
which would conceivably disable/enable assertions.

Post a reply to this message

On 3/26/19 8:10 AM, clipka wrote:
> Am 26.03.2019 um 11:46 schrieb William F Pokorny:
> 
>> Quick additional question. Is it the --enable-debug configure flag 
>> that - normally - turns on the pov asserts? I've never verified this 
>> assumption of mine by creating a pov assert in the code I know should 
>> fail.
> 
> To be honest, I haven't a clue. I'm not sure what the `--enable-debug` 
> flag does exactly (it's some standard mechanism in the Automake tools), 
> nor whether and how assertions are enabled or disabled in Unix builds.
> 
> I'm pretty sure that `--enable-debug` does enable the creation of debug 
> information in the binaries, so that a debugging tool can correlate 
> binary code addresses with original source code lines, so that you can 
> e.g. set breakpoints and step through the code line by line with a 
> debugger. I also wouldn't be surprised if it turns off optimizations by 
> default, because those tend to re-order instructions and make a 
> line-by-line correlation impossible. Whether it does anything beyond 
> that I do not know. It might dictate whether `NDEBUG` is defined or not, 
> which would conceivably disable/enable assertions.

Back at it this morning.

First, flipping the file name compare so it's ahead of the file position 
compare (and assert) works fine too with all the test cases I have that 
fail.

Second, on enabling the POV_ASSERT source code mechanism, took the time 
to do some full compiles with a known fail assert of 1 == 0 and the 
short of it for *nix is doing something like:

./configure COMPILED_BY="wfp__POV_DEBUG" CXXFLAGS="-DPOV_DEBUG"

works. The -D<var> form on the define is added as a compiler flag to do 
the POV_DEBUG define, which, turns on the POV asserts that exist in the 
code (except I guess where not otherwise controlled or hard-set).

Lastly, on the --enable-debug flag, it is not today defining POV_DEBUG 
which is what turns on the POV_ASSERTs. Most of my actual debug compiles 
use a configure command like:

./configure COMPILED_BY="wfp" --disable-optimiz --disable-strip \ 
CXXFLAGS="-Og -ggdb fverbose-asm"

so --enable-debug isn't strictly needed for debugging though on line 
documentation I read just now indicates - by default anyway - it does 
something equivalent given certain arguments. Looks like you can select 
what sort of debugging gets turned on - and you can provide a second 
argument defining variables explicitly (ie POV_DEBUG) and there is the 
NDEBUG you mentioned (boost seems to use this some) getting set by 
default if the flag's argument turns debugging off (or the flag's not 
used but enabled?). What some documentation says only; Not sure what 
works or not for --enable-debug with our current set up without doing a 
bunch of compiles and tests. An investigation for another time. The flag 
doesn't control the POV_ASSERTs today via NDEBUG in any case - so at 
best it might provide another way to define POV_DEBUG using that second 
argument.

Bill P.

Post a reply to this message

On 3/28/19 6:49 AM, William F Pokorny wrote:
> On 3/26/19 8:10 AM, clipka wrote:
...
> 
> Second, on enabling the POV_ASSERT source code mechanism, took the time 
> to do some full compiles with a known fail assert of 1 == 0 and the 
> short of it for *nix is doing something like:
> 
> ./configure COMPILED_BY="wfp__POV_DEBUG" CXXFLAGS="-DPOV_DEBUG"
> 
> works. 
...
> 

With the thought I might help others doing debug by rambling more about 
my ignorance/learning while attempting to debug this issue...

Woke up this morning remembering seeing a core file when I did my 
POV_ASSERT(1 == 0) test with a compile where POV_DEBUG was really 
defined. This got me wondering why I didn't get a core file with initial 
failing code here given an assert ran and I did a debug compile. A core 
file would have have made it easy to look at the call stack with gdb.

Well, looking at the source code this morning it's because we have this 
set up today for the parser related asserts in ./parser/configparser.h :

#ifndef POV_PARSER_DEBUG
     #define POV_PARSER_DEBUG POV_DEBUG
#endif

...

#if POV_PARSER_DEBUG
     #define POV_PARSER_ASSERT(expr) POV_ASSERT_HARD(expr)
#else
     #define POV_PARSER_ASSERT(expr) POV_ASSERT_SOFT(expr)
                                  // POV_ASSERT_DISABLE(expr)
#endif

I wasn't previously getting POV_DEBUG set so I was getting a soft assert 
which parse.cpp catches triggering a more graceful - but less 
informative (no core file) - program exit. Trying my debug compile again 
just now with POV_DEBUG set, I do get a core file and gdb gives me :

     ...
#4  0x000055a82da4b703 in pov_parser::LexemePosition::operator==
     o=...) at parser/parsertypes.cpp:62
#5  0x000055a82da40420 in pov_parser::Parser::IsEndOfInvokedMacro
     at parser/parser_tokenizer.cpp:964
#6  0x000055a82da44dcd in pov_parser::Parser::Get_Token
     at parser/parser_tokenizer.cpp:289
#7  0x000055a82da05cf7 in pov_parser::Parser::Parse_Frame
     at parser/parser.cpp:6559
#8  0x000055a82da06591 in pov_parser::Parser::Run
     at parser/parser.cpp:242
#9  0x000055a82d9cb3ef in pov::ParserTask::Run
     at backend/control/parsertask.cpp:81
...

Today, I'd find my way to IsEndOfInvokedMacro in less than an hour 
instead of the full day it took me to trace top down - so to speak.

The lessons I learned: Get POV_DEBUG set in your compile while debugging 
any issue. If still no core file generated and tangled in a POV-Ray code 
assert, be sure you're getting a hard assert and not a soft one. I 
didn't know we had 'soft' POV_ASSERT* version until today.

Bill P.

Post a reply to this message

On 3/26/19 6:46 AM, William F Pokorny wrote:
> On 3/25/19 7:41 PM, clipka wrote:
>> Am 25.03.2019 um 23:55 schrieb clipka:
>>
>>> If `Cond_Stack.back().PMac->endPosition == CurrentFilePosition()` 
>>> throws an exception, it means either of two things:
>>>
>>> (A) The positions compared are from two different files, which means 
>>> the calling code has failed to check whether it is even in the right 
>>> file.
>>>
>>> (B) The line/column tracking is buggy, and yields different positions 
>>> in different situations.
>>
>>
>> Well, looking at the code of `IsEndOfInvokedMacro()` again, it 
>> _strongly_ reeks of scenario (A):
>>
>>      return (Cond_Stack.back().PMac->endPosition ==
>>                  CurrentFilePosition()) &&
>>             (Cond_Stack.back().PMac->source.fileName ==
>>                  mTokenizer.GetInputStreamName());
>>
>> should probably be swapped around, like so:
>>
>>      return (Cond_Stack.back().PMac->source.fileName ==
>>                  mTokenizer.GetInputStreamName()) &&
>>             (Cond_Stack.back().PMac->endPosition ==
>>                  CurrentFilePosition());
>>
>> so that the `LexemePosition` comparison operator is only invoked if 
>> the file names match.
>>
...

Documenting that Christoph's suggested code change up top, which fixed 
the bug with Warren's original test case and related test cases I 
created, is NOT in the v38 master branch as of commit 74b3ebe0 (March 8, 
2019).

Christoph - I think - planned to fix the core issue, but this has not 
happened as of today so the bug persists.

Bill P.

Post a reply to this message