Code gen bug?

[MoonlitKnight]

Using CB2007, the compiler appears to generate code to delete non-existing AnsiStrings, producing access violations or other erratic behaviour at runtime.

Steps to reproduce:

1. Create a new VCL Forms app
2. Add Button1 to the main form
3. Add the following code to the main form cpp file:

class TMyClass
{
private:
AnsiString FStr;
AnsiString GetStr() { return FStr; }

public:
TMyClass(const AnsiString &s) : FStr(s) { };

__property AnsiString AsString = {read=GetStr};

};


void __fastcall TForm1::Button1Click(TObject *Sender)
{
if(SameText(TMyClass("No").AsString, "Yes") &&
SameText(TMyClass("Yes").AsString, "No"))
{
;
}
}

Perhaps I'm missing something?

Regards
Antony

[dhoke]

I haven't looked, but think someone has indicated AnsiString has a reference
counted component...???

Its probably not a codegen bug, but a user-error with regard to AnsiString
implementation.

I think the compiler has to be constructing a temporary to pass as a
reference to your TMyClass constructor, which may be creating suggested ref
counted entity, but somehow I suspect the "temporary" aspect of things is
messing up what you're trying to do, guessing that that entity goes away
when the temporary goes away, which is apparently happening much sooner than
you'd like it to, but probably within the legal range the compiler is
allowed.

Note that I haven't actually tried your example.

Post by MoonlitKnight
class TMyClass
{
AnsiString FStr;
AnsiString GetStr() { return FStr; }
TMyClass(const AnsiString &s) : FStr(s) { };
__property AnsiString AsString = {read=GetStr};
};
void __fastcall TForm1::Button1Click(TObject *Sender)
{
if(SameText(TMyClass("No").AsString, "Yes") &&

Temporary generated for "No"?

Post by MoonlitKnight
SameText(TMyClass("Yes").AsString, "No"))

Temporary generated for "Yes"?

Post by MoonlitKnight
{
;
}
}
Perhaps I'm missing something?
Regards
Antony

[Chris Uzdavinis (TeamB)]

Post by MoonlitKnight
if(SameText(TMyClass("No").AsString, "Yes") &&
SameText(TMyClass("Yes").AsString, "No"))
{
;
}
}
Perhaps I'm missing something?

I can't test what you're claiming, but I do remember that the ternary
operator had a bug where it would destroy AnsiStrings that were not
created. I wonder if you're hitting similar code. The workaround
there was to do the test off to the side, and then use them in the
ternary operator or use a normal if.

What happens if you declare two local bools, initialized to each half
of your conjunction, and then your if uses the bools instead?

[Stewart Gaskell]

Post by Chris Uzdavinis (TeamB)

Post by MoonlitKnight
if(SameText(TMyClass("No").AsString, "Yes") &&
SameText(TMyClass("Yes").AsString, "No"))
{
;
}
}
Perhaps I'm missing something?

I can't test what you're claiming, but I do remember that the ternary
operator had a bug where it would destroy AnsiStrings that were not
created. I wonder if you're hitting similar code. The workaround
there was to do the test off to the side, and then use them in the
ternary operator or use a normal if.

Yes, this is what we were seeing. The odd thing is that the behaviour was
dependant on whether the button was clicked (with the mouse), or the button
activated using the Space bar.
Perhaps the stack is being corrupted, somehow, dependant on the event
handlers being called?

Stewart

[Darko Miletic]

Post by MoonlitKnight
Using CB2007, the compiler appears to generate code to delete non-existing AnsiStrings, producing access violations or other erratic behaviour at runtime.
1. Create a new VCL Forms app
2. Add Button1 to the main form
class TMyClass
{
AnsiString FStr;
AnsiString GetStr() { return FStr; }
TMyClass(const AnsiString &s) : FStr(s) { };
__property AnsiString AsString = {read=GetStr};
};
void __fastcall TForm1::Button1Click(TObject *Sender)
{
if(SameText(TMyClass("No").AsString, "Yes") &&
SameText(TMyClass("Yes").AsString, "No"))
{
;
}
}

Slight change in the code makes this work:

class TMyClass
{
private:
AnsiString FStr;

public:
TMyClass(const AnsiString &s) : FStr(s) { }

AnsiString GetStr() { return FStr; }
__property AnsiString AsString = {read=GetStr};

};

void __fastcall TForm1::Button1Click(TObject */*Sender*/)
{
if(SameText(TMyClass("No").GetStr(), "Yes") &&
SameText(TMyClass("Yes").GetStr(), "No"))
{
;
}
}

So moral of the story is : avoid using weird constructs and always go to
c++ idioms.

