{"action":"created","issue":{"url":"https://api.github.com/repos/gap-system/gap/issues/2206","repository_url":"https://api.github.com/repos/gap-system/gap","labels_url":"https://api.github.com/repos/gap-system/gap/issues/2206/labels{/name}","comments_url":"https://api.github.com/repos/gap-system/gap/issues/2206/comments","events_url":"https://api.github.com/repos/gap-system/gap/issues/2206/events","html_url":"https://github.com/gap-system/gap/issues/2206","id":299823906,"number":2206,"title":"kernel idea: make MAX_FUNC_EXPR_NESTING global, use it in read.c, and lower it fro 1024 to e.g. 32","user":{"login":"fingolfin","id":241512,"avatar_url":"https://avatars2.githubusercontent.com/u/241512?v=4","gravatar_id":"","url":"https://api.github.com/users/fingolfin","html_url":"https://github.com/fingolfin","followers_url":"https://api.github.com/users/fingolfin/followers","following_url":"https://api.github.com/users/fingolfin/following{/other_user}","gists_url":"https://api.github.com/users/fingolfin/gists{/gist_id}","starred_url":"https://api.github.com/users/fingolfin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fingolfin/subscriptions","organizations_url":"https://api.github.com/users/fingolfin/orgs","repos_url":"https://api.github.com/users/fingolfin/repos","events_url":"https://api.github.com/users/fingolfin/events{/privacy}","received_events_url":"https://api.github.com/users/fingolfin/received_events","type":"User","site_admin":false},"labels":[{"id":727251035,"url":"https://api.github.com/repos/gap-system/gap/labels/discussion","name":"discussion","color":"322ea5","default":false},{"id":577472995,"url":"https://api.github.com/repos/gap-system/gap/labels/kernel","name":"kernel","color":"d4c5f9","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-02-23T19:28:51Z","updated_at":"2018-02-23T23:20:05Z","closed_at":null,"author_association":"OWNER","body":"Right now, there are at least two places which limit how deep functions (resp. closures) can be nested in each other:\r\n* in `code.c`, we `#define MAX_FUNC_EXPR_NESTING 1024` and use that as a hard limit on how deep one can nest functions. Going beyond this limit triggers an assertion (resp. silently corrupts memory, if assertions are turned off).\r\n* in `read.c`, specifically in `ReadCallVarAss`, we limit the nesting to 65536 on 32bit platforms and not at all (resp. to 2^48) on 64bit platforms:\r\n```C\r\n#ifndef SYS_IS_64_BIT\r\n        // we store nest and indx together in var; if we are on a 32bit\r\n        // system, then this limits both to 16 bits, which we enforce here\r\n        if (nest >= 65536) {\r\n            Pr(\"Warning: abandoning search for %s at 65536th higher frame\\n\",\r\n               (Int)STATE(Value), 0L);\r\n            break;\r\n        }\r\n#endif\r\n```\r\n\r\nThese two limits should be aligned, say by making `MAX_FUNC_EXPR_NESTING` global. For technical reasons, it should stay a power of 2.\r\n\r\nFurthermore,, I don't think I ever saw a nesting deeper than 4 or so (perhaps a bit more with the use of nested `x -> foo` lambda functions). Realistically, going much deeper becomes rather painful, and I doubt it matters. So we might also want to consider reducing the nesting limit to, say, 32. This frees up valuable bits which can be used to store variable reference more compactly (I am thinking about unifying the var identifiers for gvars, lvars, hvars, dvars into a single 32 / 64 bit word)... Anyway, right now there is no pressing reason to reduce the limit, but it will be easier if the code was refactored to use `MAX_FUNC_EXPR_NESTING` uniformly, which to me is another reason why this refactoring would be useful.\r\n\r\nLastly, I think it's not good that `code.c` crashes if the nesting is too deep -- it really should produce an error. To test a hypothetical fix for this, I'd temporarily reduce `MAX_FUNC_EXPR_NESTING` to a much smaller value, making it easier to trigger the problem with test code... Alternatively, one can write a function which generates a string containing cod for set of functions nested to a certain fixed level, in order to generate tests cases, even for the 1024 level function.\r\n\r\nSo in summary:\r\n* make `MAX_FUNC_EXPR_NESTING` global and change `read.c` to use it,\r\n* change `code.c` to give an error instead of crashing when `MAX_FUNC_EXPR_NESTING` is exceeded,\r\n* add a test case for the latter,\r\n* perhaps reduce `MAX_FUNC_EXPR_NESTING` to a smaller value"},"comment":{"url":"https://api.github.com/repos/gap-system/gap/issues/comments/368167177","html_url":"https://github.com/gap-system/gap/issues/2206#issuecomment-368167177","issue_url":"https://api.github.com/repos/gap-system/gap/issues/2206","id":368167177,"user":{"login":"fingolfin","id":241512,"avatar_url":"https://avatars2.githubusercontent.com/u/241512?v=4","gravatar_id":"","url":"https://api.github.com/users/fingolfin","html_url":"https://github.com/fingolfin","followers_url":"https://api.github.com/users/fingolfin/followers","following_url":"https://api.github.com/users/fingolfin/following{/other_user}","gists_url":"https://api.github.com/users/fingolfin/gists{/gist_id}","starred_url":"https://api.github.com/users/fingolfin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fingolfin/subscriptions","organizations_url":"https://api.github.com/users/fingolfin/orgs","repos_url":"https://api.github.com/users/fingolfin/repos","events_url":"https://api.github.com/users/fingolfin/events{/privacy}","received_events_url":"https://api.github.com/users/fingolfin/received_events","type":"User","site_admin":false},"created_at":"2018-02-23T23:20:05Z","updated_at":"2018-02-23T23:20:05Z","author_association":"OWNER","body":"Now that PR #2207 is submitted, I can clarify why I am interested in reducing those limits: doing so would allow me to squeeze `LHSRef` from PR #2207 into a single 32bit word, which would make it very convenient to store. That said, I can definitely squeeze it into 64bit anyway, which should be good enough."}}
{"actor":{"display_login":"fingolfin"}}