pbatard / EfiFs

EFI FileSystem drivers
https://efi.akeo.ie
GNU General Public License v3.0
506 stars 77 forks source link

Log print messy with gcc + EDK2 #16

Closed ventoy closed 4 years ago

ventoy commented 4 years ago

In my environment, the log output is messy after I set the FS_LOGGING efi variable. After doing some research I found some clues. It seems that Print_t is defined as follows: typedef UINTN (*Print_t) (IN CONST CHAR16 *fmt, ... );

In function SetLogging the log function will be assigned with Print if log level is suitable. But in EDK2 Print is defined with EFIAPI so that may be the problem.

pbatard commented 4 years ago

Can you explain what "messy" means? Do you get like garbage output on screen?

Also can you provide the details of the environment where you are seeing this "messy" output. For one thing the full report from your UEFI Shell's ver command would be useful. And also and indication of the hardware you're testing with, as well as the file system driver used, and any other information that might help.

Basically, I want to be in a position to replicate your issue, because it's hard to know if a fix is appropriate without being able to test it. So anything you can provide to help replicate the problem is welcome!

ventoy commented 4 years ago

This is log with messy: err

This is normal message: ok

My envirnment is a virtual machine on VMware ESXi ver

I use only iso9660.efi driver.

I build it with gcc 4.8 + EDK2, just follow the last section "Commands to compile EfiFs using EDK2 on a vanilla Debian GNU/Linux 10.1" in README

By the way, finally I add EFIAPI in the define of Print_t and PrintNone, and the output is OK now.

pbatard commented 4 years ago

Okay, I guess that makes some sense as gcc won't use __cdecl by default, so anything that's not formally declared EFIAPI when interacting with UEFI libraries could indeed spell trouble.

So you're saying that the following:

typedef UINTN (EFIAPI *Print_t)        (IN CONST CHAR16 *fmt, ... );

and

static UINTN EFIAPI PrintNone(IN CONST CHAR16 *fmt, ... ) { return 0; }

Should do the job?

ventoy commented 4 years ago

Yes, the messy disappear after that. And I think it shoud work with gcc+EDK2 ! But may not work with gnu-efi.

pbatard commented 4 years ago

Nah, gnu-efi is fine because the compiler links to the Print() calls statically, therefore the calling convention being used is always the correct one.

That's actually the reason why this issue exists in the first place: Because gnu-efi is either meant to be linked statically, or dynamically but with similar toolchain defaults, there is no real need to declare an explicit calling convention, so the author of gnu-efi left it out.

And of course, when I copy/pasted the Print() prototypes I needed, I used the gnu-efi ones, so I didn't apply a calling convention either.

But the EDK2 is different, because their Print() functions are meant to be provided as a native system library, rather than linked directly into a compiled application, so they do need an explicit calling convention in order for compilers to know how they should pass parameters.

I probably should have picked that if I had spent more time testing on Linux, but I have to say that, when you are starting to multiply environments (Windows/MSVC/gnu-efi, Windows/gcc/gnu-efi, Windows/MSVC/EDK2, Linux/gcc/gnu-efi, Linux/gcc/EDK2 — that last one is the only case where the calling convention issue will manifest itself) with executables that are designed not to produce any output by default, you're bound to miss some stuff...

Thanks to user reports like yours however, we can get these issues fixed.

I'm going to close this issue now, as I will push a commit that applies your proposed changes.

Thanks again for reporting and investigating this!