Possibly incorrect size in memory profile structure

Possibly incorrect size in memory profile structure

[Marvin Häuser]

Good day,

I have a question regarding a part of the memory profiling code. Namely 
this piece of code [1] introduced storing the PDB file name among the 
driver information. This is implement by a string pointer in the 
"MEMORY_PROFILE_DRIVER_INFO_DATA" structure (which embeds the 
"MEMORY_PROFILE_DRIVER_INFO" structure [2]). The length of the embedded 
"MEMORY_PROFILE_DRIVER_INFO" instance is set to the header size plus the 
8B-aligned size of the PDB name [3], albeit its storage is not used for 
the PDB file name, and the storage that is used instead is not aligned 
by 8B [4]. Ominously, the interior structure does have a comment that 
indicates it is (or was) supposed to hold the PDB file name at some 
point [5].

I do not see that concept being used, and instead see the previously 
described way of storage, so I believe the aligned size is a forgotten 
piece from a refactoring. I imagine early on, the interior structure was 
supposed to hold the PDB name, and the alignment was needed to satisfy 
the following data's requirements. However, the change to the exterior 
storage should make this superfluous, and the specified size seems to be 
incorrect in all cases.

Can you please give this a quick look and help me determine whether this 
is a bug? Thank you for your time!

Best regards,
Marvin

[1] 
https://github.com/tianocore/edk2/commit/1d60fe96422206d37e1d74198bb11b2cf6195157#diff-b42ade68f10fa42dfa25570f0f9a165db4b974877c98d8845e384a40252ec220R407-R428

[2] 
https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c#L25

[3] 
https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c#L417

[4] 
https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c#L404

[5] 
https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Include/Guid/MemoryProfile.h#L59

Re: [edk2-devel] Possibly incorrect size in memory profile structure

[Zeng, Star]

Not sure I can understand your question correctly. But try best to recall and comment since not on it for long time. 😊

[2], [3] and [4] are all for interior storage management by MEMORY_PROFILE_DRIVER_INFO_DATA, MEMORY_PROFILE_DRIVER_INFO_DATA.PdbString points to the PDB string.

Exterior storage are managed by ProfileProtocolGetData() and MemoryProfileCopyData() from interior storage, 8B alignment are for exterior storage, PDB string is at the end of MEMORY_PROFILE_DRIVER_INFO structure.


Thanks,
Star
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
Sent: 2021年6月19日 18:56
To: Zeng, Star <star.zeng@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Subject: [edk2-devel] Possibly incorrect size in memory profile structure

Good day,

I have a question regarding a part of the memory profiling code. Namely this piece of code [1] introduced storing the PDB file name among the driver information. This is implement by a string pointer in the "MEMORY_PROFILE_DRIVER_INFO_DATA" structure (which embeds the "MEMORY_PROFILE_DRIVER_INFO" structure [2]). The length of the embedded "MEMORY_PROFILE_DRIVER_INFO" instance is set to the header size plus the 8B-aligned size of the PDB name [3], albeit its storage is not used for the PDB file name, and the storage that is used instead is not aligned by 8B [4]. Ominously, the interior structure does have a comment that indicates it is (or was) supposed to hold the PDB file name at some point [5].

I do not see that concept being used, and instead see the previously described way of storage, so I believe the aligned size is a forgotten piece from a refactoring. I imagine early on, the interior structure was supposed to hold the PDB name, and the alignment was needed to satisfy the following data's requirements. However, the change to the exterior storage should make this superfluous, and the specified size seems to be incorrect in all cases.

Can you please give this a quick look and help me determine whether this is a bug? Thank you for your time!

Best regards,
Marvin

[1]
https://github.com/tianocore/edk2/commit/1d60fe96422206d37e1d74198bb11b2cf6195157#diff-b42ade68f10fa42dfa25570f0f9a165db4b974877c98d8845e384a40252ec220R407-R428

[2]
https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c#L25

[3]
https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c#L417

[4]
https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c#L404

[5]
https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Include/Guid/MemoryProfile.h#L59

Re: [edk2-devel] Possibly incorrect size in memory profile structure

[Marvin Häuser]

Ahhh, I see it. The size does to describe the data structure that is 
existent at that point in time, but the one that is created by 
MemoryProfileCopyData() later, and the alignment is applied because it 
ends up as a flat structure array. Makes sense, thank you a lot!

Best regards,
Marvin

On 22.06.21 12:20, Zeng, Star wrote:
> Not sure I can understand your question correctly. But try best to recall and comment since not on it for long time. 😊
>
> [2], [3] and [4] are all for interior storage management by MEMORY_PROFILE_DRIVER_INFO_DATA, MEMORY_PROFILE_DRIVER_INFO_DATA.PdbString points to the PDB string.
>
> Exterior storage are managed by ProfileProtocolGetData() and MemoryProfileCopyData() from interior storage, 8B alignment are for exterior storage, PDB string is at the end of MEMORY_PROFILE_DRIVER_INFO structure.
>
>
> Thanks,
> Star
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
> Sent: 2021年6月19日 18:56
> To: Zeng, Star <star.zeng@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Subject: [edk2-devel] Possibly incorrect size in memory profile structure
>
> Good day,
>
> I have a question regarding a part of the memory profiling code. Namely this piece of code [1] introduced storing the PDB file name among the driver information. This is implement by a string pointer in the "MEMORY_PROFILE_DRIVER_INFO_DATA" structure (which embeds the "MEMORY_PROFILE_DRIVER_INFO" structure [2]). The length of the embedded "MEMORY_PROFILE_DRIVER_INFO" instance is set to the header size plus the 8B-aligned size of the PDB name [3], albeit its storage is not used for the PDB file name, and the storage that is used instead is not aligned by 8B [4]. Ominously, the interior structure does have a comment that indicates it is (or was) supposed to hold the PDB file name at some point [5].
>
> I do not see that concept being used, and instead see the previously described way of storage, so I believe the aligned size is a forgotten piece from a refactoring. I imagine early on, the interior structure was supposed to hold the PDB name, and the alignment was needed to satisfy the following data's requirements. However, the change to the exterior storage should make this superfluous, and the specified size seems to be incorrect in all cases.
>
> Can you please give this a quick look and help me determine whether this is a bug? Thank you for your time!
>
> Best regards,
> Marvin
>
> [1]
> https://github.com/tianocore/edk2/commit/1d60fe96422206d37e1d74198bb11b2cf6195157#diff-b42ade68f10fa42dfa25570f0f9a165db4b974877c98d8845e384a40252ec220R407-R428
>
> [2]
> https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c#L25
>
> [3]
> https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c#L417
>
> [4]
> https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c#L404
>
> [5]
> https://github.com/tianocore/edk2/blob/a63914d3f603580e5aeceb5edbafe56688210141/MdeModulePkg/Include/Guid/MemoryProfile.h#L59
>
>
> 
>
>