Jump to content

Clover Problems and Solutions

ErmaC

By ErmaC,
in Clover Development

 Share
3205 posts in this topic

Recommended Posts

apianti
apianti
Posted
Posted

I know the code is better as it's faster and produces less instructions but I don't have xcode 10, so can't test that it doesn't produce that weird optimized infinite loop. You resolved it by disabling optimization, you should be able to enable optimization again with that code.

  • Like 1
Slice
Slice
Posted
Posted
3 hours ago, apianti said:

Uh, what?? 


*IdtArray++

Definitely modified inside the loop. What on earth is Xcode 10 doing? Seems like its optimization is completely broken.

May be clang account this as (*IdtArray)++ while you want *(IdtArray++)?

 

2 hours ago, apianti said:

So the changes worked or did not? I'm confused now.

No, I can't apply changes because they are not compilable.

But yes, I can reenable -Os with my codes.

apianti
apianti
Posted
Posted (edited)
18 hours ago, Slice said:

May be clang account this as (*IdtArray)++ while you want *(IdtArray++)?

 

DF is correct the postfix increment/decrement has the highest precedence of any operator in C, so it will always be the first to be evaluated from left to right. There is clearly some bugs in Xcode 10's clang.

 

18 hours ago, Slice said:

No, I can't apply changes because they are not compilable.

But yes, I can reenable -Os with my codes.

 

With the original code? That makes no sense when it was optimizing into an infinite loop of relative calls before...

 

EDIT: You can always try this: 

UINT32 *IdtArrayEnd;
UINT32 RegionBase;
IdtArray = (UINT32 *)(UINTN)(((UINT32)ProtectedModeBaseVector) * sizeof(UINT32));
IdtArrayEnd = IdtArray + 8;
for (RegionBase = (UINT32)(UINTN)LegacyRegionBase; IdtArray < IdtArrayEnd; RegionBase += sizeof(UINT32)) {
  *IdtArray = (((RegionBase & 0xF0000) << 12) | (RegionBase & 0xFFFF));
  ++IdtArray;
}

or 

UINT32 *IdtArrayEnd;
UINT32 RegionBase;
IdtArray = (UINT32 *)(UINTN)(((UINT32)ProtectedModeBaseVector) * sizeof(UINT32));
IdtArrayEnd = IdtArray + 8;
for (RegionBase = (UINT32)(UINTN)LegacyRegionBase; IdtArray < IdtArrayEnd; ++IdtArray, RegionBase += sizeof(UINT32)) {
  *IdtArray = (((RegionBase & 0xF0000) << 12) | (RegionBase & 0xFFFF));
}

Which are equivalently the same as the previous code but with having to write more code and include an additional instruction, however optimization should remove that anyway.

Edited by apianti
Slice
Slice
Posted
Posted
7 hours ago, apianti said:

With the original code? That makes no sense when it was optimizing into an infinite loop of relative calls before...

 

The key fix for the codes was 

-  UINT32                *IdtArray;
+  volatile UINT32       *IdtArray;

It prevents compiler to optimize loops such this.

apianti
apianti
Posted
Posted (edited)

No idea, someone who decide to convert the svn repo on sourceforge to github and not keep it up to date, as it has the last commit from you in 2015, a direct commit made to the svn repo r3330.

 

EDIT: It was JrCs. Apparently he invited us like four years ago, lol. I can't do anything to it though besides probably update it to be current.

Edited by apianti
Slice
Slice
Posted
Posted

I was trying to compile Clover by VS2013 with some modifications in cbuild.bat and in sources but finally I got 

Generating code
Finished generating code
UefiApplicationEntryPoint.lib(ApplicationEntryPoint.obj) : error LNK2001: unresolved external symbol memset
d:\projects\edk2\Build\Clover\RELEASE_VS2013x86\X64\Clover\rEFIt_UEFI\refit\DEBUG\CLOVER.dll : fatal error LNK1120: 1 unresolved externals
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 12.0\Vc\bin\x86_amd64\link.exe"' : return code '0x460'
Stop.


build...
 : error 7000: Failed to execute command
        C:\Program Files (x86)\Microsoft Visual Studio 12.0\Vc\bin\nmake.exe /nologo tbuild [d:\projects\edk2\Build\Clover\RELEASE_VS2013x86\X64\Clover\rEFIt_UEFI\refit]

