Leif, What are the specific warnings that are being triggered by CLANG -Weverything for the unaligned access APIs in BaseLib?
Are there other options than changing the BaseLib API definitions? My two concerns are: 1) Impacts to modules/libs that call the unaligned access functions in BaseLib. Changing I am not sure if changing a parameter from UINTx * to VOID * is backwards compatible for All caller use cases or not. 2) Loss of some type checking as I mentioned below. Thanks, Mike > -----Original Message----- > From: [email protected] [mailto:[email protected]] > Sent: Thursday, May 12, 2016 4:27 PM > To: Kinney, Michael D <[email protected]> > Cc: Leif Lindholm <[email protected]>; [email protected]; Gao, > Liming > <[email protected]>; Ard Biesheuvel <[email protected]> > Subject: Re: [edk2] [RFC] MdePkg: BaseLib: don't use aligned pointers for > unaligned > accessors > > > > On May 12, 2016, at 4:01 PM, Kinney, Michael D <[email protected]> > > wrote: > > > > Leif, > > > > Yes. Typecasts are used by the caller if the type does not match. Here is > > an > > example from the PciCfg2.c that you referenced. > > > > IN OUT VOID *Buffer > > > > // > > // Aligned Pci address access > > // > > WriteUnaligned16 (((UINT16 *) Buffer), PciRead16 (PciLibAddress)); > > > > > > Since Buffer is type VOID *, a typecast to UINT16 * is made. That is valid > > C > > code. > > Mike, > > Leif may be talking about: > > The C Standard, 6.3.2.3, paragraph 7 [ISO/IEC 9899:2011], states: > A pointer to an object or incomplete type may be converted to a pointer to a > different object or incomplete type. If the resulting pointer is not correctly > aligned for the referenced type, the behavior is undefined. > > I always like to reference Chris Lattner's LLVM blog post on undefined > behavior, but > this is not an example of the specific case. But the restrictions on type > conversions > exist to enable TBAA. > http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html > Violating Type Rules: It is undefined behavior to cast an int* to a float* and > dereference it (accessing the "int" as if it were a "float"). C requires that > these > sorts of type conversions happen through memcpy: using pointer casts is not > correct > and undefined behavior results. The rules for this are quite nuanced and I > don't want > to go into the details here (there is an exception for char*, vectors have > special > properties, unions change things, etc). This behavior enables an analysis > known as > "Type-Based Alias Analysis" (TBAA) which is used by a broad range of memory > access > optimizations in the compiler, and can significantly improve performance of > the > generated code. For example, this rule allows clang to optimize this function: > > float *P; > void zero_array() { > int i; > for (i = 0; i < 10000; ++i) > P[i] = 0.0f; > } > > into "memset(P, 0, 40000)". This optimization also allows many loads to be > hoisted > out of loops, common subexpressions to be eliminated, etc. This class of > undefined > behavior can be disabled by passing the -fno-strict-aliasing flag, which > disallows > this analysis. When this flag is passed, Clang is required to compile this > loop into > 10000 4-byte stores (which is several times slower), because it has to assume > that it > is possible for any of the stores to change the value of P, as in something > like > this: > > int main() { > P = (float*)&P; // cast causes TBAA violation in zero_array. > zero_array(); > } > > > This sort of type abuse is pretty uncommon, which is why the standard > committee > decided that the significant performance wins were worth the unexpected > result for > "reasonable" type casts. It is worth pointing out that Java gets the benefits > of > type-based optimizations without these drawbacks because it doesn't have > unsafe > pointer casting in the language at all. > > Thanks, > > Andrew Fish > > > One use case where making these APIs type specific is when a data structure > > is used that is either byte packed or the start of the structure is not > > aligned > (e.g. > > network packet processing). In these cases, the fields may have explicit > > types > such > > as UINT16, UINT32, UINT64, and the compiler does type checking when a > > pointer to > the > > field is passed into the unaligned access functions. > > > > By changing the APIs to be VOID* instead of UINT16*, UINT32*, or UINT64*, > > some type > > checking is lost. The APIs as they are defines now have worked for IPF. > > What is > > different about ARM and AArch64? Are you wanting to remove the requirement > > for the > > explicit typecast by the caller? > > > > > > > > You are correct. PatchCheck.py does not check EDK II C style. The ECC > > tool can be > > used for that. We have discussed adding ECC to PatchCheck.py, but have not > > done > that > > yet. > > > > > > > > Thanks, > > > > Mike > > > > > > > > > >> -----Original Message----- > >> From: Leif Lindholm [mailto:[email protected]] > >> Sent: Thursday, May 12, 2016 3:49 PM > >> To: Kinney, Michael D <[email protected]> > >> Cc: [email protected]; Gao, Liming <[email protected]>; Ard > >> Biesheuvel > >> <[email protected]> > >> Subject: Re: [RFC] MdePkg: BaseLib: don't use aligned pointers for > >> unaligned > >> accessors > >> > >> On Thu, May 12, 2016 at 10:29:34PM +0000, Kinney, Michael D wrote: > >>> Leif, > >>> > >>> Why does the current definition strictly require alignment? > >>> > >>> I know we have used this for a long time with many compilers passing in > >>> unaligned pointers to UINT16, UINT32, UINT64 values without any compiler > >>> warnings/errors. > >> > >> Sure, but that does not make it valid C. > >> > >> Specifying an input argument to be a UINT32 * at least in ARM, AArch64 > >> and IPF land means that the programmer provides an explicit guarantee > >> that the pointer references a naturally aligned location. > >> > >> Also, the reason that edk2 compiles without warnings is that code in > >> the tree makes (also invalid) casts at the call sites to these > >> functions in order to get rid of said warnings. See for example > >> MdeModulePkg/Universal/PcatSingleSegmentPciCfg2Pei/PciCfg2.c > >> > >> (A naive grep throws up 129 instances of explicit casts on lines > >> calling a ReadUnaligned function and 211 instances of ones calling a > >> WriteUnaligned function.) > >> > >>> Also, the patch contains locals that are initialized in the declaration > >>> of the local. We prefer locals to be initialized in the body of the > >>> function. > >> > >> Understood - PatchCheck.py didn't warn me about that :) > >> > >> Regards, > >> > >> Leif > >> > >>> Thanks, > >>> > >>> Mike > >>> > >>>> -----Original Message----- > >>>> From: Leif Lindholm [mailto:[email protected]] > >>>> Sent: Thursday, May 12, 2016 3:22 PM > >>>> To: [email protected] > >>>> Cc: Kinney, Michael D <[email protected]>; Gao, Liming > >>>> <[email protected]>; Ard Biesheuvel <[email protected]> > >>>> Subject: [RFC] MdePkg: BaseLib: don't use aligned pointers for unaligned > >> accessors > >>>> > >>>> The ReadUnaligned##/WriteUnaligned## functions provide a portable way of > >>>> accessing potentially unaligned locations, but the prototypes in > >>>> BaseLib.h all specify strictly aligned pointers. > >>>> > >>>> Change the prototypes as well as the implementations across Ia32/X64, > >>>> ARM/AArch64 and IPF to use VOID * pointers instead. > >>>> > >>>> Contributed-under: TianoCore Contribution Agreement 1.0 > >>>> Signed-off-by: Leif Lindholm <[email protected]> > >>>> --- > >>>> > >>>> Built and tested for ArmVirtPkg for AArch64/ARM DEBUG/RELEASE. > >>>> Build tested only for OvmfPkg for Ia32/X64 DEBUG/RELEASE. > >>>> Not tested at all for IPF. > >>>> > >>>> I am not entirely convinced by the BitFieldWrite32 logic in > >>>> WriteUnaligned24, but this patch is not the place to fix this. > >>>> > >>>> I am also not convinced there is much point in keeping separate > >>>> implementations for IPF and ARM* - the optimization issues mentioned > >>>> in the git log could even have been affected by the incorrect > >>>> pointer types. > >>>> > >>>> This is the first patch triggered by my CLANG -Weverything exercises > >>>> earlier this year. More to come. > >>>> > >>>> MdePkg/Include/Library/BaseLib.h | 16 ++++++------ > >>>> MdePkg/Library/BaseLib/Arm/Unaligned.c | 16 ++++++------ > >>>> MdePkg/Library/BaseLib/Ipf/Unaligned.c | 16 ++++++------ > >>>> MdePkg/Library/BaseLib/Unaligned.c | 48 > >>>> ++++++++++++++++++++++------------ > >>>> 4 files changed, 56 insertions(+), 40 deletions(-) > >>>> > >>>> diff --git a/MdePkg/Include/Library/BaseLib.h > >>>> b/MdePkg/Include/Library/BaseLib.h > >>>> index c41fa78..133656a 100644 > >>>> --- a/MdePkg/Include/Library/BaseLib.h > >>>> +++ b/MdePkg/Include/Library/BaseLib.h > >>>> @@ -2549,7 +2549,7 @@ DivS64x64Remainder ( > >>>> UINT16 > >>>> EFIAPI > >>>> ReadUnaligned16 ( > >>>> - IN CONST UINT16 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ); > >>>> > >>>> > >>>> @@ -2571,7 +2571,7 @@ ReadUnaligned16 ( > >>>> UINT16 > >>>> EFIAPI > >>>> WriteUnaligned16 ( > >>>> - OUT UINT16 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT16 Value > >>>> ); > >>>> > >>>> @@ -2592,7 +2592,7 @@ WriteUnaligned16 ( > >>>> UINT32 > >>>> EFIAPI > >>>> ReadUnaligned24 ( > >>>> - IN CONST UINT32 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ); > >>>> > >>>> > >>>> @@ -2614,7 +2614,7 @@ ReadUnaligned24 ( > >>>> UINT32 > >>>> EFIAPI > >>>> WriteUnaligned24 ( > >>>> - OUT UINT32 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT32 Value > >>>> ); > >>>> > >>>> @@ -2635,7 +2635,7 @@ WriteUnaligned24 ( > >>>> UINT32 > >>>> EFIAPI > >>>> ReadUnaligned32 ( > >>>> - IN CONST UINT32 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ); > >>>> > >>>> > >>>> @@ -2657,7 +2657,7 @@ ReadUnaligned32 ( > >>>> UINT32 > >>>> EFIAPI > >>>> WriteUnaligned32 ( > >>>> - OUT UINT32 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT32 Value > >>>> ); > >>>> > >>>> @@ -2678,7 +2678,7 @@ WriteUnaligned32 ( > >>>> UINT64 > >>>> EFIAPI > >>>> ReadUnaligned64 ( > >>>> - IN CONST UINT64 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ); > >>>> > >>>> > >>>> @@ -2700,7 +2700,7 @@ ReadUnaligned64 ( > >>>> UINT64 > >>>> EFIAPI > >>>> WriteUnaligned64 ( > >>>> - OUT UINT64 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT64 Value > >>>> ); > >>>> > >>>> diff --git a/MdePkg/Library/BaseLib/Arm/Unaligned.c > >>>> b/MdePkg/Library/BaseLib/Arm/Unaligned.c > >>>> index 34f1732..ef9efbc 100644 > >>>> --- a/MdePkg/Library/BaseLib/Arm/Unaligned.c > >>>> +++ b/MdePkg/Library/BaseLib/Arm/Unaligned.c > >>>> @@ -33,7 +33,7 @@ > >>>> UINT16 > >>>> EFIAPI > >>>> ReadUnaligned16 ( > >>>> - IN CONST UINT16 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> volatile UINT8 LowerByte; > >>>> @@ -65,7 +65,7 @@ ReadUnaligned16 ( > >>>> UINT16 > >>>> EFIAPI > >>>> WriteUnaligned16 ( > >>>> - OUT UINT16 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT16 Value > >>>> ) > >>>> { > >>>> @@ -93,7 +93,7 @@ WriteUnaligned16 ( > >>>> UINT32 > >>>> EFIAPI > >>>> ReadUnaligned24 ( > >>>> - IN CONST UINT32 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> ASSERT (Buffer != NULL); > >>>> @@ -122,7 +122,7 @@ ReadUnaligned24 ( > >>>> UINT32 > >>>> EFIAPI > >>>> WriteUnaligned24 ( > >>>> - OUT UINT32 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT32 Value > >>>> ) > >>>> { > >>>> @@ -149,7 +149,7 @@ WriteUnaligned24 ( > >>>> UINT32 > >>>> EFIAPI > >>>> ReadUnaligned32 ( > >>>> - IN CONST UINT32 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> UINT16 LowerBytes; > >>>> @@ -181,7 +181,7 @@ ReadUnaligned32 ( > >>>> UINT32 > >>>> EFIAPI > >>>> WriteUnaligned32 ( > >>>> - OUT UINT32 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT32 Value > >>>> ) > >>>> { > >>>> @@ -208,7 +208,7 @@ WriteUnaligned32 ( > >>>> UINT64 > >>>> EFIAPI > >>>> ReadUnaligned64 ( > >>>> - IN CONST UINT64 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> UINT32 LowerBytes; > >>>> @@ -240,7 +240,7 @@ ReadUnaligned64 ( > >>>> UINT64 > >>>> EFIAPI > >>>> WriteUnaligned64 ( > >>>> - OUT UINT64 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT64 Value > >>>> ) > >>>> { > >>>> diff --git a/MdePkg/Library/BaseLib/Ipf/Unaligned.c > >>>> b/MdePkg/Library/BaseLib/Ipf/Unaligned.c > >>>> index 7d0d8dd..5ba8029 100644 > >>>> --- a/MdePkg/Library/BaseLib/Ipf/Unaligned.c > >>>> +++ b/MdePkg/Library/BaseLib/Ipf/Unaligned.c > >>>> @@ -30,7 +30,7 @@ > >>>> UINT16 > >>>> EFIAPI > >>>> ReadUnaligned16 ( > >>>> - IN CONST UINT16 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> ASSERT (Buffer != NULL); > >>>> @@ -56,7 +56,7 @@ ReadUnaligned16 ( > >>>> UINT16 > >>>> EFIAPI > >>>> WriteUnaligned16 ( > >>>> - OUT UINT16 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT16 Value > >>>> ) > >>>> { > >>>> @@ -84,7 +84,7 @@ WriteUnaligned16 ( > >>>> UINT32 > >>>> EFIAPI > >>>> ReadUnaligned24 ( > >>>> - IN CONST UINT32 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> ASSERT (Buffer != NULL); > >>>> @@ -113,7 +113,7 @@ ReadUnaligned24 ( > >>>> UINT32 > >>>> EFIAPI > >>>> WriteUnaligned24 ( > >>>> - OUT UINT32 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT32 Value > >>>> ) > >>>> { > >>>> @@ -140,7 +140,7 @@ WriteUnaligned24 ( > >>>> UINT32 > >>>> EFIAPI > >>>> ReadUnaligned32 ( > >>>> - IN CONST UINT32 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> UINT16 LowerBytes; > >>>> @@ -172,7 +172,7 @@ ReadUnaligned32 ( > >>>> UINT32 > >>>> EFIAPI > >>>> WriteUnaligned32 ( > >>>> - OUT UINT32 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT32 Value > >>>> ) > >>>> { > >>>> @@ -199,7 +199,7 @@ WriteUnaligned32 ( > >>>> UINT64 > >>>> EFIAPI > >>>> ReadUnaligned64 ( > >>>> - IN CONST UINT64 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> UINT32 LowerBytes; > >>>> @@ -231,7 +231,7 @@ ReadUnaligned64 ( > >>>> UINT64 > >>>> EFIAPI > >>>> WriteUnaligned64 ( > >>>> - OUT UINT64 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT64 Value > >>>> ) > >>>> { > >>>> diff --git a/MdePkg/Library/BaseLib/Unaligned.c > >> b/MdePkg/Library/BaseLib/Unaligned.c > >>>> index 68dafa6..ed58861 100644 > >>>> --- a/MdePkg/Library/BaseLib/Unaligned.c > >>>> +++ b/MdePkg/Library/BaseLib/Unaligned.c > >>>> @@ -32,12 +32,14 @@ > >>>> UINT16 > >>>> EFIAPI > >>>> ReadUnaligned16 ( > >>>> - IN CONST UINT16 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> + CONST UINT16 *Input = Buffer; > >>>> + > >>>> ASSERT (Buffer != NULL); > >>>> > >>>> - return *Buffer; > >>>> + return *Input; > >>>> } > >>>> > >>>> /** > >>>> @@ -58,13 +60,15 @@ ReadUnaligned16 ( > >>>> UINT16 > >>>> EFIAPI > >>>> WriteUnaligned16 ( > >>>> - OUT UINT16 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT16 Value > >>>> ) > >>>> { > >>>> + UINT16 *Output = Buffer; > >>>> + > >>>> ASSERT (Buffer != NULL); > >>>> > >>>> - return *Buffer = Value; > >>>> + return *Output = Value; > >>>> } > >>>> > >>>> /** > >>>> @@ -83,12 +87,14 @@ WriteUnaligned16 ( > >>>> UINT32 > >>>> EFIAPI > >>>> ReadUnaligned24 ( > >>>> - IN CONST UINT32 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> + CONST UINT32 *Input = Buffer; > >>>> + > >>>> ASSERT (Buffer != NULL); > >>>> > >>>> - return *Buffer & 0xffffff; > >>>> + return *Input & 0xffffff; > >>>> } > >>>> > >>>> /** > >>>> @@ -109,13 +115,15 @@ ReadUnaligned24 ( > >>>> UINT32 > >>>> EFIAPI > >>>> WriteUnaligned24 ( > >>>> - OUT UINT32 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT32 Value > >>>> ) > >>>> { > >>>> + UINT32 *Output = Buffer; > >>>> + > >>>> ASSERT (Buffer != NULL); > >>>> > >>>> - *Buffer = BitFieldWrite32 (*Buffer, 0, 23, Value); > >>>> + *Output = BitFieldWrite32 (*Output, 0, 23, Value); > >>>> return Value; > >>>> } > >>>> > >>>> @@ -135,12 +143,14 @@ WriteUnaligned24 ( > >>>> UINT32 > >>>> EFIAPI > >>>> ReadUnaligned32 ( > >>>> - IN CONST UINT32 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> + CONST UINT32 *Input = Buffer; > >>>> + > >>>> ASSERT (Buffer != NULL); > >>>> > >>>> - return *Buffer; > >>>> + return *Input; > >>>> } > >>>> > >>>> /** > >>>> @@ -161,13 +171,15 @@ ReadUnaligned32 ( > >>>> UINT32 > >>>> EFIAPI > >>>> WriteUnaligned32 ( > >>>> - OUT UINT32 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT32 Value > >>>> ) > >>>> { > >>>> + UINT32 *Output = Buffer; > >>>> + > >>>> ASSERT (Buffer != NULL); > >>>> > >>>> - return *Buffer = Value; > >>>> + return *Output = Value; > >>>> } > >>>> > >>>> /** > >>>> @@ -186,12 +198,14 @@ WriteUnaligned32 ( > >>>> UINT64 > >>>> EFIAPI > >>>> ReadUnaligned64 ( > >>>> - IN CONST UINT64 *Buffer > >>>> + IN CONST VOID *Buffer > >>>> ) > >>>> { > >>>> + CONST UINT64 *Input = Buffer; > >>>> + > >>>> ASSERT (Buffer != NULL); > >>>> > >>>> - return *Buffer; > >>>> + return *Input; > >>>> } > >>>> > >>>> /** > >>>> @@ -212,11 +226,13 @@ ReadUnaligned64 ( > >>>> UINT64 > >>>> EFIAPI > >>>> WriteUnaligned64 ( > >>>> - OUT UINT64 *Buffer, > >>>> + OUT VOID *Buffer, > >>>> IN UINT64 Value > >>>> ) > >>>> { > >>>> + UINT64 *Output = Buffer; > >>>> + > >>>> ASSERT (Buffer != NULL); > >>>> > >>>> - return *Buffer = Value; > >>>> + return *Output = Value; > >>>> } > >>>> -- > >>>> 2.1.4 > >>> > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
