Jief_Machak Posted April 26, 2020 Author Share Posted April 26, 2020 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... Link to comment Share on other sites More sharing options...
Pene Posted April 26, 2020 Share Posted April 26, 2020 (edited) 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 Link to comment Share on other sites More sharing options...
Slice Posted April 27, 2020 Share Posted April 27, 2020 @Jief_Machak LOAD_OPTIONS is not working again. This screen Verbose (-v) is not applied. Link to comment Share on other sites More sharing options...
Jief_Machak Posted April 27, 2020 Author Share Posted April 27, 2020 Oups, I'll have a look. Link to comment Share on other sites More sharing options...
Jief_Machak Posted April 27, 2020 Author Share Posted April 27, 2020 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 ? Link to comment Share on other sites More sharing options...
Slice Posted April 28, 2020 Share Posted April 28, 2020 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. Link to comment Share on other sites More sharing options...
Jief_Machak Posted April 28, 2020 Author Share Posted April 28, 2020 -v is back. 1 Link to comment Share on other sites More sharing options...
Slice Posted April 28, 2020 Share Posted April 28, 2020 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. Link to comment Share on other sites More sharing options...
Jief_Machak Posted April 28, 2020 Author Share Posted April 28, 2020 I tried and couldn't reproduce. But I found an uninitialised var that could explain "ParseBootOption: invalid input params". Could you try now ? Link to comment Share on other sites More sharing options...
Slice Posted April 28, 2020 Share Posted April 28, 2020 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? Link to comment Share on other sites More sharing options...
Jief_Machak Posted April 28, 2020 Author Share Posted April 28, 2020 If you still get this "ParseBootOption: invalid input params", and if uncheck -v still doesn't work. Link to comment Share on other sites More sharing options...
Slice Posted April 28, 2020 Share Posted April 28, 2020 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? Link to comment Share on other sites More sharing options...
Jief_Machak Posted April 28, 2020 Author Share Posted April 28, 2020 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. Link to comment Share on other sites More sharing options...
Slice Posted April 28, 2020 Share Posted April 28, 2020 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. Link to comment Share on other sites More sharing options...
Pene Posted April 28, 2020 Share Posted April 28, 2020 (edited) 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 Link to comment Share on other sites More sharing options...
Slice Posted April 29, 2020 Share Posted April 29, 2020 Good, Ubuntu presents. Link to comment Share on other sites More sharing options...
Pene Posted April 29, 2020 Share Posted April 29, 2020 (edited) @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 Link to comment Share on other sites More sharing options...
Jief_Machak Posted April 29, 2020 Author Share Posted April 29, 2020 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. 1 Link to comment Share on other sites More sharing options...
Slice Posted April 29, 2020 Share Posted April 29, 2020 Simple test is Shell.efi which can return to Clover. 1 Link to comment Share on other sites More sharing options...
Jief_Machak Posted April 30, 2020 Author Share Posted April 30, 2020 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. Link to comment Share on other sites More sharing options...
Jief_Machak Posted April 30, 2020 Author Share Posted April 30, 2020 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. Link to comment Share on other sites More sharing options...
Pene Posted May 2, 2020 Share Posted May 2, 2020 (edited) @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 Link to comment Share on other sites More sharing options...
Jief_Machak Posted May 3, 2020 Author Share Posted May 3, 2020 @Pene I'll have a look. Do you receive notification when a left a comment on a github commit on github website ? Link to comment Share on other sites More sharing options...
Pene Posted May 3, 2020 Share Posted May 3, 2020 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. Link to comment Share on other sites More sharing options...
Jief_Machak Posted May 3, 2020 Author Share Posted May 3, 2020 So I'm the one not having notification ! Link to comment Share on other sites More sharing options...
Recommended Posts