copy of literal array is not made when it is passed as argument to a non-const qualified formal parameter

[YashasSamaga]

A copy of a literal array is not made on the heap before passing it to a function as an argument to a non-const qualified formal parameter. The changes made to the argument modify the array literal (this shouldn't be happening).

Example:

The following snippet highlights the issue:

#include <a_samp>
f(arr[]) {
    printf("%s", arr);
    arr[3] = 'Z';
}
wrapper() {
    f("ABCDEFGHIK");
}
main () {
  wrapper();
  wrapper();
}

produces

ABCDEFGHIK
ABCZEFGHIK

Most people would expect "ABCDEFGHIK" to be passed in both the function calls. However, it does not. The compiler pushes the address of the literal array on to the stack and the function modifies the literal array. These changes are wrongly retained for future function calls.

What could be done?

A copy of the literal array has to be made on the heap while passing it as an argument to a non-const qualified formal parameter. This will ensure that any changes to the array will not be reflected in the future function calls (which is intended).

In case the formal parameter is const qualified, then a copy wouldn't be needed as the function guarantees that it will not modify the literal array.

Issues to be considered

• could break scripts which rely on the bug

• could add unnecessary performance cost to poorly written scripts (functions which are not const-correct, i.e: an array parameter which is not modified is not marked as const; this leads to unnecessary copying of the literal array on the heap)

More issues pointed out by Y_Less (#276 (comment)):

• SAMP natives are not const correct

• Adding a warning for unmodified non-const arrays will introduce new warnings in legacy code.

• How would this be controlled for vararg functions? What is the correct behaviour for passing "hi" to F(...). The ... simply cannot be declared const because not all the parameters may be constant, and there's no real way to determine which parameters are written to or not. Do you add the overhead of a copy to every string literal on the off-chance it is written to?

• Are there any demonstrable bugs changing this behaviour would have prevented?

[Y-Less]

More issues:

• Adding a warning for unmodified non-const arrays will introduce new warnings in legacy code.

• How would this be controlled for vararg functions? What is the correct behaviour for passing "hi" to F(...). The ... simply cannot be declared const because not all the parameters may be constant, and there's no real way to determine which parameters are written to or not. Do you add the overhead of a copy to every string literal on the off-chance it is written to?

• Are there any demonstrable bugs changing this behaviour would have prevented?

[samphunter]

PAWN Language Guide, page 17:

When you are working with strings, or arrays in
general, note that pawn always passes arrays by reference. It does this to
conserve memory and to increase performance

According to PAWN language guide, this is correct behavior.

[YashasSamaga]

In the context of that paragraph in the PAWN Language Guide, the array being passed is not a literal. And people would expect the array to be passed as reference in this case.

However, in case of literals, I believe that these supposedly constant literals should not be modified. In fact, if you try to modify a string literal in C++, you will mostly get a segmentation fault (because the literals end up in read-only data segment and modern operating systems tend to keep this section write-protected). But PAWN is not C++.

[Y-Less]

In retrospect I don't think adding warnings to legacy code is too terrible, since it will force people to actually update (assuming they updated the compiler). However, that doesn't answer the varargs question.

[samphunter]

@YashasSamaga right, but that should be fixed with a warning as Y_Less saied, not with copying data to prevent the modification.

@Y-Less agreed. It would also help if it was a new warning, so that it could be disabled by anyone not wanting to bother with fixing legacy codebases.

As for the vararg, I think we should just accept it is not possible to fix that. Making a copy would be terrible for performance in calls to functions such as format. We could use const in front of ... but even then, there may be functions that need to both work with literals and modify strings passed to it, such as CallLocalFunction.

The only way to fix this I can think of is enforcing the read-only property in the abstract machine, not in pawn itself. Even then, that does not solve natives.

[YashasSamaga]

@samphunter adding a warning does not fix the problem that was stated in the issue; Y-Less meant the warning which suggests adding const qualifier to arrays parameters when they are not modified.

This warning just makes people write const-correct code which would be be EXTREMELY useful if the suggestion mentioned in this issue was implemented because it would point out where the scripter could prevent costly (and useless) copies.

I'd vote for that warning being added whether or not the fix suggested in this issue will be implemented.

---

vaargs

C/++ variable arguments are passed by reference and the costness of the arguments can be violated if the function modifies it. I think it's the responsibility of the person who writes the function to minimize the number of modifications to the arguments.

[YashasSamaga]

These are my opinions on the issues listed.

could break scripts which rely on the bug

There probably aren't many scripts which abuse this bug. If there are some, then it's bad code (in my opinion) and they should update it.

could add unnecessary performance cost to poorly written scripts (functions which are not const-correct, i.e: an array parameter which is not modified is not marked as const; this leads to unnecessary copying of the literal array on the heap)

can be prevented by writing const-correct code.

With the help of the warning 214 (possibly a const array argument was intended), const-correct code can be written.

SAMP natives are not const correct

I don't think anything can be done apart from modifying the includes itself.

Adding a warning for unmodified non-const arrays will introduce new warnings in legacy code.

This would promote people to fix bad code. It suspect it won't take long for popular libraries to go const-correct.

I'd vote to add the warning whether or not the change suggested in this issue is implemented.

How would this be controlled for vararg functions? What is the correct behaviour for passing "hi" to F(...). The ... simply cannot be declared const because not all the parameters may be constant, and there's no real way to determine which parameters are written to or not. Do you add the overhead of a copy to every string literal on the off-chance it is written to?

All variable arguments to variadic functions will be passed as reference. By that, we assume that all arguments are const. It is up to the person who writes such a function to ensure that modifications to the arguments are minimized. In many cases, the modifications done to the arguments can be undone before returning (a common example would be adding a NULL character to truncate a string).

Are there any demonstrable bugs changing this behaviour would have prevented?

As far as I can think, there probably isn't any common case where implementing the change suggested would prevent.

So the suggested change might end up doing more harm than good (for example: non-const-correct SAMP natives).

[Y-Less]

There probably aren't many scripts which abuse this bug. If there are some, then it's bad code and they should update it.

I've abused this, but I've abused pretty much every bug, and don't really count for consideration since my code is still maintained and I know it's abusive.

I don't think anything can be done apart from modifying the includes itself.

fixes.inc covers this already.

I'd vote for that warning being added whether or not the fix I suggest in this issue will be implemented.

Same.

[samphunter]

I vote for the warning.

But I really don't see, why go through the effort of implementing the copying, if code using it is bad anyway.

[YashasSamaga]

@samphunter The issue is that array literals (any literal for that matter) should not change its value. It's a constant. In the case outlined in the issue, the values of the literals are being changed which is contrary to the common notion of what a literal is.

func(arr[]) { arr[2] = 'Z'; }
main () { 
     func("ABCDEF");
}

The problem is that func modifies the string literal. Literals are supposed to be immutable but the above code actually changes it.

bad code?

If any code assumes that the modifying the array parameter will modify the string literal which was passed, it is bad (in my opinion).

If a function does not mark an array parameter as const when it actually does not modify it, it is bad (in my opinion). Writing const-correct code helps the compiler make certain optimizations (like the one I would be describing below).

If the suggestion in this issue is implemented, a copy of the array literal will be made on the heap if and only if the parameter is not marked const. This is because we don't want array literals to be modified (because they are literals and are't supposed to be mutable). However, if a function parameter is marked as const, we would not have to make a copy of the array literal because it won't be modified. The purpose was copying was to prevent the function from changing the array literal but since it does not change, we can directly pass the array literal safely. Hence, marking array parameters which are not modified by the function as const, the extra copy can be eliminated (isn't this an optimization?).

