2

I 've got a base abstract class and 2 derived classes from base. When i get to call the function from the derived classes the program crashes!! Here's the code!

My base Class

class product{
protected:
    float ipsos,aktina;
    int n;
public:
    product(){};
    virtual float getvolume() =0;
};

My Derived class

class product1:public product{
    public:
       product1();
       float getakt(){return aktina;};
       float getips(){return ipsos;};
       float getvolume();
};

product1::product1(){
  //inputing aktina,ipsos,n
}
float product1::getvolume(void){
    return (3.14)*aktina*aktina*ipsos;
}

I 've got another 1 derived class that has another implemantation of getvolume().. Here's my main:

int main(){
int i;float v;
product1 *p1;
product2 *p2;


if((p1=(product1*)malloc(2*sizeof(product1)))==NULL){       
    cout <<"Not enough memory for 2"<< "objects" << endl;
    exit(1);
}

for(i=0;i<2;i++){
    product1 temp;
    p1[i]=temp;
}
cout<<p1[0].getakt()<<" "<<p1[0].getips();
v=p1[0].getvolume();
cout<<v;

return 1;

}

Programs does fine until it goes to v=p1[0].getvolume() where it crashes and i cant understand what the problem is!

Xenakis Karamanos
  • 25
  • 5

1 Answers1

3

The problem is that you are using malloc.

malloc is a C library function that allocates memory and returns a pointer to that memory. It does not call constructors and it does not set up virtual tables, so your program is entirely broken due to that: your call to the virtual function getVolume() cannot work properly.

In C++, especially when using runtime polymorphism, we use new instead.

When I change the following line:

