C++ proposition

[Pene]

2 minutes ago, Jief_Machak said:

So I'm the one not having notification ! 

 

So does this mean you did not get a notification when I tagged you yesterday in a reported issue? Some error in building on linux related to printf_lite: https://github.com/CloverHackyColor/CloverBootloader/issues/153

[Jief_Machak]

I saw it by chance. Just fixed it.

I don't know the problem with my github notification.

[Pene]

1 hour ago, Jief_Machak said:

I saw it by chance. Just fixed it.

I don't know the problem with my github notification.

Maybe I wasn't clear - I get email notifications. Not any other kind of notifications.

Edited May 3, 2020 by Pene

[Pene]

1 hour ago, Jief_Machak said:

@Pene

I saw you replaced "Entry->LoadOptions = loaderEntry->LoadOptions;" by "Entry->LoadOptions = Split<XStringArray>(loaderEntry->LoadOptions.ConcatAll(" "_XS8).wc_str(), " ");"

For me, they are strictly equivalent because you concat all to split it. I'm wondering if you were trying to do something else ?

No. The problem was that

Entry->LoadOptions = loaderEntry->LoadOptions

did not work, because loaderEntry probably stopped existing at some later stage. So it result was that Entry->LoadOptions was empty at the time when StartLoader() executed.

I think it wasn't copied, it was probably just assigned the pointer.

That's why until you find a better solution, I made that change, which actually performed a copy, and it solved the problem.

 

If you wish you can test, for example, before that change:

 

Comment out at main.c@RefitMain():

//gST->ConOut->OutputString = NullConOutOutputString;

And add at the beginning of main.c@StartLoader():

DebugLog(2, "LoadOptions at StartLoader: <%s>\n", LoadOptions.ConcatAll(" "_XS8).wc_str());
gBS->Stall(10000000);

Then from Clover GUI, press SPACEBAR on some Windows/Linux/macOS<10.12 entry (so that it will get into that case where that code it used). It will show you correct boot args after you press SPACEBAR (probably still exists then), but then if you do "boot with selected options" it gets to StartLoader() with empty LoadOptions:

LoadOptions at StartLoader: <>

After my change, which looks like an equivalent (that was the idea really, an equivalent that makes a copy), LoadOptions is not empty anymore.

Edited May 3, 2020 by Pene

[Jief_Machak]

14 minutes ago, Pene said:

I think it wasn't copied, it was probably just assigned the pointer

No no, it's copied. I think it was a coincidence with another problem (loadOptionsW that was destroyed too soon in "StartEFILoadedImage" ?) maybe.

There is definitely something else.

I'll have a look later.

 

[Pene]

32 minutes ago, Jief_Machak said:

loadOptionsW that was destroyed too soon in "StartEFILoadedImage" ?) maybe.

Surely not, as the debug is the first line in StartLoader(). So there is nothing to have destroyed.

I just tried to change them both back to Entry->LoadOptions = loaderEntry->LoadOptions - and again it is empty. Change back to the concat(split) and it works.

 