[Stewart Gaskell]

Post by MoonlitKnight
class TMyClass
{
AnsiString FStr;
TMyClass(const AnsiString &s) : FStr(s) { }
AnsiString GetStr() { return FStr; }
__property AnsiString AsString = {read=GetStr};
};
void __fastcall TForm1::Button1Click(TObject */*Sender*/)
{
if(SameText(TMyClass("No").GetStr(), "Yes") &&
SameText(TMyClass("Yes").GetStr(), "No"))
{
;
}
}
So moral of the story is : avoid using weird constructs and always go to
c++ idioms.

It's very difficult to avoid "weird constructs" when using propriety C++
extensions. The VCL (via its Delphi roots) is full of such beasts!
I think this bug is related to the next post. See the "Old ternary operator
problem" post.

Stewart

[Leo Siefert]

Post by Stewart Gaskell
It's very difficult to avoid "weird constructs"
when using propriety C++ extensions.

Exactly. Thus the advice to avoid using __property and use a
traditional getter/setter function instead. IMO, properties are of
value only in cases where they are needed by the form designer and
they must be handled with care whenever used in code - they may work
like normal variables much of the time, but when cornered they bite
back.

- Leo

[MoonlitKnight]

Post by Leo Siefert
Exactly. Thus the advice to avoid using __property and use a
traditional getter/setter function instead. IMO, properties are of
value only in cases where they are needed by the form designer and
they must be handled with care whenever used in code - they may work
like normal variables much of the time, but when cornered they bite
back.

Thanks for comments. This advice seems to suggest we should avoid VCL altogether - right? For example, how would I get the value of a clientdataset field without using one of its properties? This has nothing to do with the form designer - you have no choice but to use 'AsString' or other appropriate property (unless you roll your own VCL derivatives)!

Rgds
Antony

[Chris Uzdavinis (TeamB)]

Post by MoonlitKnight

Post by Leo Siefert
Exactly. Thus the advice to avoid using __property and use a
traditional getter/setter function instead. IMO, properties are of
value only in cases where they are needed by the form designer and
they must be handled with care whenever used in code - they may work
like normal variables much of the time, but when cornered they bite
back.

Thanks for comments. This advice seems to suggest we should avoid
VCL altogether - right? For example, how would I get the value of a
clientdataset field without using one of its properties? This has
nothing to do with the form designer - you have no choice but to use
'AsString' or other appropriate property (unless you roll your own
VCL derivatives)!

No. Well, some of the VCL *notations*, yes. But overall the VCL
offers a lot of value, rapid development, decoupling of the details of
the windowing system from what you really care about, etc.

The main thing I like to stress is to very strictly partition your
application into "pure C++" code, and "VCL code". Do not let them
bleed into each other, and keep the VCL code as small as is
reasonable. That is, unless it is strictly necessary, don't use
VCL-specific features when pure C++ offers another way to do it.
Anything VCL in C++ is inherently non-standard and subject to issues
such as (but not limited to)

1) strange interactions between the delphi and C++ object model (i.e.,
order of creation, virtual constructors, etc.)

2) non standard features have no specification. Hopefully they're
well documented. Historically, they haven't been.

3) nothing prevents them from changing in the future, as they're not
governed by any sort of committee

4) loss of portability

