Carsey, Jaben [mailto:jaben.car...@intel.com] wrote:
]Sent: Friday, December 19, 2014 11:03 AM
]To: edk2-devel@lists.sourceforge.net; 'Olivier Martin'; Justen, Jordan L; Jin,
Eric
]Subject: Re: [edk2] patch review feedback about FvSimpleFileSystem module
]
]Scott,
]
]I know that STATIC has also been heavily discussed about shell code.
]
]I believe that the use of STATIC makes the code both more readable and
]safer as the compiler protects from certain behaviors.
]
]I don't know much about comdat folding, but do you have a patch to fix that
]root cause of this issue?
Hello Jaben,
OK, I will send a patch in a separate email. Comdat folding has one purpose:
code size reduction. So it make sense to enable comdat folding for EDK2 code,
though it creates quite a surprise the first time you encounter it while
source level debugging. Here is a command line example that demonstrates
comdat folding.
Source file main.c
int my_foo (int data) {return data + data + data;}
int my_bar (int value){return value * 3;}
main (int argc, char *argv [])
{
int a = my_foo (argc);
int b = my_bar (argc);
return a + b;
}
Compile command, tested with VS2005/VS2010/VS2013:
cl /nologo /Zi /O1 /Ob1 /Gy main.c /link /opt:ref /opt:noicf && dumpbin
-disasm main.exe | findstr "call.*my_"
Output:
main.c
000000014000100C: E8 EF FF FF FF call my_foo
0000000140001013: E8 EC FF FF FF call my_bar
Nothing unusual so far.
Now add comdat folding:
cl /nologo /Zi /O1 /Ob1 /Gy main.c /link /opt:ref /opt:icf && dumpbin
-disasm main.exe | findstr "call.*my_"
Output:
main.c
0000000140001008: E8 F3 FF FF FF call my_foo
000000014000100F: E8 EC FF FF FF call my_foo
The disassembly shows that function my_bar has been eliminated. Function
my_foo is used in its place. So when source level debugging, stepping into
my_bar() brings you to the source code for function my_foo instead. In this
example, the functions are not declared as static. To demonstrate using
static functions, the functions need to be made bigger so that the compiler
won't inline them.
Here Microsoft answers the question: Why does the debugger show me the wrong
function?
http://blogs.msdn.com/b/oldnewthing/archive/2005/03/22/400373.aspx
Thanks,
Scott
]-Jaben
> -----Original Message-----
> From: Scott Duplichan [mailto:sc...@notabs.org]
> Sent: Friday, December 19, 2014 5:47 AM
> To: edk2-devel@lists.sourceforge.net; 'Olivier Martin'; Justen, Jordan L; Jin,
> Eric
> Subject: Re: [edk2] patch review feedback about FvSimpleFileSystem
> module
>
> Tian, Feng [mailto:feng.t...@intel.com] wrote:
>
> ]Hi, Martin
> ]
> ]Per EDKII Coding Style 2.0 Spec, STATIC is not recommended for EDKII use.
> ](if you look into MdeModulePkg and MdePkg, you will find nobody uses this
> ]keyword as well).
>
> I appreciate Martin's use of STATIC for local functions. The recently updated
> EDK2 coding document mentions not using static functions as a work-around
> for a broken debugger. The broken debugger has never been identified. The
> reason is clear: The problematic debugger behavior described is what
> happens when comdat folding is enabled for the Microsoft tool chain. The
> Microsoft tool chain was the only supported tool chain when this problem
> was seen. EDK2 enables Microsoft comdat folding for all builds: RELEASE,
> DEBUG, and even NOOPT. Clearly someone didn't realize comdat folding was
> enabled and incorrectly assumed the resulting behavior was due to a
> debugger bug. The correct solution to the problem is to either disable
> Microsoft comdat folding for the NOOPT build or to recognize and live with
> comdat folding. Banning an important part of the C language to work around
> a problem that cannot be reproduced is not a sound engineering practice.
>
> Thanks,
> Scott
>
> ]For concern about why I create FV_FILESYSTEM_FILE for every Open()
> operation, ]yes, it's mainly because of the case you mentioned. It's
> mandatory although ]UEFI spec doesn't say anything about this. Otherwise
> we have no way to get ]correct Position info for every Open() if we only
> return the same ]EFI_FILE_PROTOCOL instance. Other comments are good to
> me. I will fix those ]typos and comment errors at check-in.
> ]
> ]Last, for review concern, Jodran's suggestion to send changes based on
> original ]patch is a good idea. I will follow up it later. For check-in
> process
> concern, ]it's mainly because we have strict check on new contributed
> modules and interface ]changes proposed adding to EDKII core packages. We
> couldn't check in a new module ]which doesn't follow UEFI spec to core
> package until we fix it. The 1st revision ]of the patch for UEFI SCT pass and
> the 2nd revision of the patch for partial read ]are the cases.
> ]
> ]Thanks
> ]Feng
> ]
> -----Original Message-----
> From: Olivier Martin [mailto:olivier.mar...@arm.com]
> Sent: Thursday, December 18, 2014 20:37
> To: Tian, Feng; Justen, Jordan L; Jin, Eric
> Cc: edk2-devel@lists.sourceforge.net; Colin Drake
> Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem module
>
> Missing 'STATIC' to FvFsFindExecutableSection(), FvFsGetFileInfo(),
> RemoveLastItemFromPath(), TrimFilePathToAbsolutePath(),
> InitializeUnicodeCollationSupportWorker() declarations.
>
> In FvSimpleFileSystemOpen(), you allocate a new FV_FILESYSTEM_FILE
> structure. It means if a file is opened twice in the same time there would be
> two instances of FV_FILESYSTEM_FILE. I do not disagree with the approach -
> for instance it allows to open the file in read only in by one driver and in
> read/write by an another driver. The UEFI specification does not say anything
> about concurrent accesses to a same EFI_FILE_PROTOCOL instance.
> The only comment I have is in FvSimpleFileSystemInternal.h,
> FV_FILESYSTEM_FILE is commented such as "Struct representing a file. There
> will be one of these for each opening file on each FV".
> We could definitely have more than "one of these".
>
> There is typo issue line 909 of FvSimpleFilesystem.c "Return Volume Lable" ->
> "Return Volume Label"
> Renamed variable 'FsVolumeLab' into 'FsVolumeLabel'
>
> Line 919 (same comment line 899), you wrote:
> ASSERT_EFI_ERROR (Status);
> return EFI_SUCCESS;
> You should write:
> ASSERT_EFI_ERROR (Status);
> return Status;
>
> As Jordan said, it would have be easier to push my first patch after reviewing
> it and then to add your new features as new patches.
>
> > -----Original Message-----
> > From: Tian, Feng [mailto:feng.t...@intel.com]
> > Sent: 18 December 2014 08:47
> > To: Justen, Jordan L; Olivier Martin; Jin, Eric
> > Cc: edk2-devel@lists.sourceforge.net; Colin Drake; Tian, Feng
> > Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem
> > module
> >
> > Jordan
> >
> > As I know, there have had several different implementations for
> > FvSimpleFileSystem besides you mentioned. But till now, only ARM
> > officially proposed to add this feature to MdeModulePkg. So I have to
> > use their version rather than others.
> >
> > I agree zip file is not a good way for review, here is the .patch file.
> >
> > As for commit log, it has been the beginning of the mail thread. No
> > change on it.
> >
> > Thanks
> > Feng
> >
> > -----Original Message-----
> > From: Justen, Jordan L
> > Sent: Thursday, December 18, 2014 3:50 AM
> > To: Tian, Feng; Olivier Martin; Jin, Eric
> > Cc: edk2-devel@lists.sourceforge.net; Colin Drake
> > Subject: Re: [edk2] patch review feedback about FvSimpleFileSystem
> > module
> >
> > On 2014-12-16 23:29:05, Tian, Feng wrote:
> > > Martin,
> > >
> > > Please review the latest patch again,
> >
> > Once again, you did not supply a Contributed-under or Signed-off-by.
> >
> > > in which partial read is supported.
> >
> > I think Colin's version already supported this:
> > https://github.com/cfdrake/FileSystemPkg
> >
> > struct _FILE_PRIVATE_DATA.Position:
> >
> https://github.com/cfdrake/FileSystemPkg/blob/master/FfsDxe/Ffs.h#L118
> >
> > FfsRead:
> >
> https://github.com/cfdrake/FileSystemPkg/blob/master/FfsDxe/Ffs.c#L930
> >
> > > Password: efi;0000
> >
> > I don't think a password zip file is a good way to provide patches.
> >
> > I think the best response would have been to reply to Olivier's
> > original patch, and point out changes that should be made. This way
> > ARM could make the changes, and we wouldn't have a munged up
> copyright
> > situation.
> >
> > A second best approach would be to apply Olivier's patch, and then
> > make a new patch of your own based on his patch. (Then we could easily
> > see what you changed from his.)
> >
> > At the least, it seems like you need to add Contributed-under and
> > Signed-off-by to be appended to Olivier's patch commit message.
> >
> > -Jordan
> >
> > > -----Original Message-----
> > > From: Tian, Feng
> > > Sent: Monday, December 08, 2014 09:28
> > > To: Olivier Martin; Justen, Jordan L; Jin, Eric
> > > Cc: edk2-devel@lists.sourceforge.net; Tian, Feng
> > > Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem
> > > module
> > >
> > > Hi, Martin
> > >
> > > Recently I found this patch has other serious issues. So the
> > > check-in
> > has to be delayed.
> > >
> > > According to UEFI spec, the SimpleFileSystem protocol interface
> > produced must support partial read. But the patch doesn't support it.
> > >
> > > And I ever said I had made the patch pass UEFI SCT, but after
> > > digging
> > into it, I found it's because the UEFI SCT SimpleFileSystem test case
> > has bug as well.
> > >
> > > Now I am working with SCT owner to solve these bugs together.
> > >
> > > Thanks
> > > Feng
> > >
> > > -----Original Message-----
> > > From: Olivier Martin [mailto:olivier.mar...@arm.com]
> > > Sent: Saturday, December 06, 2014 03:19
> > > To: Justen, Jordan L; Tian, Feng
> > > Cc: edk2-devel@lists.sourceforge.net
> > > Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem
> > > module
> > >
> > > Feng, what is the next step for moving forward with this patch.
> > > As I said in a previous email, I am happy with the changes you added
> > to the original patch.
> > > Thanks,
> > > Olivier
> > >
> > > > -----Original Message-----
> > > > From: Jordan Justen [mailto:jordan.l.jus...@intel.com]
> > > > Sent: 13 November 2014 02:50
> > > > To: Tian, Feng; Olivier Martin
> > > > Cc: edk2-devel@lists.sourceforge.net
> > > > Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem
> > > > module
> > > >
> > > > On 2014-11-12 18:10:11, Tian, Feng wrote:
> > > > > Jordan,
> > > > >
> > > > > As Contributed-under and Signed-off-by message are mandatory
> > > > > infos, I didn't mention it.
> > > >
> > > > Yes, it is mandatory that every contribution have this. Therefore,
> > > > it should be mentioned with every contribution of code. I usually
> > > > don't even look at a contribution that doesn't have these.
> > > >
> > > > > Ok, let's mend the commit message:
> > > > >
> > > > > MdeModulePkg/FvSimpleFileSystem: Add a new module to provide
> > > > > access
> > > > to executable files in FVs.
> > > > >
> > > > > This module implements Simple FileSystem protocol over Firmware
> > > > Volume (FV).
> > > > > EFI Modules included into a FV can be listed and launched from
> > the
> > > > > EFI Shell or any other EFI applications.
> > > > >
> > > > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > > > Signed-off-by: Brendan Jackman <brendan.jack...@arm.com>
> > > > > Signed-off-by: Feng Tian <feng.t...@intel.com>
> > > > > Reviewed-by: Olivier Martin <olivier.mar...@arm.com>
> > > > >
> > > > > I would prefer to let package maintainer, that's me:), to commit
> > > > > code for quality control. Just giving feedback is not enough as
> > we
> > > > > have additional works to do.
> > > >
> > > > I do agree about testing. I would pull in Olivier's change. Test
> > it,
> > > > and only then give him a r-b and tell him it is okay for him to
> > > > commit the change when his series is ready.
> > > >
> > > > But, you can handle contributions differently for the packages you
> > > > maintain.
> > > >
> > > > Except, one note. If the contribution had several patches to your
> > > > package. In that case you should preserve them as separate patches.
> > > >
> > > > > For these series patches submitted by Martin, only this one gets
> > > > > approval at this time. Others need more discussion.
> > > >
> > > > Yes. Normally I would think Olivier would get his whole series
> > > > reviewed and ready before committing it. Maybe it is okay to split
> > > > it in some cases.
> > > >
> > > > -Jordan
> > > >
> > > > > -----Original Message-----
> > > > > From: Justen, Jordan L
> > > > > Sent: Thursday, November 13, 2014 09:25
> > > > > To: Tian, Feng; 'Olivier Martin (olivier.mar...@arm.com)'
> > > > > Cc: edk2-devel@lists.sourceforge.net
> > > > > Subject: Re: [edk2] patch review feedback about
> > FvSimpleFileSystem
> > > > module
> > > > >
> > > > > On 2014-11-11 23:16:21, Tian, Feng wrote:
> > > > > > Hi, Martin
> > > > > >
> > > > > > Here is my review comments on your FvSimpleFileSystem uefi
> > driver.
> > > > Sorry for late response.
> > > > > >
> > > > > > 1. Fixed system hang due to wrong assertions in
> > > > FvSimpleFileSystemDriverStart() and
> FvSimpleFileSystemOpenVolume().
> > > > > > a. change ASSERT (NumChars == GUID_STRING_SIZE); to
> > > > > > ASSERT
> > > > ((NumChars + 1) * sizeof (CHAR16) == GUID_STRING_SIZE);
> > > > > > b. change ASSERT (NumChars == FVFS_VOLUME_LABEL_SIZE);
> > > > > > to ASSERT ((NumChars + 1) * sizeof (CHAR16) ==
> > > > FVFS_VOLUME_LABEL_SIZE); 2. Fixed SimpleFileSystem SCT errors.
> > > > > > a. EFI_FILE_PROTOCOL.GetInfo() should handle
> > > > > > VolumeLabel
> > > > info rather than return EFI_UNSUPPORTED.
> > > > > > b. EFI_FILE_PROTOCOL.GetInfo() should initial Size
> > > > > > field
> > > > of EFI_FILE_SYSTEM_INFO at run time.
> > > > > > c. Some interfaces in EFI_FILE_PROTOCOL should return
> > > > EFI_WRITE_PROTECTED rather than EFI_UNSUPPORTED.
> > > > > > d. EFI_FILE_PROTOCOL.Delete() should return
> > > > EFI_WARN_DELETE_FAILURE rather than EFI_UNSUPPORTED.
> > > > > > 3. Added missing ComponentName protocol interface and modify
> > > > > > code
> > > > to use APIs defined in UefiLib to install DriverBinding and
> > > > ComponentName protocol.
> > > > > > 4. Fixed wrong UnicodeCollation protocol usage.
> > > > > > 5. Added missing function comments.
> > > > > > 6. Added missing user extension meta data in INF file.
> > > > > > 7. Remove BasePathLib dependency and use internal
> > > > > > implementation
> > > > as the move operation for this library and others in ShellPkg need
> > > > more discussions.
> > > > > >
> > > > > > I attached the modified path for your review, will check it in
> > > > > > to
> > > > MdeModulePkg/Universal after pass your review. BasePathLib will be
> > > > on other thread.
> > > > >
> > > > > I wonder what Olivier is supposed to do with this patch. I
> > > > > notice it
> > > > lacks a Contributed-under and Signed-off-by for your changes.
> > > > >
> > > > > I assume you'll add that, and then Olivier will add it to the
> > > > > commit
> > > > messages?
> > > > >
> > > > > Another (I think easier) approach would be to provide feedback
> > and
> > > > let Olivier modify his code based on your suggestions.
> > > > >
> > > > > You might also give your r-b and let Olivier commit the patch
> > > > > series
> > > > since he has commit access. (It seems he had a 3 patch series
> > > > > originally.)
> > > > >
> > > > > -Jordan
> > >
> > >
> > >
> > >
> > >
> > >
> > > --------------------------------------------------------------------
> > > -
> > -
> > > -------- Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT
> > > Server from Actuate! Instantly Supercharge Your Business Reports and
> > > Dashboards with Interactivity, Sharing, Native Excel Exports, App
> > > Integration & more Get technology previously reserved for
> > > billion-dollar corporations, FREE
> > >
> >
> http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.
> > > clktrk
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/edk2-devel
> > > CAUTION: Please confirm that the password protected .zip attachment
> > > which contains the file(s) of type
> > > FvSimpleFileSystemDxe.zip
> > > is legitimate prior to opening. To make sure this message is not
> > > infected with a virus, it is important to verify that you are
> > > expecting the message or else confirm its legitimacy with the sender.
> > >
>
>
>
> ------------------------------------------------------------------------------
> Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from
> Actuate! Instantly Supercharge Your Business Reports and Dashboards with
> Interactivity, Sharing, Native Excel Exports, App Integration & more Get
> technology previously reserved for billion-dollar corporations, FREE
> http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.cl
> ktrk
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
>
> ------------------------------------------------------------------------------
> Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from
> Actuate! Instantly Supercharge Your Business Reports and Dashboards with
> Interactivity, Sharing, Native Excel Exports, App Integration & more Get
> technology previously reserved for billion-dollar corporations, FREE
> http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.cl
> ktrk
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
