flyx / OpenGLAda

Thick Ada binding for OpenGL and GLFW
flyx.github.io/OpenGLAda/
MIT License
96 stars 13 forks source link

Link Problem with GNAT 2018 #127

Closed rogermc2 closed 6 years ago

rogermc2 commented 6 years ago

I have recently installed GNAT 2018 but my examples no longer build as they fail with:

gprbuild -d -P/Ada_Source/OpenGLAda/opengl.gpr -XLibrary_Type=static -XAuto_Exceptions=enabled -XMode=debug -XWindowing_System=quartz
Compile
   [ada]          gl-api-mac_os_x.adb
gnat1: invalid switch: -gnat05
gprbuild: *** compilation phase failed
[2018-06-17 17:11:48] process exited with status 4, 1% (1/57), elapsed time: 00.85s

I don't think that I can correct this on my own system as the -gnat05 switch appears to be inherited from opengl.gpr etc..

flyx commented 6 years ago

According to this, AdaCore no longer supports Ada 2005 with its new Community Edition. My advice would be to switch to the GnuAda release instead (has not yet been updated to AdaCore's 2018 release).

rogermc2 commented 6 years ago

I installed gcc-8.1.0-x86_64-apple-darwin15-bin but builds fail with:

Builder results (6 items in 1 file)
    /Ada_Source/OpenGLAda/src/gl/implementation/gl-objects-textures.adb
        544:7 warning: in instantiation at gl-enums-indexes.ads:17
        544:7 warning: cannot call "Get_Max" before body seen
        544:7 warning: Program_Error will be raised at run time
        553:7 warning: cannot call "Get_Max" before body seen
        553:7 warning: in instantiation at gl-enums-indexes.ads:17
        553:7 warning: Program_Error will be raised at run time
flyx commented 6 years ago

Known issue, someone else reported that via email. Should have never compiled. I'll try to fix it soon.

rogermc2 commented 6 years ago

OK. I am currently looking at a problem regarding passing strings from Ada to C++ that I found and solved on another project and that might have some bearing on the Varyings problem.

flyx commented 6 years ago

That would be great. I figured I would craft a release even though the Varyings problem still lingers, so that other features are out and I can release Windows binaries.

rogermc2 commented 6 years ago

Preliminary information (part of my attempt to write Ada interfaces to ImageMagick; treat with great caution): For procedure Read_File (theImage : out Magick_Image.API.Class_Image.MPP_Image; File_Name : String), instructions like theImage.Read (To_C (File_Name)); and theImage.Read (To_C ("logo:")); seem to lose the first character of File_Name and ../logo: when sent to C++. For example, on the C++ side, logo: ends up as ogo:.

My solution at the moment is to add an additional character at the start of the string: theImage.Read (To_C (" " & File_Name)); theImage.Read (To_C (" ../logo: "));

C++ interface:

package Magick_Image.API is
   package Class_Image is
      type MPP_Image is tagged limited record
         Ref : access Image_ReferencePP.Class_Image_Ref.Image_Ref;
      end record;
      pragma Import (Cpp, MPP_Image);

      procedure Read (this : in out MPP_Image;  File_Name : Interfaces.C.char_array); 
      pragma Import (Cpp, Read, "_ZN6Magick5Image4readERKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE");
   end Class_Image;
   use Class_Image;  --  I don't know if this really needed?
end Magick_Image.API;

I have managed to build ImageMagick with debug (-g) which allows me to track calls through the C++ code and also to include printf statements where helpful. 'printf' allowed me to identify the missing first character problem.

rogermc2 commented 6 years ago

The problem does not occur with

theImage.Write ("../logo.png");

so perhaps a problem with To_C?

For File_Name = logo.png the problem occurs using theImage.Write (To_C (File_Name)); which generates a file named ogo.png This is corrected with: theImage.Write (To_C (" " & File_Name));

rogermc2 commented 6 years ago

The same problem and solution occurs with New_String.

flyx commented 6 years ago

A minimal example (one Ada file, one C/C++ file) which exhibits this behavior would be a great help. This seems like a compiler bug, because it definitely should not happen and I definitely do not want to depend on a compiler bug.

rogermc2 commented 6 years ago

My Ada Example and highest level Ada Interface files. Let me know if you need others of my Ada Interface files.

example_1.adb.zip Archive.zip

The C++ file. I didn't implement image.crop in Ada. main.cpp.zip

rogermc2 commented 6 years ago

If it is a compiler bug, I think its in all of AdaCore GNAT 2017, 2018 and GCC 8.1.0. If you can confirm that it is a compiler bug, who should I report it to?

rogermc2 commented 6 years ago

If you need it, attached is my current Ada interface for ImageMagick. Very much a work-in-progress and probably full of inadequacies. Archive.zip

My ImageMagck build notes, in case you are interested: ImageMagick build.rtf.zip

flyx commented 6 years ago

I did a quick test with a minimal example:

/* c_test.c */
#include<stdio.h>

void c_test(const char value[]) {
  puts(value);
}
-- toc_test.adb
with Interfaces.C; use Interfaces.C;

procedure ToC_Test is
   procedure C_Test (Value : char_array) with Import,
     Convention => C, External_Name => "c_test";
begin
   C_Test (To_C ("This is a test"));
end ToC_Test;
-- toc_test.gpr
project ToC_Test is
   for Source_Files use ("toc_test.adb", "c_test.c");
   for Main use ("toc_test.adb");
   for Languages use ("Ada", "C");
end ToC_Test;

This code does not exhibit any abnormal behavior, the string is correctly shown. So it looks like your problem is not at To_C, but somewhere else.

flyx commented 6 years ago

I did some further testing by putting OpenGLAda's varyings code into the test procedure:

with Interfaces.C; use Interfaces.C;

procedure ToC_Test is
   Varyings : constant String := "first,second,third";
   type Char_Access_Array is array (size_t range <>) of access char with
     Convention => C;

   procedure C_Test (Length : Interfaces.C.size_t; Values : Char_Access_Array)
     with Import, Convention => C, External_Name => "c_test";

   Parameter_Buffer : Interfaces.C.char_array :=
      Interfaces.C.To_C (Varyings);
   Pointer_Array : Char_Access_Array (1 .. Parameter_Buffer'Length / 2);
      -- cannot be longer than this if every name has a length of at least 1
   Pointer_Count : size_t := 0;
   Recent_Was_Comma : Boolean := True;
begin
   for Pos in Parameter_Buffer'First .. Parameter_Buffer'Last - 1 loop
      if Parameter_Buffer (Pos) = ',' then
         if Recent_Was_Comma then
            raise Constraint_Error with
               "every varying name must have at least one character";
         end if;
         Parameter_Buffer (Pos) := Interfaces.C.nul;
         Recent_Was_Comma := True;
      elsif Recent_Was_Comma then
         Pointer_Count := Pointer_Count + 1;
         Pointer_Array (Pointer_Count) :=
            Parameter_Buffer (Pos)'Unchecked_Access;
         Recent_Was_Comma := False;
      end if;
   end loop;
   C_Test (Pointer_Count, Pointer_Array);
end ToC_Test;

C code is now:

#include<stdio.h>
#include<string.h>

void c_test(size_t length, const char* values[]) {
  for(size_t i = 0; i < length; i++) {
    puts(values[i]);
  }
}

That outputs

first
second
third

as expected. I have yet to see an instance of your problem happening on my machine. btw, I cannot compile the code you uploaded, there are missing files. Could you put everything into a folder, make sure it compiles as it is, and then upload it again?

flyx commented 6 years ago

btw, I just pushed a fix for the GCC 8.1.0 compiler errors.

rogermc2 commented 6 years ago

The Ada code: Ada_Magick.zip The C++ Xcode project: ImageMagick_Test.zip

rogermc2 commented 6 years ago

ImageMagic files in case you need them. images.zip Magick++.zip MagickCore.zip

rogermc2 commented 6 years ago

Your quick test with a minimal example also works on my system. Thanks for confirming that the problem does not seem to be with To_C. I will investigate further. Your assistance is much appreciated.

Thanks for the GCC 8.1.0 fixes. Unfortunately, I still can't build my examples. I get failure messages stating that my example source files and common files have been compiled with different GNAT versions, which seems ridiculous as they are all compiled in the same build. I have also tried cleaning and rebuilding common.

error: "initialize.adb" and "colour_fragments.adb" compiled with different GNAT versions
error: "utilities.adb" and "colour_fragments.adb" compiled with different GNAT versions
error: "maths.adb" and "colour_fragments.adb" compiled with different GNAT versions
error: "quaternions.adb" and "colour_fragments.adb" compiled with different GNAT versions
error: "program_loader.adb" and "colour_fragments.adb" compiled with different GNAT versions
error: "initialize.adb" must be recompiled ("ada.ads" has been modified)
error: "utilities.adb" must be recompiled ("ada.ads" has been modified)
error: "maths.adb" must be recompiled ("ada.ads" has been modified)
error: "quaternions.adb" must be recompiled ("ada.ads" has been modified)
error: "program_loader.adb" must be recompiled ("ada.ads" has been modified)
rogermc2 commented 6 years ago

Your quick test with a minimal example is C code. I wonder if the problem is to do with the C++ interface mangling?

rogermc2 commented 6 years ago

My quick test with a minimal example is C++ code based on my normal approach also works OK! I'll see if I can use this test as the basis for fixing my problem code. CPP_Test.zip

rogermc2 commented 6 years ago

The attached code is the closest analysis that I can find after quite a few hours of investigation. It seems like a problem with the mangling? Interesting results if To_C is not used like theImage.Write ("logo.png"); The generated file name is a concatenation of strings. For example, CPP_Test ("Testing"); as the last statement produces: Testing logo:logo.pngpogoC.png

IM_CPP_Test.zip

flyx commented 6 years ago

Try a gprclean -r on the main project to get rid of the error messages. Make sure that no traces of the old compiler remain.

rogermc2 commented 6 years ago

I now have the C++ code working. The problem was that the GNAT 2018 version of gprbuild needs to use LLVM compilers. However, the gprconfig did not include code for LLVM C++ compiler so I could not build a suitable cpgr file. AdaCore have now remedied that. The latest gprbuild needs to be downloaded from AdaCore, rebuilt and reinstalled. 'gprconfigcan then buildconfigfiles forLLVM C++compilers. With regard to my Ada programs, I think I am close to the solution. I have been passing filenames asCstrings whereas I thinkC++ std::strings` are required. So I need to fix that.

flyx commented 6 years ago

Be aware that you cannot pass an Ada String to a C++ parameter of type std::string. In Ada, String is an array of Characters. In C++, a std::string is a managed pointer to growable heap memory containing a list of characters. You would need to define the internal structure of C++'s std::string in Ada to be able to call a function taking an std::string.

rogermc2 commented 6 years ago

Many thanks, I was coming to that conclusion. Is it possible obtain a description of the internal structure of C++'s std::string? If so, can you provide some advice on how to proceed?

flyx commented 6 years ago

I am pretty sure that figuring out how to call a C++ function taking an std::string in Ada is irrelevant to the actual issue. You can still pass a String (Ada) and receive a char* (C++). The only thing different is that you should use the convention Cpp on the Ada side.

rogermc2 commented 6 years ago

I tried

    procedure Read (this : in out MPP_Image;
                      File_Name : String);
      pragma Import (Cpp, Read, "_ZN6Magick5Image4readERKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE");

but the program crashes immediately with

MacBook-Air:CPP_Test_7_Ada rogermcmurtrie$ ./copy_7 
raised CONSTRAINT_ERROR : erroneous memory access

The same occurs using the debugger:

(gdb) start 
Temporary breakpoint 3 at 0x10000140d: file /OpenGL_Projects/ogldev/IM_Test/CPP_Copy_Tests/CPP_Test_7_Ada/src/Copy_7.adb, line 12.
Starting program: /OpenGL_Projects/ogldev/IM_Test/CPP_Copy_Tests/CPP_Test_7_Ada/copy_7 
[New Thread 0xc03 of process 1901]
[New Thread 0xe03 of process 1901]
During startup program terminated with signal SIGTRAP, Trace/breakpoint trap.

I agree that this problem is probably irrelevant to the the Varyings problem.

rogermc2 commented 6 years ago

I overcame the startup program termination problem by prefixing my input string with a space character:

procedure Read_File (theImage : out Magick_Image.API.Class_Image.MPP_Image; 
 File_Name : String) is
...
theImage.Read (" " & File_Name);

The test program then operates and fails as before. I'm investigating further using String with the binder:

procedure Read (this : in out MPP_Image; File_Name : String);
      pragma Import (Cpp, Read, "_ZN6Magick5Image4readERKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE");

However, although the program runs 'gdb`, which worked previously, now fails with:

(gdb) run
Starting program: /OpenGL_Projects/ogldev/IM_Test/CPP_Copy_Tests/CPP_Test_7_Ada/copy_7 
[New Thread 0x1703 of process 2502]
[New Thread 0x1903 of process 2502]
During startup program terminated with signal ?, Unknown signal.

when run directly, the result is

MacBook-Air:CPP_Test_7_Ada rogermcmurtrie$ ./copy_7 
Magick::Image::read Filename /OpenGL_Projects/ogldev/Content/bricks.jpg??  
Options.cpp filename, filename: /OpenGL_Projects/ogldev/Content/bricks.jpg??
     filename Length 16, strlen 45

The ?? have only appeared since I introduced String and as a parameter into the binder. The problem is indicated by filename Length 16 which is generated by fileName_.length() while strlen 45 is generated by strlen(fileName_.c_str()). Any string longer than 16 characters produces filename Length 16 while strlen produces the correct number of characters. Any advice appreciated, but if you don't have time' don't worry.

rogermc2 commented 6 years ago

Latest success? I preceded filename with a \ instead of a space and this seems to have improved matters:

MacBook-Air:CPP_Test_7_Ada rogermcmurtrie$ ./copy_7 
Magick::Image::read Filename /OpenGL_Projects/ogldev/Content/bricks.jpg?  
Options.cpp filename, filename: /OpenGL_Projects/ogldev/Content/bricks.jpg?
     filename Length 46, strlen 45
Options.cpp filename, fileName_ length 46
fileName_ characters:
/ O p e n G L _ P r o j e c t s / o g l d e v / C o n t e n t / b r i c k s . j p g  ?   

Previously only the first 16 fileName_ characters were printed and Options.cpp filename, fileName_ length 46 was 16. The ? at the end of the filename is an artefact that makes the filename invalid.

flyx commented 6 years ago

You are still passing your Ada String to a C++ std::string object. That cannot work. My guess at what happens:

You are passing in a String. Since Read has Cpp convention, GNAT compiles this call into passing a pointer to the first character of the string, since it assumes that a char* is taking the value on the C++ side.

Now C++ expects an std::string. I don't know its implementation, but from what you write, I would assume the following: It probably looks something like this:

template<typename C>
struct basic_string {
  C* ptr;
  size_t length, capacity;
};

So you have first a pointer to the actual string, and after that, the current length of the string's content and the current capacity of allocated memory which ptr points to. The actual implementation is probably more complicated, but you get the idea.

So the C++ method gets as parameter a single pointer to your Ada String, but expects more. The pointer to your Ada String is stored in ptr and length and capacity get garbage values because you did not pass enough data.

The, when you do strlen on the value, it starts at *ptr and continues to read data until it encounters a null byte. Since you did not use To_C, your String data is not terminated with a null byte and therefore, you get some garbage at the end that happens to be there, possibly from previous operations in your Ada code.

Instead, when you call .length(), it reads the length attribute of the std::string object it gets. Since you did not provide this value, again, this will be garbage – whatever value was written to that memory when it was last used.

Now, to answer the question why you need to prefix your path: I would guess that ptr does not directly point to an array of characters, but rather to something like this:

struct inner {
  unsigned char refcount;
  char[] data;
};

This is an educated guess based on the knowledge that some std::string implementations are implemented with reference counting, to avoid copying the data every time a string gets assigned and to copy-on-write. So your space in front of the data would become the refcount which is why it does not show up in the string on the C++ side.

So long story short: Do not wrap C++ functions taking an std::string. It does not work. You cannot know the implementation because it is implementation-defined. clang's implementation probably differs from gcc's implementation and so on.

rogermc2 commented 6 years ago

Thanks very much for your detailed discussion which I will study and try to think of a way ahead. "Do not wrap C++ functions taking an std::string. It does not work" is very good advice. I've posted a question on google.ada group in case someone knows how to do it. Currently, I'm looking at writing an intermediate C++ routine to covert C strings to std::strings and writing a wrapper for the converter function.

rogermc2 commented 6 years ago

Google Ada group returned this which looks promising: https://github.com/Lucretia/test_binding

rogermc2 commented 6 years ago

My intermediate C++ routine, which is quite simple, seems to have done the job. Filenames are now properly interpreted by the ImageMagick functions.

flyx commented 6 years ago

Did you figure out whether this problem had any relation to the varyings problem?

rogermc2 commented 6 years ago

My Image read/write problem does not appear to have any relation to the varyings problem. There appears to have been two parts to the Image read/write problem. One problem requiring the LLVM C++ compiler for successful compilation and linking of some ImageMagick routines and the other problem requiring the correct initialization of std::string from an Ada string by first converting the Ada string to a C string and then using this to initialize a std::string via its constructor. For example:

      procedure Read (this : in out MPP_Image;
                      File_Name : Interfaces.C.Strings.chars_ptr);
      pragma Import (Cpp, Read, "readFile");
void readFile (Magick::Image& anImage, char* fileName)
  {
  std::string cppString (fileName);
  anImage.read (cppString);
  }

My test program now works in that it reads a 'jpg' file into an image record and then writes the image to a png file which displays the correct image via Preview. I checked that the LLVM compilers do not the fix varyings problem.