Wiki tells about problems the dfa encounters:
http://wiki.freepascal.org/FPC_New_Features_3.0#Node_dfa_for_liveness_analysis

In this code if "b" is true in the first test, it will be true also in
the second test, thus using "i" is safe and valid.
  ...
  if b then
    i:=1;
  writeln;
  if b then
    writeln(i);

I understand that complex code can have many variations of such constructs and analysing them all is difficult.

However for "case ... of" construct with enum types the dfa could be improved easily, at least I could imagine so.
There is plenty of code like this:

  function TControlBorderSpacing.GetSpace(Kind: TAnchorKind): Integer;
  begin
    case Kind of
    akLeft: Result:=Left;
    akTop: Result:=Top;
    akRight: Result:=Right;
    akBottom: Result:=Bottom;
    end;
  end;

All the enum values are listed and the Result is set for sure, yet FPC complains:
controls.pp(3721,1) Warning: Function result variable does not seem to be initialized

If the compiler used the information about all enums being present, it would reduce the number of dfa false positives in eg. Lazarus sources dramatically.

Juha

Result is undefined if caller passes an out of range value.

An example: TControlBorderSpacing.GetSpace(TAnchorKind(-1));

Result is indeed not initialized, so silencing this message may be harmful.

Denis

Denis Kozlov wrote

Result is undefined if caller passes an out of range value.

That would be an error at the time this out-of-range value would be converted to TAnchorKind. If range checking is off there or disabled via an explicit type cast, then the result is undefined by definition. The compiler can and does assume in various cases that a type only holds values valid for that type.

Jonas

For some reason my reply went only to Denis first. Another try here...

Denis Kozlov wrote

Result is undefined if caller passes an out of range value. An example: TControlBorderSpacing.GetSpace(TAnchorKind(-1)); Result is indeed not initialized, so silencing this message may be harmful.

Yes but that is an illegal situation. It is already trapped by range check -Cr. The compiler can safely assume that such situation does not happen. Actually the compiler already assumes that in other situations. I remember a bug years ago in Lazarus, caused by an out of range enum value. FPC had changed its behavior and the bug got triggered.

Juha

I have just created a new GUI application, enabled range checking "-Cr" and executed the following:

ShowMessage(IntToStr(Form1.BorderSpacing.GetSpace(TAnchorKind(-1))));

The displayed value is a semi-random integer. No compiler errors or run-time errors.

P.S. Same behaviour for Lazarus 1.6.4 + FPC 3.0.2 and Lazarus 1.4.4 + FPC 2.6.4.

Denis

That is why I said "If range checking is off there or disabled via an explicit type cast, then the result is undefined by definition." You use an explicit typecast above.

Jonas

The typecast suppresses a range error in your code. However the LCL method GetSpace should give an error if it is compiled with -Cr.
So, build Lazarus + LCL with profile "Debug IDE" and try again.

Juha

I just wanted to highlight that these cases as legal and I presume not uncommon, particularly if values are deserialized and typecasted.

It seems that there is no way for compiler to know if the result has indeed being initialized by reason of enumeration of valid values, because one has also account for invalid values.

I think that compiler should always warn (as it does now) if result is not initialized against all possible cases.

Denis

Jonas Maebe wrote

That is why I said "If range checking is off there or disabled via an explicit type cast, then the result is undefined by definition." You use an explicit typecast above.

Denis Kozlov wrote

I just wanted to highlight that these cases as legal and I presume not uncommon, particularly if values are deserialized and typecasted.

They are legal in the sense that they result in undefined behavior and in that case the compiler is free to act as it sees fit.

Regards,
Sven

Then this de-serialisation code must perform range checking. Again: if you assign an invalid value to a variable by typecasting, disabling range checking, inline assembly, passing a pointer to C code and overwriting the value there, or in any other way, Pascal code that works with the resulting value has undefined behaviour. The program could crash, silently fail, raise a random exception at some point, or do anything else.

