Fix the code causing -Wpragma-pack-suspicious-include warning

[rosetta-jpn]

When gmmlib is compiled with media-driver, the compilar complians the code because of -Wpragma-pack-suspicious-include .
There are a couple of headers code that causes the compiler warning. For example,

gmmlib/Source/GmmLib/inc/External/Common/GmmResourceInfoExt.h:195:14: error: non-default #pragma pack value changes the alignment of struct or union members in the included file [-Werror,-Wpragma-pack-s
uspicious-include]
  195 |     #include "External/Linux/GmmResourceInfoLinExt.h"
      |              ^
gmmlib/Source/GmmLib/inc/External/Common/GmmResourceInfoExt.h:39:9: note: previous '#pragma pack' directive that modifies alignment is here
   39 | #pragma pack(push, 8)

[johnmach]

Hi @rosetta-jpn

Thank you for highlighting your interest in Gmmlib.
We appreciate your suggestions. We will have an internal review and decide on the best possible way to help address your recommendation soon.

[pvb-intel]

Hi @rosetta-jpn

To understand this issue better, could you please provide the following details as we are not able to reproduce this issue.

1. What is the operating system on which it is built?
2. Can you share the build commands and cmake flags.
3. What is the build type (Release/Debug) and share the build logs as well if available.
4. What is the GmmLib Version on which you experiencing this issue. ?

[rosetta-jpn]

I am building gmmlib with media-driver in Android (AOSP), where Android.bp (soong) is used instead of cmake.

The build warning seems to be independent to build system and platoform.
Have you built with -Wall?

[JeevakaPrabu]

@rosetta-jpn @prahaladhan12 @johnmach please review the fix proposal #135

[rosetta-jpn]

Additional fix is needed. After the following fix, the build warning doesn't happen due to gmmlib.
I am not sure that this is a proper fix though.

diff --git a/Source/GmmLib/inc/External/Common/GmmClientContext.h b/Source/GmmLib/inc/External/Common/GmmClientContext.h
index 9907b7d..9210a77 100644
--- a/Source/GmmLib/inc/External/Common/GmmClientContext.h
+++ b/Source/GmmLib/inc/External/Common/GmmClientContext.h
@@ -24,6 +24,9 @@ OTHER DEALINGS IN THE SOFTWARE.
 #include "GmmCommonExt.h"
 #include "GmmInfo.h"
 
+// Set packing alignment
+#pragma pack(push, 8)
+
 //////////////////////////////////////////////////////////////////
 // Memory Allocators and DeAllocators for Gmm Create Objects
 // This is needed for Clients who want to construct using their
