Segfault avec des variables temporaires

Salut !

Je suis en train d’essayer de coder un interpréteur Lisp en C++ (C++14). J’ai créé une classe template pour stocker un arbre binaire, sauf que ça segfault un peu n’importe comment …

Voici mon main :

 1
 2
 3
 4
 5
 6
 7
 8
 9
10

int main()
{
    tree::ast t1{tree::ast{"lorem"}, tree::ast{"ipsum"}};    
    tree::ast t2{{"+"}, {{"a"}, {{{"-"}, {{"1"}, {{"2"}, {"nil"}}}}, {"nil"}}}};

    std::cout << t1 << std::endl << t2;

    std::cout << tree::ast{tree::ast{"lorem"}, tree::ast{"ipsum"}};    
    return 0;
}

Mon problème est que quand j’utilise des variables temporaires et que je les affiche (l.6) tout fonctionne alors que quand je passe directement le constructeur à l’affichage (l.8) ça segfault. Je n’arrive pas à poster la sortie ici dans une balise secret sans que ça fasse une bouillie de charactères alors tout est sur github :

La sortie quand je lance mon programme : https://github.com/Joh11/lisp-interpreter-v2/blob/master/log.txt

Et ma classe binary_tree (ast n’est qu’un typedef pour binary_tree<std::string>) : https://github.com/Joh11/lisp-interpreter-v2/blob/master/tree.hpp

En gros ma classe se présente sous cette forme :

  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177

namespace tree
{
   template <typename T>
   class binary_tree
   {
   public:
  // The enum to determine the type of the node
  enum class node_type {LEAF, BRANCH};

  // The two constructors to build a leaf and a branch
  binary_tree(T const& value = T()) : _value{value}, _nodeType{node_type::LEAF} {}
  
  binary_tree(binary_tree const& left, binary_tree const& right) : _children{new binary_tree{left}, new |binary_tree{right}}, _nodeType{node_type::BRANCH} {}

  // Copy constructor (1/5)
  binary_tree(binary_tree const& other) : _nodeType{other._nodeType}
  {
      // We have to deep copy (recursively)
      if(_nodeType == node_type::LEAF)
      _value = other._value;
      else
      {
      _children.left = new binary_tree{*other._children.left};
      _children.right = new binary_tree{*other._children.right};      
      }
  }

  // Copy assignment (2/5)
  // We cannot use copy and swap for this one because of ambiguity
  binary_tree & operator=(binary_tree const& other)
  {
      if(&other == this)
      return *this;
      
      auto copy{other};

      _nodeType = copy._nodeType;
      
      if(copy._nodeType == node_type::LEAF)
      std::swap(_value, copy._value);
      else
      {
      std::swap(_children.left, copy._children.left);
      std::swap(_children.right, copy._children.right);
      }

      return *this;
  }

  // Destructor (3/5)
  // We have to free the subtrees if it's a branch
  ~binary_tree()
  {
      if(_nodeType == node_type::BRANCH)
      {
      delete _children.left;
      delete _children.right;
      }
  }

  // Move constructor (4/5)
  binary_tree(binary_tree && other) : _nodeType{std::move(other._nodeType)}
  {
      if(_nodeType == node_type::LEAF)
      _value = std::move(other._value);
      else
      {
      _children.left = other._children.left;
      _children.right = other._children.right;


      // Set the other's children to nullptr to not delete our new children
      // We can do this because "delete nullptr;" is legit it seems
      other._children.left = nullptr;
      other._children.right = nullptr;
      }
  }

  // Move assignment operator (5/5)
  binary_tree & operator=(binary_tree && other)
  {
      _nodeType = std::move(other._nodeType);

      if(_nodeType == node_type::LEAF)
      _value = std::move(other._value);
      else
      {
      _children.left = other._children.left;
      _children.right = other._children.right;


      // Set the other's children to nullptr to not delete our new children
      // We can do this because "delete nullptr;" is legit it seems
      other._children.left = nullptr;
      other._children.right = nullptr;
      }

      return *this;
  }

  std::ostream & display(std::ostream & o, bool first = true) const
  {
      if(_nodeType == node_type::LEAF)
      return o << _value;

      if(_children.right->is_leaf() && _children.right->value() != "nil")
      {
      o << "(";
      _children.left->display(o);
      o << " . ";
      _children.right->display(o);
      return o << ")";
      }

      if(first)
      {
      o << "(";
      _children.left->display(o);

      if(_children.right->is_leaf() && _children.right->value() == "nil")
      {
          return o << ")";
      }
      
      o << " ";
      return _children.right->display(o, false);
      }
      else
      {
      if(_children.right->is_leaf() && _children.right->value() == "nil")
      {
          _children.left->display(o);
          return o << ")";
      }
      else
      {
          _children.left->display(o);
          o << " ";
          return _children.right->display(o, false);
      }
      }
  }

   private:
  union
  {
      // The leaf part
      T  _value;

      // The branch part
      struct
      {
      binary_tree *left;
      binary_tree *right;
      } _children;
  };

  node_type _nodeType;
   };

   // A function to display a binary tree
   // The template type has to be <<able
   template <typename T>
   std::ostream & operator<<(std::ostream & o, binary_tree<T> const& t)
   {
  return t.display(o);
   }