It seems that there is no way for compiler to know if the result has indeed being initialized by reason of enumeration of valid values, because one has also account for invalid values.

No, the compiler does not have to care about the fact that you may have put invalid data in that variable by bypassing the type system. If you do that, you are on your own. The compiler also uses type information to perform optimizations (e.g. "high(TAnkorKind) < Kind" will be replaced by "false" at compile time, because there is no valid program flow with which this statement can become false).

I think that compiler should always warn (as it does now) if result is not initialized against all possible cases.

The DFA simply does not take into account the result of case-statements at all right now. If you have a "case bytevar of" with every possible value from 0 to 255, you will still get the warning.

Jonas

Jonas Maebe wrote

The DFA simply does not take into account the result of case-statements at all right now. If you have a "case bytevar of" with every possible value from 0 to 255, you will still get the warning.

Sven Barth wrote

They are legal in the sense that they result in undefined behavior and in that case the compiler is free to act as it sees fit.

By covering all possible cases I meant including "else" condition in all case statements, which either defaults to some acceptable value or raises an exception to highlight an invalid value. This is just a standard practice for me, so my code never produces "uninitialized result" warnings, unless I actually made a mistake by not covering all possible cases, in which case I would really like to see that warning no matter what. I prefer to see a controlled exception instead of having an invalid value producing a pseudo-valid semi-random result which may be much more difficult to trace later.

Complier currently warns (cares) about such execution flows which are triggered by invalid data, and I happen to find it useful :)

However, I do understand that compiler can rightfully not care about invalid data, hence, choose not to issue the "uninitialized result" warning if all valid values have been enumerated.

Denis

Denis Kozlov wrote

I just wanted to highlight that these cases as legal and I presume not uncommon, particularly if values are deserialized and typecasted.

No they are not legal. Why you write this nonsense? Values outside the range dictated by the type system are always wrong. No excuses, no exceptions.
For the purposes of DFA the compiler can always assume the values are in range.

- Later: -

Complier currently warns (cares) about such execution flows which are triggered by invalid data, and I happen to find it useful :)

Again nonsense.
The range check -Cr warns about out-of-range data, yes, but this thread is about DFA and not about range checks.
I feel you hijacked my DFA thread for your own purpose but I don't really know what that purpose is.
Are you saying that typecasts should be forbidden to prevent range errors? No, it would not prevent bugs in code.
Anyway, please start a new thread about range check issues.

Juha

For the record: this can already happen. If a jump table for a case statement is generated, the compiler does not check the boundaries if the jump table covers the whole declared (!) range of the case variable.

Juha,

Just to get this straight, I am talking about DFA and your proposal to remove "uninitialized result" warning when all declared values of a set have been enumerated in a case statement. I don't have any issues with range check errors, they were brought up and became part of conversation. Now, put aside range checks and focus on DFA and case statements.

Consider the code fragment below. Currently, FCP 3.0.2 with -O3 shows "uninitialized result" warning for Convert2 and Convert3 functions, but not for Convert1. I find this perfect as is, and, of course, you can rightfully disagree.

You suggest that only Convert3 function should raise "uninitialized result" warning, while Convert1 and Convert2 should not. I find this less useful, and, again, you can rightfully disagree, but it won't change the fact that it is still less useful for me (and possibly others).

I won't indulge in any further debate. I voiced my opinion/concern and will leave free to ignore it.

----------------------------
type
   TConvertType = (ctA, ctB);
function Convert1(Value: TConvertType): Integer;
begin
   case Value of
     ctA: Result := 1;
     ctB: Result := 2;
     else Result := 0;
   end;
end;
function Convert2(Value: TConvertType): Integer;
begin
   case Value of
     ctA: Result := 1;
     ctB: Result := 2;
   end;
end;
function Convert3(Value: TConvertType): Integer;
begin
   case Value of
     ctA: Result := 1;
   end;
