Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0001094Inform 6Generalpublic2013-04-01 11:282016-10-01 12:44
Reporterzarf 
Assigned ToDavidK 
PrioritynormalSeveritymildReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version6.32 
Target VersionFixed in Version 
Summary0001094: Replacing a recursive function causes absurdity
DescriptionIn the sample below, both invocations of func(2) should refer to the new (replacement) version of the function. Instead, the call in test() goes to the original.

Even crazier, the call in the new func() goes to the original! But the call in the original func() goes to the replacement! So a simple recursion winds up ping-ponging back and forth between the two versions of func().

This bug was not introduced with the recent changes to Replace. I've verified it going back to 6.32 -- it's probably been this way forever. Both Z-code and Glulx.
Minimal Source Text To Reproduce
[ Main;
	test();
	test2();
];

Replace func;

[ func arg;
	print "<orig-", arg, " ";
	if (arg) {
		func(arg-1);
	}
	print ">";
];

[ test;
	print "test: func ", func, " prints: ";
	func(2);
	new_line;
];

[ func arg;
	print "<repl-", arg, " ";
	if (arg) {
		func(arg-1);
	}
	print ">";
];

[ test2;
	print "test2: func ", func, " prints: ";
	func(2);
	new_line;
];

! Actual output:
! test: func 130 prints: <orig-2 <repl-1 <orig-0 >>>
! test2: func 212 prints: <repl-2 <orig-1 <repl-0 >>>
TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0002002)
zarf (developer)
2013-04-01 11:42

vaporware points out that I'm replacing functions in the main file, and Replace is documented as being intended only for functions in a header (containing the System_file directive). Indeed, if I redo the test with a header and System_file, the problem goes away.

(I never understood what System_file was for. Until now!)

So this may be a "stop doing that" situation. However, I think it should be possible to either (a) produce a warning, if not an error, for this case or (b) handle it correctly.

(Once a routine is marked as Replaced, any reference to its name should produce a backpatch marker?)
(0002805)
zarf (developer)
2014-05-20 07:36

When hacking I7 templates, I sometimes wind up Replacing functions (even though I7 generates one-file code).

Turns out this also breaks the $OMIT_UNUSED_FUNCTIONS feature. Which I will want to use someday.

So another vote for regularizing Replace and supporting it for all functions. If that has no run-time cost.

(Admittedly both votes are from me.)
(0004554)
zarf (developer)
2016-09-19 09:11

Related issue described at http://www.intfiction.org/forum/viewtopic.php?f=7&t=19605#p112510 [^] :

"[The functions] were being called between the directive and my replacement functions. So the Glulx template code was running the original function, but any calls from rules (being later in the source code) would run my replacement."

So this is a special case of the general rule "Replaced functions cannot be forward-called between the Replace directive and the end of the replacement definition." (Maybe not forward-called at all? Test that.)

Should definitely be an error. I7 template hacking will still be possible as long as you get the ordering right.
(0004555)
zarf (developer)
2016-09-19 11:52

The problem case seems to be only when the function is called between the original definition and the replacement definition.
(0004556)
zarf (developer)
2016-09-21 12:35

This is a hard problem because Replace can be used pretty loosely. The common case is "Replace X; (wrong definition of X); (right definition of X)" But in fact there could be just one definition of X, or there could be three or more. The implicit rule is to use the last definition and ignore any earlier ones.

Any use of X as a symbol after the last definition works correctly, because the symbol refers to the last definition. Any use before the first definition also works correctly, because it's a forward reference, and therefore the use gets backpatched, and the backpatch uses the last definition. It's the in-between case that causes trouble.

I have three possible solutions:

(A) Refuse to use X as a symbol after the first definition. Refuse to allow a third definition. This gives the right result in the common case, but it breaks code that has just one definition (might occur by carelessness) or three definitions (might happen if you need to override a function that some library has already overridden).

(B) Always backpatch any use of a Replaced function name. This should work but it might change the binary output in the common case. (Because backpatched values are always four bytes.) I don't want to change the output.

