1

I recently started working on a Delphi project and found very strange piece of code.

// Type declarations

type
    TDataSet = record
    VolumeLevel : char;
    DeviceMasks : char;
    DeviceList: array [1..MAX] of array [1..8] of char;
    DisplayList: array [1..MAX] of array [1..8] of char;
end;

type
    TSerialPacket = record
    Preamble: array[1..4] of byte;
    PacketType: byte;
    PacketLen: byte;
    Data: array of char;
    Checksum: byte;
end;

...

// Private fields

Packet  : TSerialPacket;
DataSet : TDataSet;

...

// Actual procedure

SetLength(Packet.Data, sizeof(DataSet));
Packet.Data := @DataSet;

I haven't used Delphi very much so this code seems incomprehensible to me. Compiler thinks this is alright and I can run the code. I ran it with debugger but the value of Packet.Data does not seem to change. Can anyone tell me what this does? It seems very strange to assign a pointer of a custom record to an array of chars.

Also, for some reason the SetLength triggers an error: "Project Foo.exe raised exception class EAccessViolation with message 'Access violation at address 00403860 in module 'Foo.exe'. Read of address 00000000.". During both the working and the crashing runs value of Packet.Data is () and sizeof DataSet is 260. I haven't been able to pinpoint what exactly changes. As far as I know, SetLength should not depend on any other variables than Packet.Data and DataSet.

(I use Delphi XE on Windows 7.)

Ville Salonen
  • 2,654
  • 4
  • 29
  • 34
  • FWIW: With D2007 I get `[DCC Fehler] Project1.dpr(35): E2010 Inkompatible Typen: 'dynamic array' und 'Pointer'` as I would have expected from a quick look at the code. – Uli Gerhardt Jan 11 '11 at 11:32
  • Strange. With Packet.Data := DataSet I got an error "Incompatible types: 'dynamic array' and TDataSet." but Packet.Data := @DataSet is OK. – Ville Salonen Jan 11 '11 at 11:36
  • Ah - mea culpa. I have changed my default compiler options to include {$TYPEDADDRESS ON} - this explains the difference. But as you currently experience you probably should include it, too. :-) – Uli Gerhardt Jan 11 '11 at 12:03
  • Yes, I probably should. As soon as I get the software working, I'll turn on all warning levels and fix the problems. Unfortunately the previous authors seem to have only fixed errors and left all the warnings unattended. – Ville Salonen Jan 11 '11 at 12:26
  • I know that kind of developer. ;-) – Uli Gerhardt Jan 11 '11 at 12:33
  • @Ulrich there are quite a few Delphi developers who will swear that it is better to run with typed address off........ – David Heffernan Jan 11 '11 at 14:42
  • @David: I found a few bugs with it that otherwise would have been hard to track down. To me that's a fair trade-off with having to write a cast here and there. But I had to learn that even some big component developers don't try to compile their code with TYPEDADDRESS ON. :-/ – Uli Gerhardt Jan 11 '11 at 15:21
  • @Ulrich The most significant example of such is Embarcadero themselves since the VCL is, I believe, compiled $T- – David Heffernan Jan 11 '11 at 21:37
  • Indeed. I keep noticing that when I "borrow" some VCL code. :-) – Uli Gerhardt Jan 12 '11 at 07:41

2 Answers2

4

I guess the last line (

Packet.Data := @DataSet;

) should rather be

Move(DataSet, Packet.Data[0], SizeOf(DataSet));
Uli Gerhardt
  • 13,748
  • 1
  • 45
  • 83
  • What is the difference between these two? Is the one above some kind of deprecated way or has it always been an ugly hack? – Ville Salonen Jan 11 '11 at 12:27
  • AFAICS the pointer assignment is a bug. I wouldn't trust it to work reliably in slightly more complex code than your test sample. As I commented above I advise anybody to set $TYPEDADDRESS to ON. Then the compiler detects this kind of bug for you. – Uli Gerhardt Jan 11 '11 at 12:32
  • Although this didn't solve my whole problem, it pointed me a way forward so I'm marking it as accepted. I don't even know if the problem would be solvable given the limited code sample above. I'll get back to this question when I solve it. – Ville Salonen Jan 11 '11 at 14:34
  • @Ville, the technique you asked about in your question has always been wrong, and it has always been prone to memory corruption and invalid-pointer-operation exceptions. Whoever wrote that code wasn't thinking. It sets the length of an array, and then it throws that array away by assigning a new value to the variable. The new value isn't a dynamic array. It might look a little similar to a dynamic array, but any attempt to *use* it could fail because the new value doesn't include the requisite dynamic-array bookkeeping that a real dynamic array would have. – Rob Kennedy Jan 11 '11 at 17:39
1

I think I know what is going wrong with your code...

Like already stated, this is pretty bad:

SetLength(Packet.Data, sizeof(DataSet)); 
Packet.Data := @DataSet;

I'D assume your code will crash on SetLength only the 2nd time you run through it. What happens is 

Packet.Data := @DataSet;

Here, the Array of Char pointer is replaced by the address of the Dataset variable. The Array of Char that was created by "Setlength" is then "freed".

When you get to SetLength for a second time, what the compiler thinks is a pointer to an Array of Char is in fact a pointer to a TDataset. It is similar to calling 

SetLength(@Dataset, SizeOf(Dataset));

(Except that the compiler won't allow that one)

I hope that helps at finding any additional problems your are experiencing.

Ken Bourassa
  • 6,363
  • 1
  • 19
  • 28