Matgen84 Posted February 17, 2020 Share Posted February 17, 2020 Hi @Jief_Machak I've Debug mode at Clover start, I think is temporary as you says before. The transition time in c +++ Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 17, 2020 Author Share Posted February 17, 2020 It's sure is ! :-) If it's working for you, comment line from main.cpp from 2125 to 2158, and change #define DEBUG_MAIN 2 to #define DEBUG_MAIN 1 line 52. I'd like to have at least 4/5 users that successfully compile this new version, checking the C++ globals works and I'll remove it. Thanks for the test. 1 Link to comment Share on other sites More sharing options...
Matgen84 Posted February 17, 2020 Share Posted February 17, 2020 (edited) 10 minutes ago, Jief_Machak said: It's sure is ! :-) If it's working for you, comment line from main.cpp from 2125 to 2158, and change #define DEBUG_MAIN 2 to #define DEBUG_MAIN 1 line 52. I'd like to have at least 4/5 users that successfully compile this new version, checking the C++ globals works and I'll remove it. Thanks for the test. Some lines in main.cpp seems to be already commented. Or I understand well // sign So I comment 2149-2150, is it enough to revert debug commit? Le met know. Thanks for your job. Spoiler { // XStringW str(L"local str value"); // DBG("str = %s\n", str.data()); // str.StrCat(L" appended text"); // DBG("str = %s, len=%d\n", str.data(), str.length()); // // XStringW str2(str); // DBG("str2 = %s\n", str2.data()); // str2.StrnCpy(str.data(), 2); // DBG("str2 = %s\n", str2.data()); // str2.StrnCat(L"2ndtext", 2); // DBG("str2 = %s\n", str2.data()); // str2.Insert(1, str); // DBG("str2 = %s\n", str2.data()); // str2 += L"3rdtext"; // DBG("str2 = %s\n", str2.data()); // // XStringW* str3 = new XStringW(); // *str3 = L"str3data"; // DBG("str3 = %s\n", str3->data()); // delete str3; } // destruct_globals_objects(NULL); // That should be done just before quitting clover module. Now, it's just for test. DBG("press"); PauseForKey(L"press"); //dumping SETTING structure // if you change something in Platform.h, please uncomment and test that all offsets // are natural aligned i.e. pointers are 8 bytes aligned /* DBG("Settings offsets:\n"); Edited February 17, 2020 by Matgen84 Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 17, 2020 Author Share Posted February 17, 2020 Yes, some lines are commented. Different tests I made while I was exploring the C++ globals initialisation. All that will go away soon. This is what you comment out. construct_globals_objects(); // do this after SelfLoadedImage is initialized DBG("g_str = %s\n", g_str.data()); DBG("g_str2 = %s\n", g_str2.data()); extern XStringW global_str1; DBG("global_str1 = %s\n", global_str1.data()); extern XStringW global_str2; DBG("global_str2 = %s\n", global_str2.data()); { // XStringW str(L"local str value"); // DBG("str = %s\n", str.data()); // str.StrCat(L" appended text"); // DBG("str = %s, len=%d\n", str.data(), str.length()); // // XStringW str2(str); // DBG("str2 = %s\n", str2.data()); // str2.StrnCpy(str.data(), 2); // DBG("str2 = %s\n", str2.data()); // str2.StrnCat(L"2ndtext", 2); // DBG("str2 = %s\n", str2.data()); // str2.Insert(1, str); // DBG("str2 = %s\n", str2.data()); // str2 += L"3rdtext"; // DBG("str2 = %s\n", str2.data()); // // XStringW* str3 = new XStringW(); // *str3 = L"str3data"; // DBG("str3 = %s\n", str3->data()); // delete str3; } // destruct_globals_objects(NULL); // That should be done just before quitting clover module. Now, it's just for test. DBG("press"); PauseForKey(L"press"); 1 Link to comment Share on other sites More sharing options...
Slice Posted February 17, 2020 Share Posted February 17, 2020 I am trying to compile under windows, VS2017 toolset. It fails. "C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\bin\Hostx86\x64\cl.exe" /Fod:\projects\clover\Build\Clover\RELEASE_VS2017\X64\Library\VBoxPeCoffLib\VBoxPeCoffLib\OUTPUT\.\ /nologo /c /WX /GS- /W4 /Gs32768 /D UNICODE /O1b2s /GL /Gy /FIAutoGen.h /EHs-c- /GR- /GF /Gw /MP /FAcs /FRd:\projects\clover\Build\Clover\RELEASE_VS2017\X64\Library\VBoxPeCoffLib\VBoxPeCoffLib\OUTPUT\BasePeCoff.SBR -DMDEPKG_NDEBUG -DCLOVER_BUILD -DAMD_SUPPORT -DANDX86 -DLODEPNG -Dinline=__inline /Id:\projects\clover\Library\VBoxPeCoffLib /Id:\projects\clover\Build\Clover\RELEASE_VS2017\X64\Library\VBoxPeCoffLib\VBoxPeCoffLib\DEBUG /Id:\projects\clover /Id:\projects\clover\Include /Id:\projects\clover\Library\OpensslLib\Include /Id:\projects\clover\Library\OpensslLib\openssl-1.0.1e\include /Id:\projects\clover\MdePkg /Id:\projects\clover\MdePkg\Include /Id:\projects\clover\MdePkg\Include\X64 d:\projects\clover\Library\VBoxPeCoffLib\PeCoffLoaderEx.c d:\projects\clover\Library\VBoxPeCoffLib\BasePeCoff.c Building ... d:\projects\clover\MdePkg\Library\BasePeCoffExtraActionLibNull\BasePeCoffExtraActionLibNull.inf [X64] Building ... d:\projects\clover\MdePkg\Library\UefiBootServicesTableLib\UefiBootServicesTableLib.inf [X64] cl: ப error D8036: "/FRd:\projects\clover\Build\Clover\RELEASE_VS2017\X64\Library\VBoxPeCoffLib\VBoxPeCoffLib\OUTPUT\BasePeCoff.SBR" ᯮ 쪨 室묨 䠩 Building ... d:\projects\clover\MdePkg\Library\BaseMemoryLib\BaseMemoryLib.inf [X64] NMAKE : fatal error U1077: "C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\bin\Hostx86\x64\cl.exe" : "0x2" Stop. It worked a time ago but I am not sure if the problem related to latest changes. Microsoft says that error D8036 is '/option' not allowed with multiple source files These compiler options cannot be used with multiple source files: Name assembly file listing (/Fa) Rename object file (/Fo) Create source browser information without local variables (/Fr) Create source browser information that includes local variables (/FR) What is it? EDK2 developers uses VS. Link to comment Share on other sites More sharing options...
Slice Posted February 17, 2020 Share Posted February 17, 2020 May be we need a rule for cpp files in MSVC toolset? Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 17, 2020 Author Share Posted February 17, 2020 I tried before switching to C++. I think some VS2017 got restricted. That's why we probably need a updated tools_def.template. Maybe get all the definition for that toolset from the latest tools_def.template from edkII. Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 17, 2020 Author Share Posted February 17, 2020 (edited) I've committed the final step to switch to C++ compiler. I've tested gcc53, xcode5 and xcode8 compilation. Tomorrow, I'll remove the debug code displayed when you launch. Edited February 17, 2020 by Jief_Machak 2 Link to comment Share on other sites More sharing options...
chris1111 Posted February 17, 2020 Share Posted February 17, 2020 (edited) Starting new build with the latest change that's not look good here What is this ???? BuildEnv.sh not exist Lots of other file not exist ?? EDIT *** End the Build package failed Edited February 17, 2020 by chris1111 Link to comment Share on other sites More sharing options...
vector sigma Posted February 17, 2020 Share Posted February 17, 2020 buildme should autodetect an old setup and clean the enviroment automatically for c++ Clover. All fine here. 1 Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 18, 2020 Author Share Posted February 18, 2020 For this final switch, I had to add a LOT of CONST, because one the difference with C, is that C++ enforce that litteral are read only (sure it can be "deactivated"). adding a CONST somewhere doesn't change the compiled code. It's just a check at compiled time. But in 2 places, a little code modification was needed. One of this place is smbios,c when updating vendor name for RAM. Could that be related ? Have a look to the difference I've committed in smbios.c. Maybe I understood incorrectly how UpdateSmbiosString works. Link to comment Share on other sites More sharing options...
apianti Posted February 18, 2020 Share Posted February 18, 2020 (edited) 11 hours ago, Slice said: EDKII tools_def.template is not reliable for us. We edited it many times. As well I can tell that the windows compilation worked for me a month ago on the same computer. I use the default templates in windows always, I never have any issues, I removed the template overrides in windows a long time ago so they should be default unless you have never removed them since I changed that. There are a bunch of hard coded paths, missing call keywords, and other things in the script. It does not work correctly on my computer, and hasn't for a very long time, I just fix it but never pushed it. 53 minutes ago, Slice said: The new Clover compiled with GCC53 works on QEMU but hangs on real hardware scanning SPD. I didn't catch the reason yet. Could be weird optimization or something. Sometimes build cleanall, will fix these issues, sometimes it's something else. 35 minutes ago, Jief_Machak said: For this final switch, I had to add a LOT of CONST, because one the difference with C, is that C++ enforce that litteral are read only (sure it can be "deactivated"). adding a CONST somewhere doesn't change the compiled code. It's just a check at compiled time. But in 2 places, a little code modification was needed. Uh, no, the behavior is different than C (which declares as external linkage by default) unless you add one or more additional qualifiers (depending on the specification compliance): Any of the following names declared at namespace scope have internal linkage: ... non-volatile non-template (since C++14) non-inline (since C++17) non-exported (since C++20) const-qualified variables (including constexpr) that aren't declared extern and aren't previously declared to have external linkage; 35 minutes ago, Jief_Machak said: One of this place is smbios,c when updating vendor name for RAM. Could that be related ? Have a look to the difference I've committed in smbios.c. Maybe I understood incorrectly how UpdateSmbiosString works. That is entirely possible. Edited February 18, 2020 by apianti Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 18, 2020 Author Share Posted February 18, 2020 7 hours ago, chris1111 said: End the Build package failed clover-genconfig wasn't building because of 'extern "C"'. Fixed. 2 Link to comment Share on other sites More sharing options...
Slice Posted February 18, 2020 Share Posted February 18, 2020 SPD scan debug.log finished at 7:078 0:003 0000 1FD spd read status 41 7:082 0:004 0000 1FE spd read status 41 7:086 0:003 0000 1FF spd read status 41 7:090 0:003 00. 7:094 0:003 00 80 spd page change status 41 7:095 0:001 01 80 spd page change status 44 7:099 0:003 spd page change error for byte 50:128! 7:101 0:001 00 80 spd read status 41 7:103 0:001 00 81 spd read status 41 7:106 0:002 00 82 spd read status 41 7:108 0:001 00 83 spd read status 41 7:110 0:001 00 84 spd read status 41 7:112 0:001 00 85 spd read status 41 7:113 0:001 00 86 spd read status 41 7:116 0:002 00 87 spd read status 41 7:118 0:001 00 88 spd read status 41 7:120 0:001 00 89 spd read status 41 7:122 0:001 00 8A spd read status 41 7:132 0:010 00 8B spd read status 41 7:134 0:001 00 8C spd read status 41 7:138 0:003 00 8D spd read status 41 7:139 0:001 00 8E spd read status 41 7:141 0:001 00 8F spd read status 41 7:143 0:001 00 90 spd read status 41 7:145 0:001 00 91 spd read status 41 7:147 0:001 00 92 spd read status 41 0:100 0:100 MemLog inited, TSC freq: 2591585259 It means the procedure hangs before DBG("DDR speed %dMHz\n", speed); Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 18, 2020 Author Share Posted February 18, 2020 I probably got it. for (UINTN i=6; i >= 0; i--) (line 339) must be for (INTN i=6; i >= 0; i--) Same line 355. @Slice Could you try ? Link to comment Share on other sites More sharing options...
mhaeuser Posted February 18, 2020 Share Posted February 18, 2020 Heads up for your CONST literals, AMI disk stack may write to the file path args to EFI_FILE_PROTOCOL (replace trailing \ with '\0') Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 18, 2020 Author Share Posted February 18, 2020 I push that modification, because it was wrong anyway. Link to comment Share on other sites More sharing options...
Slice Posted February 18, 2020 Share Posted February 18, 2020 39 minutes ago, Jief_Machak said: I probably got it. for (UINTN i=6; i >= 0; i--) (line 339) must be for (INTN i=6; i >= 0; i--) Same line 355. @Slice Could you try ? OK, but only at evening. Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 18, 2020 Author Share Posted February 18, 2020 Just now, Slice said: OK, but only at evening. These loops were wrong. I don't understand why it's working on my computer. Maybe getVendorName is not always called ? Link to comment Share on other sites More sharing options...
Slice Posted February 18, 2020 Share Posted February 18, 2020 This is the error. Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 18, 2020 Author Share Posted February 18, 2020 1 hour ago, Download-Fritz said: Heads up for your CONST literals, AMI disk stack may write to the file path args to EFI_FILE_PROTOCOL (replace trailing \ with '\0') Which function of EFI_FILE_PROTOCOL may overwrite the path args ? If it's the case, it'll probably still work because, internally, litterals are still writable, I think. Even though I would remove the CONST and make a wrap function that duplicate the string. Link to comment Share on other sites More sharing options...
Slice Posted February 18, 2020 Share Posted February 18, 2020 6 hours ago, Jief_Machak said: For this final switch, I had to add a LOT of CONST, because one the difference with C, is that C++ enforce that litteral are read only (sure it can be "deactivated"). adding a CONST somewhere doesn't change the compiled code. It's just a check at compiled time. But in 2 places, a little code modification was needed. One of this place is smbios,c when updating vendor name for RAM. Could that be related ? Have a look to the difference I've committed in smbios.c. Maybe I understood incorrectly how UpdateSmbiosString works. I think you understood incorrectly. The purpose of UpdateSmbiosString is to update SmbiosTableN with a contents of Butter. So EFI_STATUS UpdateSmbiosString (OUT APPLE_SMBIOS_STRUCTURE_POINTER SmbiosTableN, IN OUT SMBIOS_TABLE_STRING* Field, IN CHAR8* Buffer) Link to comment Share on other sites More sharing options...
Slice Posted February 18, 2020 Share Posted February 18, 2020 @Jief_Machak Using such call XStringW::XStringW() { DBG("Construteur\n"); Init(); } Do you have to define Init() without parameters? EDITED: I see, sorry. Link to comment Share on other sites More sharing options...
apianti Posted February 18, 2020 Share Posted February 18, 2020 (edited) 5 hours ago, Jief_Machak said: I probably got it. for (UINTN i=6; i >= 0; i--) (line 339) must be for (INTN i=6; i >= 0; i--) Same line 355. @Slice Could you try ? That is not how you should have fixed that loop. UINTN i = 7; while (i > 0) { --i; ... } What is up with no one knowing the difference between --i/++i and i--/i++? You should almost never be using i--/i++ unless you need the value before decrement/increment in the current expression. Otherwise, this emits at least two more instructions than the prefix operator does. 3 hours ago, Jief_Machak said: Which function of EFI_FILE_PROTOCOL may overwrite the path args ? If it's the case, it'll probably still work because, internally, litterals are still writable, I think. Even though I would remove the CONST and make a wrap function that duplicate the string. He's saying, you can't make a protocol structure const because it may write to the structure when you use the defined methods. The structure may have hidden elements that you can't access properly. EDIT: I pointed out a problem with your logic. Constant literals are linked internally in c++, not externally. So, those will be duplicated across every object unit they are used in. I doubt this is what you want to achieve in that case, as you would have the object, and you could write to it but it would most likely be in code memory as read only. EDIT2: Also, you would not be sure you were getting the copy of that object you wanted since it would most likely always be the one in the local translation unit. (I think it must be, lol) Edited February 18, 2020 by apianti Link to comment Share on other sites More sharing options...
Slice Posted February 18, 2020 Share Posted February 18, 2020 I have booted with this new Clover and it looks like all is fine. 0:103 0:000 Clover : Image base = 0xC3C06000 0:105 0:001 CTOR C3CDBEE0 -1009926432 0:105 0:000 CTOR C3C0642C -1010801620 0:106 0:000 atexit(C3C1C3BC, C3CDA780, C3CBCF38, 24) 0:107 0:001 atexit : allocate 0:108 0:000 atexit : exit (atexit_func_entry_count=1) 0:109 0:001 atexit(C3C1C3BC, C3CDA7A0, C3CBCF38, 24) 0:111 0:001 atexit : exit (atexit_func_entry_count=2) 0:112 0:001 atexit(C3C1C3BC, C3CDA7E0, C3CBCF38, 24) 0:113 0:001 atexit : exit (atexit_func_entry_count=3) 0:114 0:001 atexit(C3C1C3BC, C3CDA7C0, C3CBCF38, 24) 0:116 0:001 atexit : exit (atexit_func_entry_count=4) 0:117 0:001 g_str = g_str:foobar 0:118 0:000 g_str2 = g_str:foobar2 0:118 0:000 global_str1 = global_str1 0:119 0:000 global_str2 = global_str2 0:120 0:000 destruct_globals_objects: idx=3 -1010711620, C3CDA7C0, C3CBCF38 0:122 0:001 destruct_globals_objects: idx=2 -1010711620, C3CDA7E0, C3CBCF38 0:124 0:001 destruct_globals_objects: idx=1 -1010711620, C3CDA7A0, C3CBCF38 0:125 0:001 destruct_globals_objects: idx=0 -1010711620, C3CDA780, C3CBCF38 2 Link to comment Share on other sites More sharing options...
Recommended Posts