(C) Add a REPLACE-USED flag for symbols; use this for any use of a Replaced function name *after it is defined for the first time*. Refuse to accept new definitions once this flag appears. This does the job of throwing an error instead of generating invalid output, but it's an annoying error. It would be better to generate valid output in all cases.

I don't want (A). (B) is ideal if it doesn't change the compiled output. (C) is adequate. I will do more testing of (B).
(0004557)
curiousdannii (developer)
2016-09-21 18:32

Now that I've shifted to more precise code insertions all the calls in my replacements are either before any definition or after the last. I also don't replace any recursive functions. So for now this doesn't affect me.

Am I right in understanding (C) that if I used my current 'after "Section Name" in "File.i6t"' strategy but because of the layout of the original templates there was a call between the first definition and my replacement then it would throw an error? You're right that it's adequate because there is the workaround (don't use Replace and instead replace the whole template section) but it's definitely far from ideal. I'd prefer the flexibility of (B).
(0004558)
zarf (developer)
2016-09-21 20:45

Yes, it would throw an error. My assumption is that the templates are divided finely enough that you can usually put the replacement definition right after the original definition.
(0004561)
zarf (developer)
2016-09-22 14:41

Patch: https://github.com/erkyrath/inform6/commit/fc814dd6fdf93244234acffb9994440e88dcf631 [^]

I managed to make (B) work. (I'm pretty sure.)

Test cases:

- replacerecursetest.inf: New test case. This tests recursive replaced functions; also tests calls to replaced functions at every stage of the process. (Tested G+Z.)
- replacerenametest.inf: An existing test case, created to test the "Replace X Y" renaming syntax. This compiles identically with the patch. (Tested G+Z.)

(See https://github.com/erkyrath/glk-dev/tree/master/unittests [^] for those two tests.)

Other tests:

- Advent.inf and a one-line I7 game. These compiled identically with the patch.

- An early Hadean Lands test game which used "Replace DivideParagraphPoint". This did not compile identically, because (it turns out) one internal function was calling DivideParagraphPoint between the two definitions. So this was a latent bug (although with no symptoms) which the patch fixed correctly.

- An I7 example using my Unicode Parser extension, which uses "Replace" heavily. This did not compile identically. I found that it was producing this code pattern:

Replace FloatParse;
Replace FLOAT_TOKEN;
[ FloatParse; !...original
];
[ FLOAT_TOKEN; !... original
   FloatParse();
];
[ FloatParse; !...replacement
];
[ FLOAT_TOKEN; !... replacement
   FloatParse();
];

In this case, the original version of FLOAT_TOKEN() is compiled wrong (since it is between the two FloatParse() definitions) but then it is replaced and so never called. Both versions remain in the compiled binary (the usual behavior for Replace) but the incorrect value is in dead code and never affects the behavior of the game.

Conclusion: this patch can change the compiled output for a correctly-working game. I was hoping to avoid that, but I think the case is rare and the value of the patch is high enough to make it worthwhile.

- Issue History
Date Modified Username Field Change
2013-04-01 11:28 zarf New Issue
2013-04-01 11:28 zarf Status new => assigned
2013-04-01 11:28 zarf Assigned To => zarf
2013-04-01 11:42 zarf Note Added: 0002002
2014-05-20 07:36 zarf Note Added: 0002805
2016-09-19 09:11 zarf Note Added: 0004554
2016-09-19 11:52 zarf Note Added: 0004555
2016-09-21 12:35 zarf Note Added: 0004556
2016-09-21 18:32 curiousdannii Note Added: 0004557
2016-09-21 20:45 zarf Note Added: 0004558
2016-09-22 14:20 zarf Assigned To zarf => DavidK
2016-09-22 14:41 zarf Note Added: 0004561
2016-10-01 12:44 DavidK Status assigned => resolved
2016-10-01 12:44 DavidK Resolution open => fixed


Copyright © 2000 - 2010 MantisBT Group
Powered by Mantis Bugtracker