   // Aliases and type stuff
   using ast = binary_tree<std::string>;

   // Return (nil . nil)
   inline ast empty_branch()
   {
  return {{"nil"}, {"nil"}};
   }
}

Je soupçonne que c’est quelque chose avec le constructeur de copie, le constructeur de déplacement vu que c’est quand je n’utilise pas de variables temporaires que ça coince. J’ai essayé de ne pas définir le constructeur de déplacement et l’opérateur d’affectation par déplacement mais ça ne marche toujours pas…

Je suis sous Linux Mint avec GCC 5.4, avec comme options -g -Wall si ça change quelque chose.

Merci d’avance !

Ah oui effectivement je ne m’en était même pas douté j’ai pas beaucoup utilisé d’union avant…

Sauf que std::variant n’est possible sur gcc qu’avec la version 7 et que les string apportent quand même un confort non négligeable . Je pourrais aussi utiliser boost mais bon flemme …

Du coup j’ai viré l’union et j’ai tout mis en attribut, c’est pas deux pointeurs en plus qui devraient bouffer toute ma ram

Merci !

Au fait j’y pense c’est bizarre, le compilateur aurait dû me prévenir non ?

Lu’!

1
2
3
4
5
6

binary_tree(binary_tree const& left, binary_tree const& right) : _children{new binary_tree{left}, new |binary_tree{right}}, _nodeType{node_type::BRANCH} {}

//...

_children.left = new binary_tree{*other._children.left};
_children.right = new binary_tree{*other._children.right};

Pas de gestion correcte des exceptions : code incorrect.

Elle est header-only …

Ben surtout que de toute façon quand tu fais une union, tes pointeurs prennent cet espace puisque c’est tout le principe d’une union.

Ben non, la conversion est autorisée, au développeur de gérer cela correctement.

Salut merci d’avoir répondu

Pour la gestion des exceptions avec les new, il faut juste que je mette tout ça dans un bloc try qui attrappe les std::bad_alloc non ?

Mais du coup qu’est ce que j’en fait ? Si je ne peux pas instancier mes subtrees autant ne pas attraper les exceptions et laisser le programme planter non ? (question de pur amateur btw, pas taper )

Attraper std::bad_alloc n’a que peu d’intérêt AMHA (tu as plus de mémoire, tu veux faire quoi ? autant crasher).

Mais le problème se situe au niveau des leaks mémoire. Si l’initialisation de _children.right échoue, la mémoire de _children.left n’est pas libérée (délivrée). Utilise des pointeurs intelligents.

Si on suit ton raisonnement, si tu rencontre un quelconque problème dans l’allocation de _children.right, le programme va planter et la mémoire sera récupéré par l’OS. Donc pas de fuite mémoire. Mais d’une manière générale, je ne pense pas que ce soit une bonne idée de compter sur le plantage du programme pour régler ses soucies de fuites mémoire.

Ici l’idée est de faire un interpréteur, le minimum à faire si un allocation échoue, c’est de le dire à l’utilisateur de façon claire et précise : indiquer quel variable n’a pas pu être allouée, et dans quel fichier/ligne.

Utilise des pointeurs intelligents. Source:minirop

Je plussoie. D’ailleurs, je dirais même évite les pointeurs au maximum. Dans l’ordre tu devrais voir plus de 1. références, 2. unique_ptr, 3. shared_ptr, 4. weak_ptr, 5. raw pointeurs grosso modo.

Le raw-pointer n’est pas si bas dans la hiérarchie à mon sens . La première chose, c’est déjà de définir s’il y a responsabilité envers l’objet associé, et ensuite de s’assurer que cette responsabilité n’est jamais gérée à travers l’utilisation explicite de new et delete. Personnellement, je résumerais ça comme ça :

Est ce que je suis responsable de l’objet auquel je veux accéder ?

• Oui : suis-je l’unique responsable ?

  • Oui : l’objet est il polymorphe ou susceptible d’être changé ?

    • Oui : std::unique_ptr

    • Non : est-il optionnel ?

      • Oui : std::optional
      • Non : composition directe
  • Non : std::shared_ptr

• Non : l’objet est il optionnel ?

  • Oui : l’objet est il lié à un shared_ptr ?

    • Oui : std::weak_ptr
    • Non : raw-pointer

  • Non : l’objet est il lié à un objet d’une durée de vie incohérente avec la mienne ?

    • Oui : std::shared_ptr*

    • Non : l’objet peut-il être changé ?

      • Oui : std::reference_wrapper (ou pointeur nu si la syntaxe vous semble trop lourde)
      • Non : référence

* : En prenant P comme le point d’accès que nous sommes en train de définir et O le point qui référence déjà l’objet, cela peut impliquer de changer le choix précédemment fait pour O. Voir même de s’apercevoir que c’est finalement P qui est l’unique responsable de la ressource et que O devrait finalement être une référence.

Les notions en gras sont à mon sens celle que l’on devrait privilégier car elles amènent à une conception beaucoup plus simple qui rend la durée de vie des ressources beaucoup plus prévisible en plus de limiter la coûts à l’exécution.

(PS : évidemment, c’est surtout un avis personnel cette hiérarchie et elle est ouverte à discussion).