C++ proposition

[Matgen84]

Hi @Jief_Machak

 

I've Debug mode at Clover start, I think is temporary as you says before. The transition time in c +++

 

 

[Jief_Machak]

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.

[Matgen84]

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

[Jief_Machak]

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");

 

[Slice]

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.

[Slice]

May be we need a rule for cpp files in MSVC toolset?

[Jief_Machak]

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.

[Jief_Machak]

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

[chris1111]

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

[vector sigma]

buildme should autodetect an old setup and clean the enviroment automatically for c++ Clover. All fine here. 

[Jief_Machak]

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.

[apianti]

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

[Jief_Machak]

7 hours ago, chris1111 said:

End the Build package failed

clover-genconfig wasn't building because of 'extern "C"'.

Fixed. 

[Slice]

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);

 

[Jief_Machak]

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 ?

[mhaeuser]

Heads up for your CONST literals, AMI disk stack may write to the file path args to EFI_FILE_PROTOCOL (replace trailing \ with '\0')

[Jief_Machak]

I push that modification, because it was wrong anyway. 

[Slice]

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.

[Jief_Machak]

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 ?

[Slice]

This is the error.

[Jief_Machak]

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.

[Slice]

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)

[Slice]

@Jief_Machak

Using such call

XStringW::XStringW()
{
DBG("Construteur\n");
	Init();
}

Do you have to define Init() without parameters?

 

EDITED: I see, sorry.

[apianti]

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

[Slice]

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