if((p1=(product1*)malloc(2*sizeof(product1)))==NULL){

to:

if((p1=new product1[2])==NULL){

the segmentation fault goes away.

You will also have to delete[] this array at the end of your program, and you can see from my demos that the compiler is complaining (rather strangely, I might add) about uninitialised data members. You should fix those (and turn on compiler warnings).

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
  • better write explicitly to `delete[]` array. – mip Nov 30 '14 at 16:30
  • There's a small problem with your theory, in the fact that OP is using the derived class `product1` directly, hence no virtual functions are actually invoked, and the V-Table is not necessary. I started writing a similar answer, but found myself puzzled as soon as I realized that `p1[0].getvolume()` does not go through the V-Table. – barak manos Nov 30 '14 at 16:32
  • 1
    @barakmanos: `product1::getVolume()` is still a virtual function. The program doesn't know _until it queries the virtual table_ that there is no more-derived overrider to invoke. That's the point of the virtual table: to figure that out. Furthermore, my demonstrations _show_ the fix working. You'd be right if the OP were doing `p1[0].product1::getVolume()`, I think, but this is definitely a virtual call as it stands. – Lightness Races in Orbit Nov 30 '14 at 16:33
  • Why? The compiler identifies `p1` as a pointer to a `product1` object, and can therefore replace the function call with the address of function `product1::getVolume`. No need to replace it with code that queries the V-Table. – barak manos Nov 30 '14 at 16:36
  • @barakmanos: That's like saying that, in `product* p1 = new product1; p1->getVolume()`, "the compiler sees that `p1` is a pointer to a `product` object, and can therefore replace the function call with the address of function `product::getVolume`". The _whole point_ of runtime polymorphism is that the static type of the pointer is _insufficient_ and a vtable lookup must be made. – Lightness Races in Orbit Nov 30 '14 at 16:37
  • But it's not `product* p1`, it is `product1* p1`. – barak manos Nov 30 '14 at 16:38
  • 3
    @barakmanos: So? In general it cannot be known at compile-time that `p1` points to a `product1` — what if it points to some even-more-derived type? Granted in this contrived example the compiler could optimise the entire thing since the allocations is made in the same translation unit as the usage, but it's extremely unlikely to do so, and the OP's program has UB merely by not properly constructing the object. – Lightness Races in Orbit Nov 30 '14 at 16:38
  • If the function was virtual in `product1` **AND** there was another class inheriting from `product1`, then - yes, function-resolution during runtime would make sense. But in this case, the way I see it, the compiler can resolve the address of the function beforehand. – barak manos Nov 30 '14 at 16:40
  • @barakmanos: The function _is_ virtual in `product1`. And there could be a definition of some class inheriting from `product1` in another translation unit. – Lightness Races in Orbit Nov 30 '14 at 16:41
  • I m not that familiar with new but as i understand the for loop isnt needed after the new? – Xenakis Karamanos Nov 30 '14 at 16:42
  • Do you see that function `getvolume` is **not** `virtual` in class `product1`? Therefore, regardless of what `p1` would point to, the function `product1::getvolume` should be called. – barak manos Nov 30 '14 at 16:42
  • 3
    @barakmanos: Complete nonsense. Read your C++ book again, my friend! `[C++11: 10.3/2]:` _"If a virtual member function `vf` is declared in a class `Base` and in a class `Derived`, derived directly or indirectly from `Base`, a member function `vf` with the same name, parameter-type-list (8.3.5), cv-qualification, and ref-qualifier (or absence of same) as `Base::vf` is declared, then **`Derived::vf` is also virtual (whether or not it is so declared)** and it overrides `Base::vf`. [..]_" – Lightness Races in Orbit Nov 30 '14 at 16:43
  • @XenakisKaramanos: Yes, that is correct. I probably should have pointed that out. – Lightness Races in Orbit Nov 30 '14 at 16:44
  • Hmmmm... I think I'm gonna skip that (reading the book) and take your word for it... – barak manos Nov 30 '14 at 16:46
  • @barakmanos even if calls would be devirtualized, using it explicitly like so (with type punning malloc-ed memory) would render the whole thing UNDEFINED BEHAVIOUR anyways. And a smart compiler would not touch any of such evil code with an optimizer anyways – sehe Nov 30 '14 at 16:46
  • @LightnessRacesinOrbit Thank you very much!! Problem solved! Can u tell me how can i use new for: product **p; for 9 elements – Xenakis Karamanos Nov 30 '14 at 16:52
  • @sehe: Why? The allocated memory segment is the size of two `product1` instances. Their V-Tables are basically initialized in that `for` loop (in a very dubious way, still, by the time you exit the loop, the V-Tables are properly initialized to point to the V-Table of the `product1` class). And in any case, if the compiler could opt to resolve the address of that function (which I now understand it cannot), then there would be no need to use the V-Table anyway. – barak manos Nov 30 '14 at 16:54
  • @barakmanos: By the time you exit the loop, you've already assigned to two broken, invalid instances that were improperly allocated and not at all constructed. The damage is done. – Lightness Races in Orbit Nov 30 '14 at 16:55
  • @XenakisKaramanos: Sorry but I don't understand your new question; also please post new questions as new questions! After thinking about them for a while. – Lightness Races in Orbit Nov 30 '14 at 16:56
  • I'm not sure it matters. The values of those temporary (and also uninitialized) objects have already been copied into the array. No pointers involved, only `int` and `float` (and a valid V-Table). Although "broken", in practice it should do the job (like I said, in a very dubious way). – barak manos Nov 30 '14 at 16:57
  • @barakmanos: Well, notwithstanding your lack of certainty, it _does_ matter, as the OP's problem (and the demonstrated fix) shows. :) – Lightness Races in Orbit Nov 30 '14 at 16:58
  • Yep, I agree on that. If the V-Table pointer contains a valid value (of `product1`'s V-Table), then the call to the virtual function should not have crashed. It should be interesting to see if it is properly initialized inside the `for` loop, and then "junked out" to some invalid value. – barak manos Nov 30 '14 at 17:03
  • @barakmanos: I doubt that the default assignment op touches the virtual table at all. – Lightness Races in Orbit Nov 30 '14 at 17:07
  • Hmmmm, good point. I have assumed a `memcopy` of the entire object, but perhaps it skips the `vfptr`. – barak manos Nov 30 '14 at 17:20
  • @barakmanos: It's definitely not a `memcpy` of the entire object. `[C++11: 12.8/15]:` _"The implicitly-defined copy/move constructor for a non-union class `X` performs a memberwise copy/move of its bases and members. [..]_" There's no way in general to perform a safe memberwise copy of individual members by simply `memcpy`ing the whole lot in bulk. An optimised program _may_ perform the equivalent operation for a _trivially copyable_ type, I suppose... but this here ain't no _trivially copyable_ type. – Lightness Races in Orbit Nov 30 '14 at 17:22
  • Do you mean that even `memcpy(&p1[i],&temp,sizeof(product1))` wouldn't do the job??? – barak manos Nov 30 '14 at 17:29
  • @barakmanos yes I've tested it and seems that it done the job. – mip Nov 30 '14 at 17:36
  • I'm still playing with this. Looks like vtable pointer indeed is not copied by default copy assignment operator :o. So that's the deepest reason of this segfault. – mip Nov 30 '14 at 18:11
  • @barakmanos, @lightnessracesinorbit @sehe I'm still playing with this... And check this http://coliru.stacked-crooked.com/a/4a6cc2758bda5742 . With c++11 `final` keyword clang (and g++ also) devirtualizes the call instantly. Broken code works! Apparently initialized vtable is required to perform devirtualization when there is no `final` keyword. – mip Dec 01 '14 at 11:22
  • @doc: Yeah that makes sense. The program still has UB though so it could still break for any reason, even a nonsensical one. – Lightness Races in Orbit Dec 01 '14 at 12:41