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

Handle lists being modified by use of ~ #2477

Merged
merged 1 commit into from
May 26, 2018

Conversation

ChrisJefferson
Copy link
Contributor

Adjust how lists are built when ~ is used, so we do not crash
even if the list is modified as it is being constructed.

This also makes the behaviour of GAP consistent between bytecode
and immediate code.

@codecov
Copy link

codecov bot commented May 17, 2018

Codecov Report

Merging #2477 into master will decrease coverage by <.01%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #2477      +/-   ##
==========================================
- Coverage   74.27%   74.27%   -0.01%     
==========================================
  Files         484      484              
  Lines      245311   245311              
==========================================
- Hits       182205   182200       -5     
- Misses      63106    63111       +5
Impacted Files Coverage Δ
src/exprs.c 93.96% <91.66%> (-0.18%) ⬇️
hpcgap/lib/hpc/stdtasks.g 63.68% <0%> (-1.03%) ⬇️

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please set labels, and format the code

Otherwise looks good to me!

src/exprs.c Outdated
@@ -919,9 +919,18 @@ Obj EvalPermExpr (
**
** 'EvalListExpr' just calls 'ListExpr1' and 'ListExpr2' to evaluate the
** list expression.
**
** The 'safe' argument to ListExpr1 and ListExpr2 handle code which abuses ~
** access and change the partially built list.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handle -> handles ? though I don't see how an argument can "handle" anything by itself?

src/exprs.c Outdated
** access and change the partially built list.
**
** When 'safe' is 1, we use a slower code path which ensures the list is
** in a valid state after each element is added, and not crash if the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing words... perhaps "... is added, and that we don't crash ..."

src/exprs.c Outdated
@@ -999,8 +1008,9 @@ Obj EvalListTildeExpr (
** '[ [1], ~[1] ]' requires that the value of one subexpression is entered
** into the list value before the next subexpression is evaluated.
*/
Obj ListExpr1 (
Expr expr )
ALWAYS_INLINE Obj ListExpr1 (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ALWAYS_INLINE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix all other comments.

With this one, ALWAYS_INLINE because I want the 'safe' compiled out when safe=0, so the fast route (without tilde) is as fast as it used to be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

src/exprs.c Outdated
@@ -1015,15 +1025,21 @@ Obj ListExpr1 (
else {
list = NEW_PLIST( T_PLIST, len );
}
SET_LEN_PLIST( list, len );
if(safe) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add space after if

src/exprs.c Outdated
@@ -1053,17 +1069,25 @@ void ListExpr2 (
{
if (posshole == 1)
{
SET_FILT_LIST(list, FN_IS_NDENSE);
if(!safe || TNUM_OBJ(list) == T_PLIST) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add space after if (or just git clang-format)

gap> (function() return [Length(~),Length(~),Length(~)]; end)();
[ 0, 1, 2 ]
gap> rem := function(l) Remove(l); return 1; end;;
gap> [2,rem(~),rem(~),rem(~)];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that's pretty evil :-)

Copy link
Contributor

@ssiccha ssiccha May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't expect anyone to ever write this, but I feel there shouldn't be ways of making the interpreter crash (at least, without calling ALL_CAPS functions).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sure, it's good to add that test. People do evil things, we shouldn't reward them for it with a crash ;-)

@ChrisJefferson ChrisJefferson added topic: kernel release notes: added PRs introducing changes that have since been mentioned in the release notes labels May 18, 2018
@ChrisJefferson
Copy link
Contributor Author

Hopefully changes made, added to release notes.

src/exprs.c Outdated
@@ -919,9 +919,19 @@ Obj EvalPermExpr (
**
** 'EvalListExpr' just calls 'ListExpr1' and 'ListExpr2' to evaluate the
** list expression.
**
** The 'safe' argument to ListExpr1 and ListExpr2 should be set to 1 when
** the list expression may be modified when its elements are being
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"when its elements are being constructed" -> "while its ..." ? (It took me a moment to parse the current phrasing, but then I did understand it, so it's probably fine as it is -- I just try to be super sensitive and try to simulate how I might read a paragraph if I knew less about the kernel; but of course that's imperfect. Perhaps e.g. @ssiccha would be willing to chime and say whether the above text seems clear to him -- hopefull it is, but if not, it'd be useful to know which bits are not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fingolfin @ChrisJefferson Apart from knowing what exactly an expression is (system.h says its a UInt, but that's all I found out quickly), the formulation is clear to me.

Copy link
Contributor

@ssiccha ssiccha May 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"when its elements are being constructed" -> "while its ..."

"while its" also sounds better to me

src/exprs.c Outdated
@@ -919,9 +919,19 @@ Obj EvalPermExpr (
**
** 'EvalListExpr' just calls 'ListExpr1' and 'ListExpr2' to evaluate the
** list expression.
**
** The 'safe' argument to ListExpr1 and ListExpr2 should be set to 1 when
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this text be part of the documentation comment of ListExpr1 and ListExpr2 instead?

I understand that there are prototypes for those two functions below, but considering the rest of their documentation is elsewhere... Perhaps we could move them up, before this function, to avoid using prototypes? Then they could also be made static.

src/exprs.c Outdated
@@ -1053,18 +1065,25 @@ void ListExpr2 (
{
if (posshole == 1)
{
SET_FILT_LIST(list, FN_IS_NDENSE);
if (!safe || TNUM_OBJ(list) == T_PLIST) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is that safe? Couldn't the tnum be, say T_PLIST_DENSE at this point? That would suggest using if (!safe || IS_PLIST(list)) - or perhaps even IS_LIST?

But why is this necessary at all? SET_FILT_LIST uses a tnum dispatch table -- if the user did something really bad here, wouldn't that just trigger an error via that dispatch table?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, let me rethink this.

src/exprs.c Outdated
** When 'safe' is 1, we use a slower code path which ensures the list is
** in a valid state after each element is added, and we don't crash if the
** size or TNUM of the list is changed during the construction of elements
** in the list.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps that should say "we don't crash if the size or type of the list changed, or even if gets swapped for a non-list object" ? After all, the user could use SWAP_OBJ to replace it by, say, a bigint.

OTOH, perhaps I am just overthinking that; and people using SWAP_OBJ deserve what they get ;-).

src/exprs.c Outdated
** the list expression may be modified when its elements are being
** constructed. This can occur when an element of the list refers to ~.
**
** When 'safe' is 1, we use a slower code path which ensures the list is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, when I was reading the code below, I constantly had to refer back to this comment, because I could not remember whether safe meant: either "we are safe; it is safe to assume that the list won't be messed with", or perhaps "play it safe; use safe code which works even if the list gets messed with".

Since these are opposite, that's a bit of a problem, IMHO. Is it just me? Perhaps we can find a better name. Say protectAgainstListModification or safelyGrowList, safelyExtendList; or perhaps reverse the value, and say assumeListIsPlist or ...? Sorry, no great idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could just call it tildeInUse, seeing as that's all we use it for :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that actually sounds very good to me!

@ChrisJefferson ChrisJefferson force-pushed the tilde-crash branch 2 times, most recently from e4c2711 to 773e1f5 Compare May 24, 2018 21:44
@ChrisJefferson
Copy link
Contributor Author

Now changed/improved

src/exprs.c Outdated
*/
Obj ListExpr1 (
Expr expr )
ALWAYS_INLINE Obj ListExpr1(Expr expr, Int safe)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation now says tildeInUse, great -- but the code still uses safe :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bla!

src/exprs.c Outdated
}
else {
SET_LEN_PLIST(list, len);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also use SET_LEN_PLIST(list, tildeInUse ? 0 : len); for compactness -- but of course it's also perfectly fine as it is :-).

src/exprs.c Outdated
SET_FILT_LIST(list, FN_IS_DENSE);

if (!posshole && !safe) {
SET_FILT_LIST(list, FN_IS_DENSE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the && !safe (resp. && !tildeInUse) here? Isn't the list fully constructed at this point?

Ahh, because we might have used unbind while constructing it, through the use of ~? Is that the reason?
Perhaps a small comment could be added to point that out (or, if I am wrong, then point out the actual reason)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just assuming that SET_FILT_LIST is always an optimisation, and as you say, who knows what state the list is in, or if it's even a list.

Adjust how lists are built when ~ is used, so we do not crash
even if the list is modified as it is being constructed.

This also makes the behaviour of GAP consistent between bytecode
and immediate code.
@fingolfin fingolfin merged commit 38aacc5 into gap-system:master May 26, 2018
@ChrisJefferson ChrisJefferson deleted the tilde-crash branch June 11, 2018 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants