Random Storage_Error in Libadalang.Implementation.Inc_Ref during Libadalang.Rewriting

[dsauvage]

Environment
Debian GNU/Linux 10/11 x86_64
GNAT 12.1.2 (66b989b5)
Libadalang 22.0.0 (5f365aa4)

Bug description
After Libadalang.Rewriting.Apply is done successfully, calls to Libadalang.Rewriting.Unit are made to generate new source files.
However Storage_Error is raised in Libadalang.Implementation.Inc_Ref as it is called by Libadalang.Analysis.Wrap_Context.

Changing Analysis_Context_Type.Ref_Count type from Natural to aliased GNATCOLL.Atomic.Atomic_Counter, and updating Inc_Ref and Dec_Ref accordingly fixes the issue.

Note: This issue occurs randomly.

184001354-ref_count.patch.gz

[pmderodat]

Hello,

When successful, the Libadalang.Rewriting.Apply function destroys the Handle given to it, which invalidates all the unit and node handles that it owns. This means that calling Libadalang.Rewriting.Unit becomes illegal on related handles; doing so constitutes a use-after-free issue, which probably explains the non-deterministic error you are getting.

The correct thing to do is to perform all the calls to Libadalang.Rewriting.Unit before calling Libadalang.Rewriting.Apply, so that you don’t need rewriting handles anymore past that point. I haven’t tested your patch, but I suspect that Valgrind would keep detecting memory issues even with it.

We’ll enhance the documentation for Apply and Abort_Rewriting to make the lifetime rules clearer, thank you for reporting this issue!

[dsauvage]

Thanks for your feedback.

Yes, as in the libadalang repository file rewrite_self_arg.adb [1] from line 89,
we first remember which analysis units are to be rewritten, then we call Libadalang.Rewriting.Apply, and finally, on success we rewrite the corresponding files iterating on previously stored (and erroneous) units.

An update to this misleading file would be great.

[1] https://github.com/AdaCore/libadalang/blob/617556ae0e2a9bc37d7a95e1bd2da15c2993b872/contrib/AdaEurope2018/rewrite_self_arg.adb

[pmderodat]

Good point, thank you for pointing this out! Yes, we will definitely fix it.

[dsauvage]

Kind of an egg and chicken issue here, we can not perform the calls to Libadalang.Rewriting.Unit (as in rewrite_self_arg.adb lines 104,105,110) before the call to Libadalang.Rewriting.Apply.

• Retrieving the unit content (as in rewrite_self_arg.adb line 110) before the call to Libadalang.Rewriting.Apply returns the original unit content, and not the rewritten one
• If ever the unit name has been rewritten, then the original filename (retrieved before the call to 'Apply') would be erroneous

[pmderodat]

I don’t understand your first point: calling Libadalang.Rewriting.Unit before calling Apply lets you get Analysis_Unit values, and you can access the modified content of that unit after the call to Apply. This is what the rewrite_self_arg.adb example does. The content of the unit indeed has not changed before the call to Apply, which is by design. So the procedure to follow is:

1. Use the Libadalang.Rewriting API to modify sources as needed.
2. Use Libadalang.Rewriting.Unit in order to retrieve Analysis_Unit values for modified units.
3. Call Libadalang.Rewriting.Apply.
4. Use Libadalang.Analysis.Text or whatever API you need in order to extract data from modified units.

Can you clarify if you have tried this, and if you did and had problems, what these problems were?

Regarding your second point, renaming/deleting source files is not in the scope of the rewriting API, and anyway changing unit/source file mappings invalidates unit providers. Our recommendation would be to keep data on the side of your rewriting process to describe the file creation/renaming/deletion required, and apply these operations once the rewriting is done. After that, you will need to create a new unit provider/analysis context for the new set of analysis units.

[dsauvage]

Great thanks,
I got confused and thought Analysis_Unit values could not be used after the call to Apply.

As a summary;

• Unit_Rewriting_Handle values are not valid after Apply is called
• Analysis_Unit values should be retrieved before the call to Apply

A fix for Rewrite_Self_Arg could be:

   --  Write results
   declare
      Unit_Handles   : constant LAL_RW.Unit_Rewriting_Handle_Array
        := LAL_RW.Unit_Handles (Handle);
      --  Shall not be used after the call to Libadalang.Rewriting.Apply
      Analysis_Units :          Libadalang.Analysis.Analysis_Unit_Array
        (Unit_Handles'Range);
   begin
      for Index in Unit_Handles'Range loop
         --  Remember which analysis units are to be rewritten
         Analysis_Units (Index)
           := Libadalang.Rewriting.Unit (Unit_Handles (Index));
      end loop;

      declare
         Result : constant LAL_RW.Apply_Result := LAL_RW.Apply (Handle);
      begin
         if not Result.Success then
            TIO.Put_Line ("Error during rewriting...");
         end if;

         for Unit of Analysis_Units loop
            --  Go through all rewritten units and generate a ".new" source
            --  file to contain the rewritten sources.
            declare
               Filename      : constant String
                 := LAL.Get_Filename (Unit) & ".new";
               Charset       : constant String := LAL.Get_Charset (Unit);
               Content_Text  : constant Langkit_Support.Text.Text_Type :=
                 LAL.Text (Unit);
               --  Retreive rewritten text, 
               Content_Bytes : constant String :=
                 Langkit_Support.Text.Encode (Content_Text, Charset);
               --  and encode it using the same encoding
               --  as in the original file.
               Output_File : TIO.File_Type;
            begin
               TIO.Create (Output_File, TIO.Out_File, Filename);
               TIO.Put (Output_File, Content_Bytes);
               TIO.Close (Output_File);
            end;
         end loop;
      end;
   end;

[pmderodat]

Exactly! Actually we had an update for rewrite_self_arg.adb in the pipe yesterday, and it was merged at the same time as you wrote your last message. Considering that all is fine for now, I’m now closing this issue. Thank you again for reporting this!