5) higher chance of compiler bugs, as there is no other implementation
to test against, and perhaps they hadn't thought of all the cases.
(For example, I don't like __finally blocks at all, but they're there
because Delphi programmers like them. What happens if you throw an
excpetion from it while there is an active exception? What happens
when you use a goto back into the "try" block? Etc. Not everything
is perfectly seamless in the integration, and there are some rough
edges.

These problems seem big, but if you contain the code that uses the VCL
to a small subsystem of your application off to the side, the effects
of the issues listed become far less of a big deal, and only affect
that subsystem.

If you have VCL-isms spread throughout your entire codebase then your
exposure to potential problems is much higher, and portability is not
possible without extensive rewriting.

[Leo Siefert]

Post by MoonlitKnight
This advice seems to suggest we should avoid VCL altogether - right?

Not at all - VCL is the most important thing offered by C++ Builder.
It facilitates building a user interface and is a great time-saver.

Post by MoonlitKnight
For example, how would I get the value of a clientdataset
field without using one of its properties?

If you choose to use ClientDataset, then yes, you will be using its
properties, since its design is to expose most of its functionality
through properties. But interacting with VCL controls is quite a
different thing from adding a property to one of your own classes. My
gui form interface code makes extensive use of the properties of the
controls on the form, but I have never added a property to any of my
own classes.

Post by MoonlitKnight
This has nothing to do with the form designer

Actually, it does. TClientDataset is designed as it is so that it can
be streamed as a part of a form (or data module) and can be
manipulated in the Object Inspector, which I consider to be an
extension of the form designer. If you look at a dfm file you will see
that even though it is not a visible control, it is treated exactly
the same as other form elements

Post by MoonlitKnight
(unless you roll your own VCL derivatives)!

For myself, I do not use TClientDataset. I use standard TEdit, TMemo,
etc. controls on my forms and handle my remote database access through
my own interface, but using Indy's TIdHTTP to establish my connection.
Because both the TIdHTTP and the form on the other side of my code
want AnsiString/TStrings data to work with, My code typedefs
TStringList and passes it through by reference.

For local database access, I use CodeBase which offers a standard C++
interface, but also can be used through a set of VCL TDataSet based
controls. For database controls, it does make sense to have them VCL
based since the non-visual ones work together with the visual ones
that show the data on a form. These are especially handy for quickly
putting together little utility apps, but often I find I want more
control for my larger apps.

For most other non-visual controls, I see little utility to having
them VCL based. I use the Indy controls not because they are VCL, but
because I have not found others that work as well and have failed in
attempts to build my own. I would prefer to have a straight C++
interface for such utilities.

- Leo

[unknown]

Post by Leo Siefert
If you choose to use ClientDataset, then yes, you will be using its
properties, since its design is to expose most of its functionality
through properties. But interacting with VCL controls is quite a
different thing from adding a property to one of your own classes. My
gui form interface code makes extensive use of the properties of the
controls on the form, but I have never added a property to any of my
own classes.

Thanks for your response. I simply added the property to TMyClass as a means of illustrating the code generation bug (this was a test case). Of course the real problem is that the bug also occurs in VCL classes with AsString properties. But whereas I can ensure that my own classes are free of such properties I can't do the same when using the VCL.


Rgds
Antony

[Leo Siefert]

Post by unknown
Of course the real problem is that the bug also
occurs in VCL classes with AsString properties.

Can you show an example of where this is a problem? I did try your
code and it runs without a problem for me and I can find no evidence
that anything is being improperly deleted. I added a destructor to
TMyClass that announces destruction and all I see is a single
destruction of TMyClass{"No"), as I would expect.

- Leo

[MoonlitKnight]

Post by Leo Siefert
Can you show an example of where this is a problem? I did try your
code and it runs without a problem for me and I can find no evidence
that anything is being improperly deleted. I added a destructor to
TMyClass that announces destruction and all I see is a single
destruction of TMyClass{"No"), as I would expect.

Hi Leo. Soemtimes the bug surfaces other times it doesn't - it depends upon what's contained in the registers. On one build I did recently, all works ok if I click the button with the mouse but crashes if I press CR on the keyboard! You won't see anything unusual when debugging the C++ code - you need to step through the CPU view to see the attempted deletion of non-existing AnsiStrings.

Rgds
Antony

[unknown]

Post by Leo Siefert

Post by unknown
Of course the real problem is that the bug also
occurs in VCL classes with AsString properties.

Can you show an example of where this is a problem? I did try your
code and it runs without a problem for me and I can find no evidence
that anything is being improperly deleted. I added a destructor to
TMyClass that announces destruction and all I see is a single
destruction of TMyClass{"No"), as I would expect.

I just tried it with 2006 and got the access violation, read of address 2.

First-if-test-fail jumps to 004022DF where you can pick up the trail.
Offending code starts at 0040231D.


if(SameText(TMyClass("No").AsString, "Yes") &&
SameText(TMyClass("Yes").AsString, "No"))

t1 = AnsiString("Yes") bp-10h
t2 = AnsiString("No") bp-4
t3 = TMyClass(bp-8,t2) bp-50h
t4 = AnsiString() bp-54h
AnsiString::operator = ( t4, t3 )
SameText( t4, t1 )
test fails
[skipped code]
flag = 0 !!
~AnsiString t1
~AnsiString bp-8 ? tMyClass.FStr ?
~AnsiString t2
test flag !!
missing jump on flag !!
~AnsiString bp-1ch Not Set! Set by second test
~AnsiString bp-0ch


Function Adresses:

00402DD8 __InitExceptBlockLDTC(void *):
00402350 TMyClass(const AnsiString &s) : FStr(s) { };
00401D6C System::AnsiString::AnsiString():
004036a6 rtl100.__fastcall Sysutils::SameText(const System::AnsiString,
const System::AnsiString):


LongPath1.cpp.150: void __fastcall TForm1::Button2Click(TObject *Sender)
004021C0 55 push ebp
004021C1 8BEC mov ebp,esp
004021C3 83C4A4 add esp,-$5c
004021C6 8955B4 mov [ebp-$4c],edx
004021C9 8945B8 mov [ebp-$48],eax
004021CC B8D4454000 mov eax,$004045d4
004021D1 E8020C0000 call $00402dd8

LongPath1.cpp.152: if(SameText(TMyClass("No").AsString, "Yes") &&
004021D6 BA85434000 mov edx,$00404385 "Yes"
004021DB 8D45F0 lea eax,[ebp-$10]
004021DE E89D0E0000 call System::AnsiString::AnsiString(const char *)
004021E3 FF45D8 inc dword ptr [ebp-$28]
004021E6 FF30 push dword ptr [eax] save for SameText()
004021E8 66C745CC0C00 mov word ptr [ebp-$34],$000c

004021EE BA82434000 mov edx,$00404382 "No"
004021F3 8D45FC lea eax,[ebp-$04]
004021F6 E8850E0000 call System::AnsiString::AnsiString(const char *)
004021FB FF45D8 inc dword ptr [ebp-$28]

004021FE 8D55FC lea edx,[ebp-$04]
00402201 52 push edx
00402202 8D4DF8 lea ecx,[ebp-$08]
00402205 51 push ecx
00402206 E845010000 call $00402350 TMyClass::TMyClass
0040220B 83C408 add esp,$08
0040220E 8345D802 add dword ptr [ebp-$28],$02
00402212 8945B0 mov [ebp-$50],eax

00402215 8D45F4 lea eax,[ebp-$0c]
00402218 E84FFBFFFF call $00401d6c AnsiString::AnsiString(AnsiString)
0040221D FF45D8 inc dword ptr [ebp-$28]
00402220 8945AC mov [ebp-$54],eax

00402223 8B55B0 mov edx,[ebp-$50]
00402226 8B45AC mov eax,[ebp-$54]
00402229 E83A0F0000 call System::AnsiString::operator =(const System::AnsiString &)
0040222E 8D45F4 lea eax,[ebp-$0c]
00402231 8B00 mov eax,[eax]
00402233 5A pop edx
00402234 E86D140000 call $004036a6 SameText
00402239 84C0 test al,al
0040223B 0F849E000000 jz $004022df Fail first test

00402241 BA82434000 mov edx,$00404382
00402246 8D45E0 lea eax,[ebp-$20]
00402249 E8320E0000 call System::AnsiString::AnsiString(const char *)
0040224E FF45D8 inc dword ptr [ebp-$28]
00402251 FF30 push dword ptr [eax]
00402253 BA85434000 mov edx,$00404385
00402258 8D45EC lea eax,[ebp-$14]
0040225B E8200E0000 call System::AnsiString::AnsiString(const char *)
00402260 FF45D8 inc dword ptr [ebp-$28]
00402263 8D4DEC lea ecx,[ebp-$14]
00402266 51 push ecx
00402267 8D45E8 lea eax,[ebp-$18]
0040226A 50 push eax
0040226B E8E0000000 call $00402350
00402270 83C408 add esp,$08
00402273 8345D802 add dword ptr [ebp-$28],$02
00402277 8945A8 mov [ebp-$58],eax
0040227A 8D45E4 lea eax,[ebp-$1c]
0040227D E8EAFAFFFF call $00401d6c
00402282 FF45D8 inc dword ptr [ebp-$28]
00402285 8945A4 mov [ebp-$5c],eax
00402288 8B55A8 mov edx,[ebp-$58]
0040228B 8B45A4 mov eax,[ebp-$5c]
0040228E E8D50E0000 call System::AnsiString::operator =(const System::AnsiString &)
00402293 8D45E4 lea eax,[ebp-$1c]
00402296 8B00 mov eax,[eax]
00402298 5A pop edx
00402299 E808140000 call $004036a6
0040229E 84C0 test al,al
004022A0 0F95C1 setnz cl
004022A3 83E101 and ecx,$01
004022A6 51 push ecx
004022A7 FF4DD8 dec dword ptr [ebp-$28]
004022AA 8D45E0 lea eax,[ebp-$20]
004022AD BA02000000 mov edx,$00000002
004022B2 E8810E0000 call System::AnsiString::~AnsiString()
004022B7 FF4DD8 dec dword ptr [ebp-$28]
004022BA FF4DD8 dec dword ptr [ebp-$28]
004022BD 8D45E8 lea eax,[ebp-$18]
004022C0 BA02000000 mov edx,$00000002
004022C5 E86E0E0000 call System::AnsiString::~AnsiString()
004022CA FF4DD8 dec dword ptr [ebp-$28]
004022CD 8D45EC lea eax,[ebp-$14]
004022D0 BA02000000 mov edx,$00000002
004022D5 E85E0E0000 call System::AnsiString::~AnsiString()
004022DA 59 pop ecx
004022DB 85C9 test ecx,ecx
004022DD 7504 jnz $004022e3

004022DF 33C0 xor eax,eax
004022E1 EB05 jmp $004022e8
004022E3 B801000000 mov eax,$00000001

004022E8 50 push eax
004022E9 FF4DD8 dec dword ptr [ebp-$28]
004022EC 8D45F0 lea eax,[ebp-$10]
004022EF BA02000000 mov edx,$00000002
004022F4 E83F0E0000 call System::AnsiString::~AnsiString()

004022F9 FF4DD8 dec dword ptr [ebp-$28]
004022FC FF4DD8 dec dword ptr [ebp-$28]
004022FF 8D45F8 lea eax,[ebp-$08]
00402302 BA02000000 mov edx,$00000002
00402307 E82C0E0000 call System::AnsiString::~AnsiString()

0040230C FF4DD8 dec dword ptr [ebp-$28]
0040230F 8D45FC lea eax,[ebp-$04]
00402312 BA02000000 mov edx,$00000002
00402317 E81C0E0000 call System::AnsiString::~AnsiString()

0040231C 59 pop ecx
0040231D 84C9 test cl,cl
Bad Test. Does nothing with result!
LongPath1.cpp.158: }
0040231F FF4DD8 dec dword ptr [ebp-$28]
00402322 8D45E4 lea eax,[ebp-$1c]
00402325 BA02000000 mov edx,$00000002
0040232A E8090E0000 call System::AnsiString::~AnsiString()
0040232F FF4DD8 dec dword ptr [ebp-$28]
00402332 8D45F4 lea eax,[ebp-$0c]
00402335 BA02000000 mov edx,$00000002
0040233A E8F90D0000 call System::AnsiString::~AnsiString()
0040233F 8B4DBC mov ecx,[ebp-$44]
00402342 64890D00000000 mov fs:[$00000000],ecx
LongPath1.cpp.158: }
00402349 8BE5 mov esp,ebp
0040234B 5D pop ebp
0040234C C3 ret