How can I move forward?

apianti
apianti
Posted
Posted

How on earth did you even get it to build that far? There were so many type casting errors... There is still some problems I can't seem to fix with the build script but it builds now it just doesn't do the post build... r4814.

Slice
Slice
Posted
Posted

Yes, numerous type casting warnings->errors. Just correct all of them.

But memset... I don't know where to search.

 

EDITED: C U 

-    memset(r->scanline, 0, r->width);
+    gBS->SetMem(r->scanline, r->width, 0);
Slice
Slice
Posted
Posted

Thanks! Full success with Clover 64 and fail with Clover32

I made some more correction, rev4816.

Not sure if this boot6 will work.

Attached VS2013 compilation!!!

 

 

CLOVERX64.zip

boot6.zip

Clover32 compilation failed with

UefiApplicationEntryPoint.lib(ApplicationEntryPoint.obj) : error LNK2001: unresolved external symbol __ftoui3

 

Slice
Slice
Posted
Posted

One strange issue with D:\Projects\edk2\MdeModulePkg\Universal\PCD\Dxe\Service.h 

#if (PCD_SERVICE_DXE_VERSION != PCD_DXE_SERVICE_DRIVER_VERSION)
  #error "Please make sure the version of PCD DXE Service and the generated PCD DXE Database match."
#endif

I have to change this line

#define PCD_SERVICE_DXE_VERSION      7

from 7 to 6

while with clang it is good with 7.

apianti
apianti
Posted
Posted (edited)