Note: you must press space, and then boot, to see the problem, on any OS that is NOT macOS>=10.12 (because then those assignments don't happen). You can easily see it in Qemu also.

 

EDIT: The fact it is NOT empty before StartLoader() (you see the correct string displayed on the screen) made me suspect it was just a pointer assignment.

Edited May 3, 2020 by Pene

[Jief_Machak]

Just now, Pene said:

Surely not, as the debug is the first line in StartEFILoadedImage. So there is nothing to have destroyed.

I just tried to change it back to Entry->LoadOptions = loaderEntry->LoadOptions - and again it is empty. Change back to the concat(split) and it works.

 

Note: you must press space, and then boot, to see the problem, on any OS that is NOT macOS>=10.12 (because then those assignments don't happen). You can easily see it in Qemu also.

 

 

Strange... Probably a bug in XStringArray... I'll look.

[Jief_Machak]

Ok, think I got it. The solution is : just to remove this line.

If you look at DecodeOptions loaderEntry is the same as Entry, just the type is different. So "Entry->LoadOptions = loaderEntry->LoadOptions" is assigning onto itself.

Because the first thing done in the assign operator is to empty the array to be ready to fill it with new values... It ends up empty.

I never thought about doing the test of assigning onto itself, and I agree that it should work. I'll think about it.

 

Question : are boot args case sensitive ? Should we create an add method that ignore duplicate with a case insensitive comparison ? 

[Slice]

8 hours ago, Jief_Machak said:

Ok, think I got it. The solution is : just to remove this line.

If you look at DecodeOptions loaderEntry is the same as Entry, just the type is different. So "Entry->LoadOptions = loaderEntry->LoadOptions" is assigning onto itself.

Because the first thing done in the assign operator is to empty the array to be ready to fill it with new values... It ends up empty.

I never thought about doing the test of assigning onto itself, and I agree that it should work. I'll think about it.

 

Question : are boot args case sensitive ? Should we create an add method that ignore duplicate with a case insensitive comparison ? 

Yes, boot-args are case-sensitive.

There is a key "-v" but no key "-V".

https://opensource.apple.com/source/xnu/xnu-6153.61.1/pexpert/gen/bootargs.c.auto.html

[Pene]

On 5/4/2020 at 12:01 AM, Jief_Machak said:

Ok, think I got it. The solution is : just to remove this line.

If you look at DecodeOptions loaderEntry is the same as Entry, just the type is different. So "Entry->LoadOptions = loaderEntry->LoadOptions" is assigning onto itself.

Because the first thing done in the assign operator is to empty the array to be ready to fill it with new values... It ends up empty.

I never thought about doing the test of assigning onto itself, and I agree that it should work. I'll think about it.

Yes, interesting bug. Self assignment was useless there, but as you also said, it really should be made to work. Who knows what other weird issues can result.

[Pene]

@Jief_Machak

Hi again,

 

guidstr() @ rEFIt_UEFI/Platform/Posix/stdio.cpp is broken.

To see: press space on any load item in clover GUI, you will see UUID is empty.

To confirm the problem is there, I tried to replace guidstr() with a simpler implementation, and problem was solved. But I didn't commit anything about it, as it is better that you fix the existing implementation.

Edited May 9, 2020 by Pene

[Slice]

6 hours ago, Pene said:

@Jief_Machak

Hi again,

 

guidstr() @ rEFIt_UEFI/Platform/Posix/stdio.cpp is broken.

To see: press space on any load item in clover GUI, you will see UUID is empty.

To confirm the problem is there, I tried to replace guidstr() with a simpler implementation, and problem was solved. But I didn't commit anything about it, as it is better that you fix the existing implementation.

You mean strguid().

Here

  Guid = FindGPTPartitionGuidInDevicePath(Volume->DevicePath);
  if (Guid) {
    SubScreen->AddMenuInfoLine_f("UUID: %s", strguid(Guid));
  }

 

[Slice]

Because in the procedure _PPrint()

formats %t and %g are commented out.

[Slice]

So posix implementation can't work.

The issue resolved as

    SubScreen->AddMenuInfoLine_f("UUID: %ls", GuidLEToStr(Guid).c_str());

May I commit this change?

[Slice]

It was in deep past when I created the function 

GuidLEToStr(GUID)

I don't remember now what was the problem with EDK2 formats %t and %g. There is something about the VA_ARG can't be pointers except strings.

Probably there is no problem with VS compilation because of ms_va_args.

[Slice]

But there are 28 occurence of strguid()....

[Jief_Machak]

I have a look.

[Slice]

Also not works

    XString8 EfiBootDevice;
    EfiBootDevice.SPrintf(
			"<array><dict>"
	    "<key>IOMatch</key>"
	    "<dict>"
	    "<key>IOProviderClass</key><string>IOMedia</string>"
	    "<key>IOPropertyMatch</key>"
	    "<dict><key>UUID</key><string>%s</string></dict>"
	    "</dict>"
	    "</dict></array>", strguid(Guid));
    DBG ("  * efi-boot-device: %s\n", EfiBootDevice.c_str());
    Status        = SetNvramVariable (L"efi-boot-device", &gEfiAppleBootGuid, Attributes, EfiBootDevice.sizeInBytes(), EfiBootDevice.c_str());

The log

37:890  0:000    * GUID = 
37:891  0:000    * efi-boot-device: <array><dict><key>IOMatch</key><dict><key>IOProviderClass</key><string>IOMedia</string><key>IOPropertyMatch</key><dict><key>UUID</key><string></string></dict></dict></dict></array>
37:891  0:000  Custom boot screen not used because entry has unset use graphics
37:893  0:001  Closing log
 

[Jief_Machak]

Yes, there is a small bug as I do n > size -2, but size can be 0. Resolving now.

[Jief_Machak]

So, posix implementation can work . Committed.

But I didn't know about "GuidLEToStr" at that time, I imagine.

Are they doing the same ? If yes, we should keep strguid, not because I did it, but because it avoids memory reallocation.

[Pene]

2 hours ago, Jief_Machak said:

So, posix implementation can work . Committed.

Thanks.

I see another bug somewhere: seems LoadOptions.removeIC does not work?

this code at entry_scan/loader.cpp:

..
SubEntry = Entry->getPartiallyDuplicatedEntry();
    if (SubEntry) {
      if (WithSplash) {
        if (Quiet) {
          SubEntry->Title.SWPrintf("%ls verbose without splash", Entry->Title.s());
          SubEntry->LoadOptions.removeIC("splash"_XS8);
          SubEntry->LoadOptions.removeIC("quiet"_XS8);
..

ends up with "splash" and "quiet" included. Not sure if the bug is at removeIC itself or something else, but they are still there when booting with that option.

Edited May 10, 2020 by Pene

[Jief_Machak]

I have to admit that the XStringArray tests are far from complete...

I have a look now.

[Jief_Machak]

removeIC seems to work. I add some tests.

BUT : when I copied the literal "splash" and "quiet", I ended up with an invisible utf8 char after the h of splash. Invisible char that I can't find in loader.cpp anymore. Very strange.

I create a common var for quiet and splash. Could you try again ? If it was an invisible char problem, it should be solved.

[Pene]

10 minutes ago, Jief_Machak said:

removeIC seems to work. I add some tests.

BUT : when I copied the literal "splash" and "quiet", I ended up with an invisible utf8 char after the h of splash. Invisible char that I can't find in loader.cpp anymore. Very strange.

I create a common var for quiet and splash. Could you try again ? If it was an invisible char problem, it should be solved.

Nope, it's the same.

By the way, I tried to add a LoadOptions.AddID("something") in that code and it also wasn't added to the options that are booting.

I guess the problem is somewhere else.

 

By the way, the options there are from 

const XStringArray LINUX_DEFAULT_OPTIONS = Split<XStringArray>("ro add_efi_memmap quiet splash vt.handoff=7", " ");

Probably not even related to Linux booting.

I am still using that debug of LoadOptions from some time ago, that's how I see what's going on when booting.

Edited May 10, 2020 by Pene

[Jief_Machak]

Maybe there is another self assignment somewhere else ?

I've just fixed it. Problem still there ?