C++ proposition

[Jief_Machak]

Ok. I don't really remember. Maybe I confuse with clang. So the ctors/init_array section is always 8 bytes long (one pointer). What does mean to say to call them forward or backward, if there is only one ?

Maybe the question is, can GCC decide one day to put more than one ? Which would mean that it's better to keep the loop ?

I'm confident enough not to have both section. If not sure, we can create .ctorss and a new .initar section and call them both, each one in the right order...

[Pene]

4 hours ago, Jief_Machak said:

Ok. I don't really remember. Maybe I confuse with clang. So the ctors/init_array section is always 8 bytes long (one pointer). What does mean to say to call them forward or backward, if there is only one ?

Maybe the question is, can GCC decide one day to put more than one ? Which would mean that it's better to keep the loop ?

I'm confident enough not to have both section. If not sure, we can create .ctorss and a new .initar section and call them both, each one in the right order...

Well, I mean for CLOVER, built with GCC, currently there is only one. I'm not saying it's the general case, not sure on what it depends even...

 

About Backward/Forward - for the cases that there are more than one, I saw some documentation saying that when iterating them, in order to respect the constructors initialization order, .ctors iteration order should be backward, while .init_array should be iterated forward (as you currently do). That's why I asked if you think it's possible for us to have more than one, to see if it's at all relevant. Or if you somehow get convinced that we will always have just a single function pointer in Clover, using [0] is probably the best. I didn't really look deeply into it though, so don't go blindly by what I say 

 

I'm not sure if both may exist, but if they do, I think that only one should be used. I don't think both should be called, they're probably the same pointers just in different order. But we should probably get better documented, as I'm not really sure about all this.

Edited April 26, 2020 by Pene

[Slice]

@Jief_Machak

LOAD_OPTIONS is not working again.

This screen

Verbose (-v) is not applied.

[Jief_Machak]

Oups, I'll have a look.

[Jief_Machak]

I think I found it.

Other question : If I have -v in my boot args in config,plist, is verbose checkbox supposed to be checked when I press space on a loader icon ?

[Slice]

8 hours ago, Jief_Machak said:

I think I found it.

Other question : If I have -v in my boot args in config,plist, is verbose checkbox supposed to be checked when I press space on a loader icon ?

Yes, it supposed to be checked. As well as other arguments.

[Jief_Machak]

-v is back.

[Slice]

4 hours ago, Jief_Machak said:

-v is back.

But now it is always verbose no matter if I uncheck the key.

Log contains

106:348  23:399   boot with args: 
106:354  0:005  ParseBootOption: invalid input params

 

End of log

109:295  0:005  Using load options 'boot.efi -v slide=0 '

while I unchecked these options.

[Jief_Machak]

I tried and couldn't reproduce. But I found an uninitialised var that could explain "ParseBootOption:  invalid input params".

Could you try now ?

[Slice]

Just now, Jief_Machak said:

I tried and couldn't reproduce. But I found an uninitialised var that could explain "ParseBootOption:  invalid input params".

Could you try now ?

Try what?

[Jief_Machak]

If you still get this "ParseBootOption:  invalid input params", and if uncheck -v still doesn't work.

[Slice]

2 hours ago, Jief_Machak said:

If you still get this "ParseBootOption:  invalid input params", and if uncheck -v still doesn't work.

Did you commit something?

[Jief_Machak]

Oups, my git push didn't work and didn't see it.

I've added "BootOption.VariableSize = VarSize;" in Settings.cpp:438 just before a call to ParseBootOption (&BootOption);

ParseBootOption check BootOption->VariableSize, so left uninitialised, it could end up fail with "ParseBootOption:  invalid input params".

Please check if I'm right.

 

I don't know if it's connected with the uncheck of -v not working.

[Slice]

38 minutes ago, Jief_Machak said:

Oups, my git push didn't work and didn't see it.

I've added "BootOption.VariableSize = VarSize;" in Settings.cpp:438 just before a call to ParseBootOption (&BootOption);

ParseBootOption check BootOption->VariableSize, so left uninitialised, it could end up fail with "ParseBootOption:  invalid input params".

Please check if I'm right.

 

I don't know if it's connected with the uncheck of -v not working.

OK, I know why -v is not working. It happens when I debug kernel patches. Sorry for disturbing.

 

@Pene. A question to you.

Clover three days ago

See Ubuntu.

Latest Clover

 

The log with additional debug messages shows

34:234  0:012    found entry ubuntu,linux
34:239  0:004          AddLoaderEntry for Volume Name=EFI
34:244  0:004   image not found
34:248  0:004  IconName=linux comma=-1 size=5
34:253  0:004  got day icon 38 name{os_cata}
34:258  0:004    Full=linux
34:263  0:004  got day icon 20 name{vol_internal_ext3}
34:268  0:005   Show badge as Drive.    Loader entry created for 'PcieRoot(0x0)\Pci(0x3,0x0)\VenHw(CF31FAC5-C24E-11D2-85F3-00A0C93EC93B,80)\HD(1,GPT,065C30D9-992D-4C4C-8965-A266108C7D0F,0x800,0x60000)\EFI\Ubuntu\grubx64.efi'

So the procedure 

 

