[edk2,RFC] MdePkg: BaseLib: don't use aligned pointers for unaligned accessors

Message ID 1463091740-1015-1-git-send-email-leif.lindholm@linaro.org
State New
Headers show

Commit Message

Leif Lindholm May 12, 2016, 10:22 p.m.
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 <leif.lindholm@linaro.org>

---

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(-)

-- 
2.1.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm May 12, 2016, 10:49 p.m. | #1
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:leif.lindholm@linaro.org]

> > Sent: Thursday, May 12, 2016 3:22 PM

> > To: edk2-devel@lists.01.org

> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > 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 <leif.lindholm@linaro.org>

> > ---

> > 

> > 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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek May 17, 2016, 6:15 p.m. | #2
On 05/17/16 19:38, Kinney, Michael D wrote:
> 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.


I have not chimed in on this thread yet, but perhaps I can add something
now.

The practical point can be expressed in one sentence:

  If the result of an expression that has type pointer-to-X is not
  suitably aligned for type X, then evaluating the expression (*not*
  dereferencing its result!) is already undefined behavior.

This means that the prototypes of the ReadUnalignedXX() functions are
broken from the start, in the library class header file. These functions
are supposed to be called when the caller is either unsure if the
argument is suitably aligned, or when the caller is sure that the
argument is unaligned.

Unfortunately, in these cases, the evaluation of the arguments (before
the function call is made) is already unsafe.

The patch would make things safe (as far as strict ISO C is concerned),
but I agree it would also lose some size checking, which is not beneficial.

I think we should be looking into convincing CLANG (with some command
line option) that the *evaluation* of such unaligned pointers is always
safe (while their *dereferencing* may of course not be). Such an option
should prevent CLANG from both generating incorrect code and emitting
warnings.

I am suggesting such an option because we already diverge from standard
C with

  -fno-strict-aliasing

This option (which is also used for the XCODE toolchain family) prevents
the compiler from enforcing / exploiting the effective type rules in ISO
C . Those rules are extremely obscure, and practically impossible to
conform to in firmware code (although I definitely try in code I write
nonetheless).

I think this is a very similar case. On today's platforms, evaluating
(not dereferencing!) invalid pointers is generally safe.

int main(void)
{
  uint32_t x;
  uint16_t *p;

  /*
   * strictly speaking, undefined behavior, but watch the world not
   * give a darn
   */
  p = (uint16_t *)((uint8_t *)&x + 1);
  return 0;
}


Hence we need a flag so that CLANG allows such pointer use, without
insisting on strict standards conformance. Maybe "-Wno-cast-align" would
suffice; I'm not sure.

An interesting URL I've found is:

http://lists.llvm.org/pipermail/llvm-dev/2016-January/094012.html

Thanks
Laszlo

>> -----Original Message-----

>> From: afish@apple.com [mailto:afish@apple.com]

>> Sent: Thursday, May 12, 2016 4:27 PM

>> To: Kinney, Michael D <michael.d.kinney@intel.com>

>> Cc: Leif Lindholm <leif.lindholm@linaro.org>; edk2-devel@lists.01.org; Gao, Liming

>> <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> 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 <michael.d.kinney@intel.com> 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:leif.lindholm@linaro.org]

>>>> Sent: Thursday, May 12, 2016 3:49 PM

>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>

>>>> Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel

>>>> <ard.biesheuvel@linaro.org>

>>>> 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:leif.lindholm@linaro.org]

>>>>>> Sent: Thursday, May 12, 2016 3:22 PM

>>>>>> To: edk2-devel@lists.01.org

>>>>>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming

>>>>>> <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>>>>> 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 <leif.lindholm@linaro.org>

>>>>>> ---

>>>>>>

>>>>>> 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

>>> edk2-devel@lists.01.org

>>> https://lists.01.org/mailman/listinfo/edk2-devel

> 

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm May 23, 2016, 3:12 p.m. | #3
Hi Mike,

Apologies for going silent - I spent last week in China, with very
restricted access to my primary email account.

On Tue, May 17, 2016 at 05:38:21PM +0000, Kinney, Michael D wrote:
> Leif,

> 

> What are the specific warnings that are being triggered by CLANG

> -Weverything for the unaligned access APIs in BaseLib?


The -Weverything is actually a sidetrack here, it was just that some
of the many casts were implicitly sign-changing, which generated
warnings and caused me to have a closer look.
This does however display another way in which the existing code is
incorrect.

> 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.


It is. It is semantically equivalent to changing them to a CHAR *.
"A pointer to void shall have the same representation and alignment
requirements as a pointer to a character type." (6.2.5-28 in C11,
nearby in previous revisions of the language.)

Which is what the interface is meant to represent; providing
single-byte-aligned accesses for types that would normally require
more.

> 2) Loss of some type checking as I mentioned below.


Basline remains is - this is not valid C code, and I was entirely
serious when I said I think the "optimization errors" that caused the
split into a separate implementation for ARM* was caused by the
incorrect explicit pointer alignment.

The current implementatiom provides the compiler with information
about the underlying data structures which can be factually incorrect.
So the type checking is already explicitly disabled.

If people would be happy with it, I could submit a new version that
gets rid of all of the implementations and replaces it with a simple
header file. If that is not fine (code implementation in header
files), I could just fix MdePkg/Library/BaseLib/Unaligned.c and drop
the ARM/IPF versions?

For ARM, we build everything with -mno-unaligned-access, and for
AARCH64 we build with -mstrict-align in CC_XIPFLAGS. So before our MMU
is enabled (and unaligned accesses are fine), the compiler will never
try to be clever, and afterwards it works as expected. The current
implementation however deprives it of the information it needs to
guarantee it is able to perform its work correctly.

Regards,

Leif

> Thanks,

> 

> Mike

> 

> > -----Original Message-----

> > From: afish@apple.com [mailto:afish@apple.com]

> > Sent: Thursday, May 12, 2016 4:27 PM

> > To: Kinney, Michael D <michael.d.kinney@intel.com>

> > Cc: Leif Lindholm <leif.lindholm@linaro.org>; edk2-devel@lists.01.org; Gao, Liming

> > <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > 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 <michael.d.kinney@intel.com> 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:leif.lindholm@linaro.org]

> > >> Sent: Thursday, May 12, 2016 3:49 PM

> > >> To: Kinney, Michael D <michael.d.kinney@intel.com>

> > >> Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel

> > >> <ard.biesheuvel@linaro.org>

> > >> 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:leif.lindholm@linaro.org]

> > >>>> Sent: Thursday, May 12, 2016 3:22 PM

> > >>>> To: edk2-devel@lists.01.org

> > >>>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > >>>> <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > >>>> 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 <leif.lindholm@linaro.org>

> > >>>> ---

> > >>>>

> > >>>> 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

> > > edk2-devel@lists.01.org

> > > https://lists.01.org/mailman/listinfo/edk2-devel

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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;
 }