[fpc-devel] Const optimization is a serious bug

[Chad Berchek]

Max wrote:
> It's subjective on my side, but If it existed so many years in
> borland/inprise/embarcadero products and I can not remember my own
> bug fixes related to something like this, I consider your example
> more artificial than one might think.

I understand what you're saying, and I had the same thought myself. It
does seem hard to believe that if this were really serious it would go
unattended to for so long. So, I realize that it probably does not
happen very often. But I for one cannot say I like using a compiler that
"usually" works.

Keep in mind the main reason why we don't see this fail more often:
Usually there are a lot of other variables referring to an instance. So,
although the refcount is probably decremented incorrectly rather
frequently, it is only rarely that we see it actually become a problem
(when it reaches zero).

But I don't consider the fact that it is rare and /usually/ harmless to 
be an excuse for incorrect results.

I also don't think we should limit ourselves by settling for some of the 
mistakes that may have been made in the past in Delphi.

Martin wrote:
> I don't think it is a bug.
...
> "(const s: string)" is a declaration by the programmer, that the
> variable (the string) will not be modified at all. That is neither by
> the procedure called, nor by any code outside the procedure, via any
> other  reference to the data. If the programmer sticks to what he
> declared, then it works, if not it crashes.

Is that true? I am not necessarily asserting that your statement is 
false; it could well be true. However I personally have not seen it 
documented so if anyone has a reference I would like to see it. As far 
as I have been able to tell from the docs of Delphi and FPC, the const 
declaration does NOT mean the programmer promises to not modify the 
instance; it means he promises to not modify the variable, which the 
compiler does indeed enforce. The variable and the instance are not the 
same. The variable is const but I have not seen documentation to suggest 
that the instance is supposed to be treated as const by the programmer. 
If I am incorrect then I will gladly accept whatever documentation you 
can direct me to. Also, as my demo shows, the programmer may not even 
know when an instance is being used as a const variable. In my demo, the 
programmer does not do a function call but rather assigns a property; it 
just so happens that behind the scenes the setter takes a const parameter.

> Anyway, if you do not like the feature do not use it.

Unfortunately, that is not true. I don't have to use const in my own 
code, but I cannot control the fact that it is already widely used in 
other libraries, including those of FPC.

> But anyway, this feature is already used, and therefore will cause lots of complains if removed.

I agree that if performance is significantly hurt there will be 
complaints. But I don't see how that negates the need for it to work 
correctly. What is needed is a solution that does not degrade the 
performance noticeably, though I realize that is easier said than done.

I just want to ask, after looking at the demo, is this not the least bit 
scary to anyone? I mean, you can make one very simple, tiny change and 
turn a perfectly working program into an immediate crash for no apparent 
reason. The secrets are buried in there. In a real world scenario you 
wouldn't be able to see it crash so easily. It may or may not crash, or 
it may just crash much later, or it may not crash but just be a security 
vulnerability waiting for someone to discover.

In this regard, I want to go back to something Max said: "I can not 
remember my own bug fixes related to something like this". I agree: me 
neither. But mysterious crashes do happen, and sometimes someone might 
"fix" them by changing some other things which are only tangentially 
related, i.e., they have a convenient side effect of hiding the bug by, 
for instance, affecting the reference counting by coincidence, as such 
interactions can be quite deep.

We also have to remember that *probably almost nobody* remembers fixing 
a bug related to this. That's because most people who come up against 
this bug probably have no clue what is happening and only by making some 
other changes do they coincidentally "fix" it. I propose that for a 
problem like this you cannot rely on reported incidents to determine how 
frequent it is.