@@ -80,7 +83,7 @@ namespace GmmLib
     {
     protected:
         GMM_CLIENT                       ClientType;
-        ///< Placeholders for storing UMD context. Actual UMD context that needs to be stored here is 
+        ///< Placeholders for storing UMD context. Actual UMD context that needs to be stored here is
         union
         {
             void *pUmdAdapter;
@@ -155,7 +158,7 @@ namespace GmmLib
         /* PageTableMgr Creation and Destroy API's */
         GMM_VIRTUAL void GMM_STDCALL                    DestroyPageTblMgrObject(GMM_PAGETABLE_MGR* pPageTableMgr);
 
-        
+
 #ifdef GMM_LIB_DLL
         /* ResourceInfo and PageTableMgr Create and Destroy APIs with Client provided Memory Allocators */
         GMM_VIRTUAL GMM_RESOURCE_INFO* GMM_STDCALL       CreateResInfoObject(GMM_RESCREATE_PARAMS *pCreateParams,
@@ -181,7 +184,7 @@ namespace GmmLib
 #endif
 	GMM_VIRTUAL uint32_t GMM_STDCALL CachePolicyGetPATIndex(GMM_RESOURCE_INFO *pResInfo, GMM_RESOURCE_USAGE_TYPE Usage, bool *pCompressionEnable, bool IsCpuCacheable);
         GMM_VIRTUAL const SWIZZLE_DESCRIPTOR *GMM_STDCALL GetSwizzleDesc(EXTERNAL_SWIZZLE_NAME ExternalSwizzleName, EXTERNAL_RES_TYPE ResType, uint8_t bpe, bool isStdSwizzle = false);
-	
+
 	GMM_VIRTUAL void GMM_STDCALL            GmmSetAIL(GMM_AIL_STRUCT *pAilFlags);
 	GMM_VIRTUAL const uint64_t *GMM_STDCALL GmmGetAIL();
     };
@@ -200,7 +203,7 @@ extern "C" {
 #endif
 
     /* ClientContext will be unique to each client */
-    GMM_CLIENT_CONTEXT *GMM_STDCALL GmmCreateClientContextForAdapter(GMM_CLIENT ClientType, 
+    GMM_CLIENT_CONTEXT *GMM_STDCALL GmmCreateClientContextForAdapter(GMM_CLIENT ClientType,
 		                               ADAPTER_BDF sBdf, const void *_pSkuTable);
     void GMM_STDCALL GmmDeleteClientContext(GMM_CLIENT_CONTEXT *pGmmClientContext);
 
@@ -232,3 +235,5 @@ extern "C" {
 }
 #endif
 
+// Reset packing alignment to project default
+#pragma pack(pop)
diff --git a/Source/GmmLib/inc/External/Common/GmmInfo.h b/Source/GmmLib/inc/External/Common/GmmInfo.h
index 3b32b78..b204098 100644
--- a/Source/GmmLib/inc/External/Common/GmmInfo.h
+++ b/Source/GmmLib/inc/External/Common/GmmInfo.h
@@ -55,11 +55,14 @@ typedef struct GMM_UMD_CONTEXT_REC
     uint32_t   TBD3;
 } GMM_UMD_CONTEXT;
 
+#pragma pack(pop)
 
 #if (!defined(__GMM_KMD__) && !defined(GMM_UNIFIED_LIB))
 #include "GmmClientContext.h"
 #endif
 
+#pragma pack(push, 8)
+
 //===========================================================================
 // typedef:
 //      GMM_GLOBAL_CONTEXT
@@ -575,9 +578,9 @@ namespace GmmLib
     GMM_TEXTURE_CALC* GMM_STDCALL CreateTextureCalc(PLATFORM Platform, bool Override);
     GMM_PLATFORM_INFO_CLASS *GMM_STDCALL CreatePlatformInfo(PLATFORM Platform, bool Override);
 
-    private: 
+    private:
         void GMM_STDCALL OverrideSkuWa();
-        
+
     public:
     /////////////////////////////////////////////////////////////////////////
     /// Returns private PAT table array ptr
@@ -637,7 +640,7 @@ typedef struct _GMM_ADAPTER_INFO_
     _GMM_ADAPTER_INFO_  *pNext;                             // Linked List Next pointer to point to the Next Adapter node in the List
 
 }GMM_ADAPTER_INFO;
-    
+
 ////////////////////////////////////////////////////////////////////////////////////
 /// Multi Adpater Context to hold data related to Multiple Adapters in the system
 /// Contains functions and members that are needed to support Multi-Adapter.
@@ -714,7 +717,7 @@ void GMM_STDCALL GmmLinkKmdContextToGlobalInfo(GMM_GLOBAL_CONTEXT *pGmmLibContex
 #ifdef __cplusplus
 extern "C" {
 #endif /*__cplusplus*/
-	
+
     const GMM_PLATFORM_INFO* GMM_STDCALL GmmGetPlatformInfo(GMM_GLOBAL_CONTEXT *pGmmLibContext);
     const GMM_CACHE_POLICY_ELEMENT* GmmGetCachePolicyUsage(GMM_GLOBAL_CONTEXT *pGmmLibContext);
           GMM_TEXTURE_CALC*         GmmGetTextureCalc(GMM_GLOBAL_CONTEXT *pGmmLibContext);
@@ -722,7 +725,7 @@ extern "C" {
     const WA_TABLE*                 GmmGetWaTable(GMM_GLOBAL_CONTEXT *pGmmLibContext);
     const GT_SYSTEM_INFO*           GmmGetGtSysInfo(GMM_GLOBAL_CONTEXT *pGmmLibContext);
 
-#ifdef __GMM_KMD__ 
+#ifdef __GMM_KMD__
     int32_t             GmmGetPrivatePATTableMemoryType(GMM_GLOBAL_CONTEXT *pGmmLibContext, GMM_GFX_PAT_TYPE PatType);
     GMM_CONTEXT*        GmmGetGmmKmdContext(GMM_GLOBAL_CONTEXT *pGmmLibContext);
     GMM_GTT_CONTEXT*    GmmGetGttContext(GMM_GLOBAL_CONTEXT *pGmmLibContext);

[johnmach]

Thanks @JeevakaPrabu and @rosetta-jpn for proposing some fixes.

@rosetta-jpn the additional fix you recommended could break the backward compatibility of the default packed struct(default alignment). From you additional fix you are forcing alignment to 8bytes which was earlier default aligned.?

@rosetta-jpn Do you still see any issues with fix proposed in #135? Please provide more information on the failure/issue/regression.

At the same time, @JeevakaPrabu, Could you please verify the above additional fix? Did you not hit this error/warning for ClientContext.h inclusion?

Seems like Rosetta-jpn is trying to apply the same logic for the ClientContext.h file which has an interface class.

We would prioritize stable ABI here.

[rosetta-jpn]

@rosetta-jpn Do you still see any issues with fix proposed in #135? Please provide more information on the failure/issue/regression.

Yes, the above change is required in addition to 60f9a7a.

@rosetta-jpn the additional fix you recommended could break the backward compatibility of the default packed struct(default alignment). From you additional fix you are forcing alignment to 8bytes which was earlier default aligned.?

The problem is caused because no pack was not defined in GmmClientContext.h and it is included in GmmInfo.h after pack() is defined.

My above change is
Before:
in GmmInfo.h

pack(8)
#include "GmmClientContext.h"
pack(pop)

After:
in GmmInfo.h

pack(8)
+pack(pop)
#include "GmmClientContext.h"
+pack(8)
pack(pop)

in GmmClientContext.h

+pack(8)
+pack(pop)

So if I understand correctly, the alignment settings should not be changed in either GmmInfo.h or GmmClientContext.h.

[JeevakaPrabu]

@rosetta-jpn GmmClientContext.h will only be included if both GMM_KMD and GMM_UNIFIED_LIB are not defined. We are not hitting this path, as GMM_UNIFIED_LIB is defined. Believe #135 is enough to fix the issue but I agree it's better to do this change, so it doesn't fall through the cracks when both are not defined.

[rosetta-jpn]

Hi @JeevakaPrabu , the macro definition is out of the library control.
The header file might be included in the code base which doesn't define the macro.
Actually, intel/media-driver doesn't define the macro and causes the warning.
Could you include the change so the warning doesn't happen regardless of macro definition status?