const XImage& XTheme::LoadOSIcon(const XString& Full)
{
  // input value can be L"win", L"ubuntu,linux", L"moja,mac" set by GetOSIconName (OSVersion)
  XString First;
  XString Second;
  XString Third;
  const XImage *ReturnImage;
  UINTN Comma = Full.indexOf(',');
  UINTN Size = Full.length();
  DBG("IconName=%s comma=%lld size=%lld\n", Full.c_str(), Comma, Size);

Already got a string "linux" without name "ubuntu" and nothing to split.

[Pene]

3 hours ago, Slice said:

@Pene. A question to you.

Already got a string "linux" without name "ubuntu" and nothing to split.

Some order with the functions available (mostly for myself, because in first two commits I had some misconception):

- ThemeX.GetIcon("name"): returns icons only from the predefined ~50 icons. Most Linux distributions don't exist there, only very few are in IconNames[] (ubuntu and suse).

- ThemeX.LoadOSIcon("name1,name2,name3"): adds "os_" to each name, and checks for each using GetIcon(). If none found - sets a dummy image.

- loader.cpp@ScanLoader(), at (gSettings.LinuxScan) { ... }: scans all possible linuxes, loads special icons for them. Note that LoaderEntry does not contain iconname ("ubuntu,linux"), but only OSType (which will be OSTYPE_LINEFI).

- loader.cpp@CreateLoaderEntry(): deducts IconName from OSType, so for OSTYPE_LINEFI it will be just "linux". That's where you had a call to LoadOSIcon only with "linux".

 

Your ubuntu icon didn't get loaded, because it was only loading from path. But we should try first preloaded, and only if not found, load from directory (I didn't think of this case, because very few Linux exist in IconNames, so thanks for noticing!).

It is best to do this first with GetIcon("os_name") and if empty, load it from ThemeDir (LoadOSIcon is not so good here, because it sets a dummy icon, which we don't want yet, as we also try from path).

After this, if no icon exists, the call from CreateLoaderEntry() will load a general linux icon.

My last commit should be OK for all cases (but maybe I'll do some more cleanup at some point).

Edited April 29, 2020 by Pene

[Slice]

Good, Ubuntu presents.

[Pene]

@Jief_Machak

 

Something that just crossed my mind:

If we have to call initialization of the global constructors manually, are the destructors for them still called?

Or maybe we have to call the destructors also (there is .dtors / .finit_array)?

Edited April 29, 2020 by Pene

[Jief_Machak]

They are not called.

I didn't double check, but I'm pretty sure that the memory is reset when the kernel boot. Let's call that a mass de-allocation 

If someone can confirm...

 

It's technically possible to call the destruction of globals objects just before starting an EFI, and call again construct_global_objets if EFI didn't boot.

[Slice]

Simple test is Shell.efi which can return to Clover.

[Jief_Machak]

New evolution of XString !

 

XString is renamed XString8 for more clarity. (and _XS renamed _XS8).

 

Apart from that, usage of XString didn't change much.

 

 

+ operator is more flexible. We don't need to use the _XS or _XSW. For example, it's possible do write 

XString8 xs1 = "Test:"_XS8;
XStringW xsw1 = L"world"_XSW;
XString8 xs2 = xs1 + "hel" + 'l' + L'o' + L" the " + xsw1;

You can mix utf8 and wchar without _XS(W) operators (you can even mix utf32 !).

 

Hidden improvement :

The big issue was improving the fact that when we wrote :

XString8 xs1 = "hello"_XS8;

the static memory ("hello") was copied in a dynamic allocated memory held by xs1. (there was even a temporary object created by _XS).

For local var, it wasn't a huge deal. Technically slower, but not enough to be really noticed in Clover. For static vars, it makes quite some memory allocated to permanently represent all the literals.

So now, memory isn't copied anymore. In the example above, xs1 will just hold a pointer to "hello". Way faster than before because no memory allocation and no memory copy.

Also, it "propagates". Meaning that if you do

XString8 xs2 = xs1;

xs2 will also just be a pointer to "hello".

 

NOTE : it's possible to save even more memory and time by declaring constants as constexpr LString8 or LStringW. 

constexpr const LString8 ls1 = "foo"_XS8;

The above expression will be evaluated at compile time. Which means that a fully constructed object is stored is data section. That would work even if we don't call construct_globals_objects !.

Plus : LString(8,16,32,W) is 8 bytes smaller than the equivalent XString(8,16,32,W).

Of course, you can't modify it ! But static globals constants are supposed to be const anyway.

And you can use it in expression like an XString.

xs1 = ls1;
or
xs1 = ls1 + "bar";
etc.

A LString(8,16,32,W) var can be given as parameter to a function/method that expect a "const XString(8,16,32,W) &" parameter. But NOT a function/method that expect "XString(8,16,32,W) &" (non-const).

It's one reason more to avoid non-const reference.

 

 

 

 

 

 

[Jief_Machak]

5 hours ago, Jief_Machak said:

LString(8,16,32,W) is 8 bytes smaller

In fact, it's just a pointer. So LString(8,16,32,W) is 8 bytes, same size as char*.

In short :

constexpr const LString8 ls1 = "foo"_XS8;

take the exact same space as a char* in .data section. 

 

[Pene]

@Jief_Machak

 

While debugging a problem with -v not working, I figured that actually LoadOptions were disappearing completely after pressing "spacebar" in some cases. I found a problem where it seems just the pointer was copied and not the XStringArray (take a look at my commit 06f1f93, the change at shared_with_menu.cpp). If there's a better way to do it, feel free to fix.

Also, not sure if the same problem doesn't exist in other places, you know better where it's used.

Edited May 2, 2020 by Pene

[Jief_Machak]

@Pene I'll have a look.

Do you receive notification when a left a comment on a github commit on github website ? 

[Pene]

Just now, Jief_Machak said:

@Pene I'll have a look.

Do you receive notification when a left a comment on a github commit on github website ? 

Yes, if is is for my commit. Otherwise no.

I also answered you there when you last did that.

[Jief_Machak]

So I'm the one not having notification !