You have to remove the entire Build folder if you only change a header when using visual studio, edk2 build on windows only recompiles source objects that have changed but doesn't check includes. It's stupid. In fact you can rebuild a bunch of times, if nothing has changed in main.c (I'm not sure if others), it doesn't update the version information from version.h either. As for the 32bit build, the problem is float support since there is no guarantee of needed instructions on all 32bit processors since they don't all support sse2 like 64bit, so visual studio uses intrinsics but they are part of the c runtime that can't be included because it's tied to windows, we can however grab the sources from visual studio folder and move them into the project for 32bit support.

 

EDIT: Oh wait I'm not sure there is source for those float conversion function after looking at the crt source for 2012, 2013, 2015, or 2017.... Yes, I have all those installed, lol.

Edited by apianti
apianti
apianti
Posted
Posted (edited)

Some of your changes do not make sense. Like setting SVNREVISION in cbuild is the revision that the local copy is already at, so you are just changing what it displays. This should be retrieved through the lines you commented out. If you want to check out a different version, I usually just do so by hand with the svn command: 

svn up -rREVISION

or if not checked out yet: 

svn checkout -rREVISION URL WORKING_DIR

Changing UINT64 * and UINT32 * to unsigned long long * and unsigned long *, while the first is exactly equivalent the second is not, also unsure why you would need that change. Your change to the >0 makes any negative number or zero false, when my code made any non zero number true. I disabled the warning for the unnamed union, unsure why it wasn't working for you. Why did you add back in that extern to MapBiosKey when the function is in the same source?

 

EDIT: Sorry it should actually be a combination of mine change and a variation of yours, (BOOLEAN)(getIntegerDict(...) != 0).

Edited by apianti
Slice
Slice
Posted
Posted

OK, my changes was before your commit so why there are some conflicts. For example BOOLEAN... Usually (a<b) is already boolean. But we have old C (not C++, not C#) which has no boolean type and work with integer. I think (a>0) if better then (BOOLEAN)a.

 

About cbuild.bat be free to remake it. I need SVNREVISION to be known during compilation because Version.h is created and included into sources. In macOS I can use (svn info ...) but in windows I use TortoiseSVN and see no possibility to get actual revision.

cbuild.bat -r 4817

looks to be a good way to set SVNREVISION.

 

About unsigned long...

I will not make nonsense changes if not required. It was VS2013 forced me to do anything claiming an error in UINT64 usage. I quick fixed it just replacing UINT64 -> unsigned long long, and UINT32 -> unsigned long.

The second replace is good for VS2013 but again wrong for Clang. So now I replaced to unsigned int which is in agree with clang. Windows compatibility will check at Monday.

May be you may propose better way?

 

MapBiosKey. I don't remember initially why it is external. The warning said that external declaration inside the function and should be moved out of it. I did this. But then I got your commit where you just exclude it. I don't know. It is more safe to declare this function to be external. May be because of ABI.

 

mhaeuser
mhaeuser
Posted
Posted
5 hours ago, Slice said:

OK, my changes was before your commit so why there are some conflicts. For example BOOLEAN... Usually (a<b) is already boolean. But we have old C (not C++, not C#) which has no boolean type and work with integer. I think (a>0) if better then (BOOLEAN)a.

Comparisons always yield the type "int", just new compilers are smart enough to know the cast will not truncate. The only compilers I know having issues are <= VS2005, which are being dropped soon (or have they already been?) from edk2.

 

5 hours ago, Slice said:

About unsigned long...

I will not make nonsense changes if not required. It was VS2013 forced me to do anything claiming an error in UINT64 usage. I quick fixed it just replacing UINT64 -> unsigned long long, and UINT32 -> unsigned long.

Yes, "long" is not stable across ABIs. What was the actual error? Should never resort to using non-UEFI types imo

apianti
apianti
Posted
Posted
9 hours ago, Slice said:

OK, my changes was before your commit so why there are some conflicts. For example BOOLEAN... Usually (a<b) is already boolean. But we have old C (not C++, not C#) which has no boolean type and work with integer. I think (a>0) if better then (BOOLEAN)a.

 

3 hours ago, Download-Fritz said:

Comparisons always yield the type "int", just new compilers are smart enough to know the cast will not truncate. The only compilers I know having issues are <= VS2005, which are being dropped soon (or have they already been?) from edk2.

 

Yes, "long" is not stable across ABIs. What was the actual error? Should never resort to using non-UEFI types imo

 

Nah, it's actually like DF says, but in my edit I suggest that we use method to satisfy all, which is (BOOLEAN)(a != 0).

 

9 hours ago, Slice said:

About cbuild.bat be free to remake it. I need SVNREVISION to be known during compilation because Version.h is created and included into sources. In macOS I can use (svn info ...) but in windows I use TortoiseSVN and see no possibility to get actual revision.

cbuild.bat -r 4817

looks to be a good way to set SVNREVISION.

 

It already did that. That's what the lines that you commented out and replaced did, ran svnversion -n to get the current revision. I also use TortoiseSVN, did you not install the subversion binaries when you ran the installer? You must make a commit before the revision changes. 

rem # get the current revision number
:getrevision
  cd "%CURRENTDIR%"
  rem  get svn revision number
  svnversion -n>%F_VER_TXT%
  set /P s=<%F_VER_TXT%
  rem del %F_VER_TXT%
  set SVNREVISION=

rem # get the current revision number
:fixrevision
  if ["%s%"] == [""] goto init
  set c=%s:~0,1%
  set s=%s:~1%
  if ["%c::=%"] == [""] goto init
  if ["%c:M=%"] == [""] goto init
  if ["%c:S=%"] == [""] goto init
  if ["%c:P=%"] == [""] goto init
  set SVNREVISION=%SVNREVISION%%c%
  goto fixrevision

 

9 hours ago, Slice said:

About unsigned long...

I will not make nonsense changes if not required. It was VS2013 forced me to do anything claiming an error in UINT64 usage. I quick fixed it just replacing UINT64 -> unsigned long long, and UINT32 -> unsigned long.

The second replace is good for VS2013 but again wrong for Clang. So now I replaced to unsigned int which is in agree with clang. Windows compatibility will check at Monday.

May be you may propose better way?

 

3 hours ago, Download-Fritz said:

Yes, "long" is not stable across ABIs. What was the actual error? Should never resort to using non-UEFI types imo

 

This was a typecasting error that I fixed by using the correct type casting. As DF suggests we should not be using non-UEFI types. They should be changed back, and it will be ok again. I think you did a merge with your current working copy and it caused problems with my changes.

 

9 hours ago, Slice said:

MapBiosKey. I don't remember initially why it is external. The warning said that external declaration inside the function and should be moved out of it. I did this. But then I got your commit where you just exclude it. I don't know. It is more safe to declare this function to be external. May be because of ABI.

 

Nah, you can remove the line. The problem was the function was written below where it was called and was also declared file scope by using the declaration inside of a function which is an error in visual studio without disabling it. It was just easier to remove the declaration and move the function above where it was called.

 Share
Go to topic listing
×
×
  • Create New...