[Leo Siefert]

Post by unknown
I just tried it with 2006 and got the access violation, read of
address 2.

Ok, tried it in BDS2006 and I do gent an AV there, which would lead me
to think that the bug has been fixed, but OP indicated it was found in
CB2007 where I do not see an error.

Post by unknown
First-if-test-fail jumps to 004022DF where you can pick up the
trail. Offending code starts at 0040231D.

Sorry, but I do not have the time to wade through the assembly dump
right now. If it does produce bad code, though, it should not be too
difficult to build a project that clearly exposes the error.

- Leo

[MoonlitKnight]

Post by Leo Siefert
Ok, tried it in BDS2006 and I do gent an AV there, which would lead me
to think that the bug has been fixed, but OP indicated it was found in
CB2007 where I do not see an error.

No. The bug exists in CB2007 too. As I explained previously, the results of the bug are not predictable and sometimes appear to have no adverse affects at all. This is exactly what makes the bug particularly insidious.

Rgds
Antony

[Leo Siefert]

Post by MoonlitKnight
the results of the bug are not predictable

Do you mean that the code gen is sometimes correct and sometimes
incorrect? Will two builds with no other changes sometimes produce
different executables? If so I can try banging at something like that
when I have time.

Or do you just mean that the environment in which the exe is run
affects whether or not it errs? If that is so, and you can see the
code gen error clearly, then it should be possible to create an app
that always errs when running.

I do plan to look at this further when I have time, but I'm not a
CodeGear guy, and I work full time and have my own issues to deal with
as well. Any additional info added to the report or posted here would
be helpful.

- Leo

[MoonlitKnight]

Post by Leo Siefert
Or do you just mean that the environment in which the exe is run
affects whether or not it errs? If that is so, and you can see the
code gen error clearly, then it should be possible to create an app
that always errs when running.

Thanks for your help. The above is correct; the code generated is always in error but depending on how it is executed it may or may not cause problems (eg as explained previously if I click the button with the mouse it will work but if I press the button using the SPACE bar it causes an AV), so it's as you suggest - it depends on runtime values, state of stack etc.

Rgds
Antony

[AlexB]

Post by MoonlitKnight
Using CB2007, the compiler appears to generate code to delete
non-existing AnsiStrings, producing access violations or other
erratic behaviour at runtime.

...

Anyway please enter this test case into QC.