It is not that the code which will cause copies to be created is bad.

possible solutions:

There are two ways I can think of to solve the issue:

• trigger an error if an array literal is being passed to a non-const qualified formal parameter
  This is what happens when you try to pass a const qualified array to a non-const qualified parameter.

  new const arr[] = "ABCDEFG";
  func(arr);
  

  would give argument type mismatch (argument 1)

• make a temporary mutable copy of the array literal on the heap
  if the function modifies the contents of the array, it will modify the copy of the literal on the heap but not the actual literal

The second solution is preferred because people would hate to not have the ability to pass array literals directly to functions.

[samphunter]

@YashasSamaga Am I missing something?

The first solution is literally what I voted for, just I wanted to make it a warning instead of error. That is mostly because it does not break anything.

I am saying, that implementing the second option would be redundant, as the only case where it would make a difference would already produce a warning. And for the few people who purposefully ignore/disable the warning, why mess up their code, if they want to change those literals?

PS: If anything, we could make segfault checks in the interpreter and throw an error, if a script tries to change the code/const data part of the abstract machine. That would cover all the instances, instead of just covering some, but not others, such as vaargs.

[YashasSamaga]

The first solution is literally what I voted for, just I wanted to make it a warning instead of error. That is mostly because it does not break anything.

This would force people to make a copy of the data themselves before passing it to a function to get rid of the warning.

I am saying, that implementing the second option would be redundant, as the only case where it would make a difference would already produce a warning. And for the few people who purposefully ignore/disable the warning, why mess up their code, if they want to change those literals?

This will make the compiler do the copying process and the compiler can do it more efficiently (can directly place instructions to do the copy; if the user had to do it, they would have to invoke some function to do the copy).

If someone wants the literals to be changed, they should use an array.

It would also be a bad practice to modify literals as they go against the common notion that literals are immutable.

By the way, the compiler makes copies of default argument arrays and references on the heap for the very same reason I am contending for.

PS: If anything, we could make segfault checks in the interpreter and throw an error, if a script tries to change the code/const data part of the abstract machine. That would cover all the instances, instead of just covering some, but not others, such as vaargs.

We can't modify the server. There is no separate read-only data segment; both read-only and writable-data end up in the same data segment. Hence, it is not possible to tell which parts of the data segment are mutable and which are not.
As a matter of fact, modifying contents of the code segment is also allowed.

[samphunter]

This would force people to make a copy of the data themselves before passing it to a function to get rid of the warning.

What? Why? You mean if the function is not const correct? They should just make the function const correct, its 5 letters to add... And if the function is supposed to change the string, then they should not give it a literal.

By the way, the compiler makes copies of default argument arrays and references on the heap for the very same reason I am contending for.

Yes, but that is actually useful. I just can't see, how this could ever be.

PS: Do you mean, that this could be used instead of making a working copy? So let's say you can add a \0 instead of space, run some functions and than change it back?
If so, I still don't understand, what the point would be, if you are changing it back, then it does not matter whether we copy it or not...

[YashasSamaga]

What? Why? You mean if the function is not const correct? They should just make the function const correct, its 5 letters to add... And if the function is supposed to change the string, then they should not give it a literal.

Why wouldn't someone want to pass array literals to a function that modifies it?

Yes, but that is actually useful. I just can't see, how this could ever be.

The reason why that copy happens and the reason why copy should be made for array literals is exactly the same.