end;
----------------------------

You did of course read Jonas' statement, that if you call Convertn(TConvertType(-1))

then Convert1 and Convert2 may crash, depending on compiler version and optimization level (this may also depend on how many elements TConvertType has)

Btw, from Jonas statement follows, that the "else" part in Convert1 may be silently removed by the compiler. (maybe now or maybe by future fpc versions). This may then give an "unreachable code" warning)

The warning however is only useful for Convert2, if you speculate that the "else" in Convert1 is used for TConvertType(-1). If it isn't then you have a bigger problem, and that is not caught by this warning anyway.

Denis Kozlov wrote

You suggest that only Convert3 function should raise "uninitialized result" warning, while Convert1 and Convert2 should not. I find this less useful, and, again, you can rightfully disagree, but it won't change the fact that it is still less useful for me (and possibly others).

Sorry but this is not a matter of opinion. Your view is just flawed and wrong. FPC developers tried to explain that your "else" can be optimized away and the program may crash.
The compiler trusts that data in an enum variable is legal, within the range. It should trust the same way when doing DFA. It is logical and consistent.

If you let your enum data be out of range then your code is seriously buggy. Why would you write such buggy code?
What you must do is add sanity checks for code that initializes enum data based on some other data.
For example you are streaming and get data as string:

type
  TEnumType = (ctA, ctB);
function StrToEnum(s: string): TEnumType;
begin
  case s of
    "ctA": Result := ctA;
    "ctB": Result := ctB;
    else raise Exception.Create('Illegal TEnumType value!');
  end;
end;

The same way if you get integer data from I/O, you must verify the integer is within range _before_ typecasting it.

Logical and robust.

Juha

Juha Manninen wrote

The compiler trusts that data in an enum variable is legal, within the range. It should trust the same way when doing DFA. It is logical and consistent.

I understand your reasons now and agree that DFA should cover only logical (according to the compiler) scenarios.

Sanity checks is exactly what I do, but in both directions (see example below).

I can't control what values other users/developers may supply, nor I restrict the use of different compiler versions and build flags. It is because "TConvertType(-1)" and the likes are an acceptable statement for compiler, I make it my responsibility to handle all possible input values, no matter how illogical and invalid they may be. This is a design pattern that I follow.

If the 'else' condition is optimized away in certain compiler versions and/or by certain complier optimization flags, that is no problem, at least not for me. Then, as Martin highlighted, "unreachable code" warning may be appropriate.

Currently, TConvertType(X) doesn't produce any errors where X is out of declared range of TConvertType. So I shall continue to account for potential invalid values, even if my attempts may be optimized away in some circumstances. I might be trying to account for an illegal or undefined behaviour, but I do nonetheless, if it improves the chances of catching errors earlier.

type
   TConvertType = (ctA, ctB);
function StrToConvert(const Value: String): TConvertType;
begin
   case Value of
     'A': Result := ctA;
     'B': Result := ctB;
     else raise Exception.CreateFmt('Invalid convert value string (%s)',
[Value]);
   end;
end;
function Convert(Value: TConvertType): Integer;
begin
   case Value of
     ctA: Result := 1;
     ctB: Result := 2;
     else raise Exception.CreateFmt('Invalid convert value (%d)',
[Ord(Value)]);
   end;
end;

Denis Kozlov wrote

I can't control what values other users/developers may supply, nor I restrict the use of different compiler versions and build flags.

Compiler flags have nothing to do with the fundamental issue here. If other developers supply buggy code to you, then they have to fix it obviously. This holds true for any bugs.

It is because "TConvertType(-1)" and the likes are an acceptable statement for compiler, I make it my responsibility to handle all possible input values, no matter how illogical and invalid they may be. This is a design pattern that I follow.

