Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Repeated string equate causes an infinite loop when referenced #64

Closed
yenatch opened this issue Apr 14, 2015 · 15 comments
Closed

Repeated string equate causes an infinite loop when referenced #64

yenatch opened this issue Apr 14, 2015 · 15 comments
Assignees
Labels
bug Unexpected behavior / crashes; to be fixed ASAP! rgbasm This affects RGBASM

Comments

@yenatch
Copy link
Contributor

yenatch commented Apr 14, 2015

string equs "anything"
string equs "anything"
string

rgbasm will accept the following without complaint if string is not referenced:

string equs "anything"
string equs "anything"

If the equates don't match, rgbasm will raise Buffer safety margin exceeded:

string equs "anything"
string equs "anything else"
@yenatch
Copy link
Contributor Author

yenatch commented Jul 26, 2015

Oops, this is not really a bug (except for the circular reference causing an infinite).

@AntonioND
Copy link
Member

AntonioND commented Apr 22, 2017

It took me a while to understand the problem.

string equs "anything"
string equs "anything"

That is converted to:

string equs "anything"
anything equs "anything"

If string is used, it will be replaced by anything, which will be replaced by anything again, and so on. That's why the bug only happens when you actually use string. The third case is the same thing, I guess that the line yyunputstr(s = sym_GetStringValue(dest)) is executed in an infinite loop causing the buffer to be filled (and that's why that error message is showing).

I don't think that a circular reference should be prohibited, it could be useful for recursive expansion of EQUS, so I don't know what to do.

@AntonioND
Copy link
Member

AntonioND commented Apr 22, 2017

Maybe a warning? Something like "Self reference in an EQUS, it may cause an infinite loop." that is only shown once per execution of rbgasm.

Not sure about how to detect it, though. It's not just a symbol referencing itself, it could be a chain.

@BenHetherington
Copy link
Contributor

Hmm... I'm struggling to think of any reasons why someone would deliberately use EQUS recursively. As a result, I'd say we should show an error and stop straightaway, to avoid a (most likely undesired) infinite loop.

As for detecting it, we could keep track of all the EQUSes we've used in the current evaluation (e.g. with a stack), and stop if we encounter an EQUS we've already seen. This could be a faff, but it'd solve the issue (unless if I've overlooked something).

@AntonioND
Copy link
Member

AntonioND commented Apr 22, 2017

Well, an EQUS can be assigned a multiline string. You can do something like:

STORE_ARGS EQUS "IF _NARG > 0\n DB \\1\n SHIFT\n STORE_ARGS\nENDC"

And you could use it to store an arbitrary number of macro arguments. You can't do that with a macro inside a macro.

EDIT: No, that wouldn't work, sorry. But that's the idea, you could maybe use them to define things. I'm not too sure about limiting things just because we can't find an use case now.

@AntonioND
Copy link
Member

AntonioND commented Apr 22, 2017

On the other hand, it is true that some of the most hacky constructs in C are done using #define, so prohibiting recursive EQUS could make sense. I'm not sure about this.

Relevant: http://stackoverflow.com/a/12447739

@AntonioND
Copy link
Member

AntonioND commented Apr 22, 2017

By the way, the same problem exists with macros:

HELLO : MACRO
    HELLO
ENDM

    HELLO

And this is a completely legitimate use-case of macros, so I'm really not sure about this.

@AntonioND
Copy link
Member

AntonioND commented Apr 23, 2017

I think that the only thing that can realistically be done is to warn about this problem.

I don't think there's a good way of solving this. Both EQUS and macros can have this problem, and it can be hidden very easily if the coder wants to (by having a chain of 2 or 3 definitions, for example).

It cannot be detected by the parser either because when it finds a symbol that has to be replaced by an EQUS it simply sends the string to the lexer for it to handle. It cannot even be detected by preventing the replacement of the same EQUS in the same line twice, you could be trying to use it in two different places and that's not a problem.

Also, it can only be detected when it is actually used: An EQUS may change if you redefine it.

Macros are even worse. If you hide the recursive usage of the macro inside an IF, how can you even know if the problem is going to happen or not? You could even do crazy things like getting the assembler into a loop that only ends when a specific time is reached! xD

EDIT: Actually, that last part isn't true, the time is only calculated when the assembler starts.

@AntonioND
Copy link
Member

I've pushed this: 4228e3e

I really don't know if it makes sense to spend time trying to detect this considering that any possible test is going to be incomplete and, in the end, everything comes down to the programmer doing things right..

@meithecatte
Copy link
Contributor

I would say to not expand macros nor equs when used as a macro or equ name

@AntonioND
Copy link
Member

I don't think I understand what you mean, could you show what you mean with an example?

@meithecatte
Copy link
Contributor

When defining an EQU, the part on the left of it should not be fed into the macro expanding part. That way, string equs "anything" string equs "anything" will stay that way and just redefine string. Same goes for macros - you probably won't need any macros or similar in the name part.

@AntonioND
Copy link
Member

That doesn't help solve the problem. You can still do this:

AAA EQUS "BBB"
BBB EQUS "AAA"

AAA ; Infinite loop

@AntonioND AntonioND added bug Unexpected behavior / crashes; to be fixed ASAP! rgbasm This affects RGBASM labels Apr 2, 2018
@ISSOtm
Copy link
Member

ISSOtm commented Aug 28, 2019

A possible solution would be to enforce a recursion depth limit. This limit could be passed as a command-line argument, too, so a recursion-heavy program could be still made working.

@ISSOtm ISSOtm self-assigned this Aug 29, 2019
@ISSOtm
Copy link
Member

ISSOtm commented Oct 10, 2019

Fixed in e0e8170

@ISSOtm ISSOtm closed this as completed Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected behavior / crashes; to be fixed ASAP! rgbasm This affects RGBASM
Projects
None yet
Development

No branches or pull requests

5 participants