You have good intentions but somehow your logic slipped to a wrong track. You must prevent invalid data entering an enum variable in the first place. So, if you get integer data from eg. stream or user input and convert to enum, you must ensure the integer is within range _before_ typecasting it.
Like:

  if (i<0) or (i>Integer(High(TConvertType))) then
    raise Exception.CreateFmt('Invalid convert value (%d)', [i]);
  ctvar := TConvertType(i);

Typecast is always a "serious" thing. It means the original type is wrong and must be forced to something else.
Obviously you need a sanity check there!
There is only a limited number of places where you set an enum variable. It is rather easy to make sure the value is sane. After that you don't need to worry about it, the value is valid always.

Your extra checks for enum values are like a desperate band-aid for a bug that happened already elsewhere. No, a bug must be fixed where it is.

Juha

Interestingly, I just ran into "bad" code generation with exactly the properties discussed in this thread.

Take a function like this:

function SignatureSubpacketTypeToStr(const X: TSignatureSubpacketType): String;
begin
  case X of
    sstReserved00     : Result:= 'Reserved00';
    sstReserved01     : Result:= 'Reserved01';
    sstCreationTime   : Result:= 'CreationTime';
....

Because every declared element is covered, the generated code for it ends up being a computed goto:

0x10047c4c <+28>: mov -0x4(%ebp),%al
0x10047c4f <+31>: and $0xff,%eax
0x10047c54 <+36>: jmp *0x10071d08(,%eax,4)

Which is perfectly fine if X is guaranteed to be in range of the elements the case statement matches to. If it is not, as it may be with invalid input data (as read from a file), that jump goes somewhere undefined - and most importantly, not into any else statement.

So, while we have code that looks like Result is always properly initialized, what we get instead is code that doesn't actually work. And no kind of DFA could detect that, except also range-checking everything.

Just thought I'd share that, as a less synthetic example than some discussed here.

Regards,
Martok

No, it is not bad code generation.
It is exactly the expected result. At least that is what I understood from previous mails in this thread, specifically (in the context of out-of-range input):

Jonas Maebe wrote: > That is why I said "If range checking is off there or disabled via an > explicit type cast, then the result is undefined by definition."

"undefined result" does include the possibility of a crash.

This is the same as accessing an

   Array[enum] of foo;

if enum is out of range, then it may crash, or behave in any undefined way.

The only thing that could be added here is, that if dfa detects that all enums are used in a case statement, and yet there is an "else", then it should warn about "unreachable code".

In our previous episode, Martin said:

Afaik intermediate values in a sparse enum are not rangechecked at all? They fall within the range (lowestvalue..highestvalue), but are not valid anyway.

The attitude displayed over on #32079 is, quite frankly, terrifying. Apparently a language which from the beginning has intrinsics for reading and writing files must never be used for doing so, or wild things may happen /and that's okay/.

Implying that input should already be sanitized on a bug about something that breaks input sanitation code (but only sometimes) is just... wow.

If anybody wants it, here's the patch I'll be rolling on the windows snapshots from now on.

Have a good weekend,
Martok

On Sat, July 1, 2017 16:45, Martok wrote:

Hi,

I agree that it might be useful to extend the documentation of case ... of
as suggested in one of the comments.

I also agree to Jonas that it should not be a problem of the compiler to perform these checks. However, there's one thing that worries me - I assumed that the respective checks should be performed when reading the input from a file and an error condition should be raised when encountering a value not fitting the respective type. Unfortunately, this is apparently not the case, because the following test program happily runs even when the test file contains a value outside the TEnum range:

type
 TEnum = (one, two, three);
var
 F: file of TEnum;
 E: TEnum;
begin
{$I+}
{$R+}
 Assign (F, 'testfile.tst');
 Reset (F, 1);
 while not (Eof (F)) do
  begin
   Read (F, E);
   WriteLn (Ord (E));
  end;
 Close (F);
end.

Interestingly, the error is caught if I try to write out the read value back to console (i.e. add WriteLn (E) after the WriteLn line). I believe that the same error should already happen while reading...

Tomas