[edk2] [Patch] SourceLevelDebugPkg/DebugAgent: Add typecast to fix sign extension

[Jeff Fan]

OffsetHigh is 16bit value and its type is UINT32 and defined in structure.
If its high bit is 1, it will sign extension when do 16-bit left-shift
operation. Need to typecast if after left-shift operation on GCC.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <***@intel.com>
CC: Ruiyu Ni <***@intel.com>
---
SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c | 2 +-
.../Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c | 2 +-
SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c b/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c
index f7fbb3c..35fb5e6 100644
--- a/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c
+++ b/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c
@@ -512,7 +512,7 @@ InitializeDebugAgent (
Ia32Idtr = (IA32_DESCRIPTOR *) Context;
Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
MailboxLocation = (UINT64 *) (UINTN) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
- (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
+ (UINT32) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
Mailbox = (DEBUG_AGENT_MAILBOX *)(UINTN)(*MailboxLocation);
VerifyMailboxChecksum (Mailbox);
}
diff --git a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
index e77ff72..fcc7a4b 100644
--- a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
+++ b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
@@ -579,7 +579,7 @@ InitializeDebugAgent (
Ia32Idtr = (IA32_DESCRIPTOR *) Context;
Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
MailboxLocationPointer = (UINT64 *) (UINTN) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
- (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
+ (UINT32) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
Mailbox = (DEBUG_AGENT_MAILBOX *) (UINTN)(*MailboxLocationPointer);
//
// Mailbox should valid and setup before executing thunk code
diff --git a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
index 6ac5f88..3a759f9 100644
--- a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
+++ b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
@@ -328,7 +328,7 @@ InitializeDebugAgent (
Ia32Idtr = (IA32_DESCRIPTOR *) Context;
Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
MailboxLocation = (UINT64 *) (UINTN) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
- (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
+ (UINT32) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
mMailboxPointer = (DEBUG_AGENT_MAILBOX *)(UINTN)(*MailboxLocation);
VerifyMailboxChecksum (mMailboxPointer);
//

[Ni, Ruiyu]

-----Original Message-----
From: Fan, Jeff
Sent: Thursday, June 25, 2015 3:59 PM
Cc: Ni, Ruiyu
Subject: [Patch] SourceLevelDebugPkg/DebugAgent: Add typecast to fix sign
extension
OffsetHigh is 16bit value and its type is UINT32 and defined in structure.
If its high bit is 1, it will sign extension when do 16-bit left-shift
operation. Need to typecast if after left-shift operation on GCC.
Contributed-under: TianoCore Contribution Agreement 1.0
---
SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgen
tLib.c | 2 +-
.../Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
| 2 +-
SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAg
entLib.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git
a/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAg
entLib.c
b/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAg
entLib.c
index f7fbb3c..35fb5e6 100644
---
a/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAg
entLib.c
+++
b/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAg
entLib.c
@@ -512,7 +512,7 @@ InitializeDebugAgent (
Ia32Idtr = (IA32_DESCRIPTOR *) Context;
Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
MailboxLocation = (UINT64 *) (UINTN)
(Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
-
(Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
+ (UINT32)
(Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
Mailbox = (DEBUG_AGENT_MAILBOX *)(UINTN)(*MailboxLocation);
VerifyMailboxChecksum (Mailbox);
}
diff --git
a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDeb
ugAgentLib.c
b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDeb
ugAgentLib.c
index e77ff72..fcc7a4b 100644
---
a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDeb
ugAgentLib.c
+++
b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDeb
ugAgentLib.c
@@ -579,7 +579,7 @@ InitializeDebugAgent (
Ia32Idtr = (IA32_DESCRIPTOR *) Context;
Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
MailboxLocationPointer = (UINT64 *) (UINTN)
(Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
-
(Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
+ (UINT32)
(Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
Mailbox = (DEBUG_AGENT_MAILBOX *)
(UINTN)(*MailboxLocationPointer);
//
// Mailbox should valid and setup before executing thunk code
diff --git
a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebug
AgentLib.c
b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebug
AgentLib.c
index 6ac5f88..3a759f9 100644
---
a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebug
AgentLib.c
+++
b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebug
AgentLib.c
@@ -328,7 +328,7 @@ InitializeDebugAgent (
Ia32Idtr = (IA32_DESCRIPTOR *) Context;
Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
MailboxLocation = (UINT64 *) (UINTN)
(Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
-
(Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
+ (UINT32)
(Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
mMailboxPointer = (DEBUG_AGENT_MAILBOX
*)(UINTN)(*MailboxLocation);
VerifyMailboxChecksum (mMailboxPointer);
//
--
1.9.5.msysgit.0

[Scott Duplichan]

Jeff Fan [mailto:***@intel.com] wrote:

]Sent: Thursday, June 25, 2015 02:59 AM
]To: edk2-***@lists.sourceforge.net
]Subject: [edk2] [Patch] SourceLevelDebugPkg/DebugAgent: Add typecast to fix sign extension
]
]OffsetHigh is 16bit value and its type is UINT32 and defined in structure.
]If its high bit is 1, it will sign extension when do 16-bit left-shift
]operation. Need to typecast if after left-shift operation on GCC.

The patch is working but I think the description is not exactly right.
Shift related sign extension happens with right shift, not with left
shift. Removing the shift does not remove the gcc sign extension.
The problem is related to how the C language evaluates integers of size
smaller than int. The C99 spec says:

"If an int can represent all values of the original type, the value
is converted to an int; otherwise, it is converted to an unsigned int"

This means that a UINT16 is converted to int when evaluation an expression.
Both gcc and Microsoft compilers do this. I think the gcc/Microsoft
difference has to do with the bit fields:

struct {
UINT32 OffsetLow:16; ///< Offset bits 15..0.
. . .
UINT32 OffsetHigh:16; ///< Offset bits 31..16.
} Bits;

Gcc takes the view that "an int can represent all values of the original type"
for OffsetLow and OffsetHigh. Microsoft compilers treat these two fields as
not representable by signed int. Who is right? Well, the C99 spec is not real
clear about this particular case. However, C11 is:

"If an int can represent all values of the original type
(as restricted by the width, for a bit-field), the value
---------------------------------------------
is converted to an int; otherwise, it is converted to an
unsigned int"

So it looks like gcc is using the interpretation that has now
become standard.

Thanks,
Scott


]Contributed-under: TianoCore Contribution Agreement 1.0
]Signed-off-by: Jeff Fan <***@intel.com>
]CC: Ruiyu Ni <***@intel.com>
]---
] SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c | 2 +-
] .../Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c | 2 +-
] SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c | 2 +-
] 3 files changed, 3 insertions(+), 3 deletions(-)
]
]diff --git a/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c ]b/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c
]index f7fbb3c..35fb5e6 100644
]--- a/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c
]+++ b/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c
]@@ -512,7 +512,7 @@ InitializeDebugAgent (
] Ia32Idtr = (IA32_DESCRIPTOR *) Context;
] Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
] MailboxLocation = (UINT64 *) (UINTN) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
]- (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
]+ (UINT32) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
] Mailbox = (DEBUG_AGENT_MAILBOX *)(UINTN)(*MailboxLocation);
] VerifyMailboxChecksum (Mailbox);
] }
]diff --git a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c ]b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
]index e77ff72..fcc7a4b 100644
]--- a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
]+++ b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
]@@ -579,7 +579,7 @@ InitializeDebugAgent (
] Ia32Idtr = (IA32_DESCRIPTOR *) Context;
] Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
] MailboxLocationPointer = (UINT64 *) (UINTN) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
]- (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << ]16));
]+ (UINT32) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << ]16));
] Mailbox = (DEBUG_AGENT_MAILBOX *) (UINTN)(*MailboxLocationPointer);
] //
] // Mailbox should valid and setup before executing thunk code
]diff --git a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c ]b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
]index 6ac5f88..3a759f9 100644
]--- a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
]+++ b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
]@@ -328,7 +328,7 @@ InitializeDebugAgent (
] Ia32Idtr = (IA32_DESCRIPTOR *) Context;
] Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
] MailboxLocation = (UINT64 *) (UINTN) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
]- (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
]+ (UINT32) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
] mMailboxPointer = (DEBUG_AGENT_MAILBOX *)(UINTN)(*MailboxLocation);
] VerifyMailboxChecksum (mMailboxPointer);
] //
]--
]1.9.5.msysgit.0

[Fan, Jeff]

Scott,

Thanks your detailed explain and analysis.

Jeff

-----Original Message-----
From: Scott Duplichan [mailto:***@notabs.org]
Sent: Friday, June 26, 2015 12:05 AM
To: edk2-***@lists.sourceforge.net
Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/DebugAgent: Add typecast to fix sign extension

Jeff Fan [mailto:***@intel.com] wrote:

]Sent: Thursday, June 25, 2015 02:59 AM
]To: edk2-***@lists.sourceforge.net
]Subject: [edk2] [Patch] SourceLevelDebugPkg/DebugAgent: Add typecast to fix sign extension ] ]OffsetHigh is 16bit value and its type is UINT32 and defined in structure.
]If its high bit is 1, it will sign extension when do 16-bit left-shift ]operation. Need to typecast if after left-shift operation on GCC.

The patch is working but I think the description is not exactly right.
Shift related sign extension happens with right shift, not with left shift. Removing the shift does not remove the gcc sign extension.
The problem is related to how the C language evaluates integers of size smaller than int. The C99 spec says:

"If an int can represent all values of the original type, the value
is converted to an int; otherwise, it is converted to an unsigned int"

This means that a UINT16 is converted to int when evaluation an expression.
Both gcc and Microsoft compilers do this. I think the gcc/Microsoft difference has to do with the bit fields:

struct {
UINT32 OffsetLow:16; ///< Offset bits 15..0.
. . .
UINT32 OffsetHigh:16; ///< Offset bits 31..16.
} Bits;

Gcc takes the view that "an int can represent all values of the original type"
for OffsetLow and OffsetHigh. Microsoft compilers treat these two fields as not representable by signed int. Who is right? Well, the C99 spec is not real clear about this particular case. However, C11 is:

"If an int can represent all values of the original type
(as restricted by the width, for a bit-field), the value
---------------------------------------------
is converted to an int; otherwise, it is converted to an
unsigned int"

So it looks like gcc is using the interpretation that has now become standard.

Thanks,
Scott


]Contributed-under: TianoCore Contribution Agreement 1.0
]Signed-off-by: Jeff Fan <***@intel.com>
]CC: Ruiyu Ni <***@intel.com>
]---
] SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c | 2 +-
] .../Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c | 2 +-
] SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c | 2 +- ] 3 files changed, 3 insertions(+), 3 deletions(-) ] ]diff --git a/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c ]b/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c
]index f7fbb3c..35fb5e6 100644
]--- a/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c
]+++ b/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c
]@@ -512,7 +512,7 @@ InitializeDebugAgent (
] Ia32Idtr = (IA32_DESCRIPTOR *) Context;
] Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
] MailboxLocation = (UINT64 *) (UINTN) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
]- (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
]+ (UINT32) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
] Mailbox = (DEBUG_AGENT_MAILBOX *)(UINTN)(*MailboxLocation);
] VerifyMailboxChecksum (Mailbox);
] }
]diff --git a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c ]b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
]index e77ff72..fcc7a4b 100644
]--- a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
]+++ b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
]@@ -579,7 +579,7 @@ InitializeDebugAgent (
] Ia32Idtr = (IA32_DESCRIPTOR *) Context;
] Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
] MailboxLocationPointer = (UINT64 *) (UINTN) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
]- (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << ]16));
]+ (UINT32) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << ]16));
] Mailbox = (DEBUG_AGENT_MAILBOX *) (UINTN)(*MailboxLocationPointer);
] //
] // Mailbox should valid and setup before executing thunk code
]diff --git a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c ]b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
]index 6ac5f88..3a759f9 100644
]--- a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
]+++ b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
]@@ -328,7 +328,7 @@ InitializeDebugAgent (
] Ia32Idtr = (IA32_DESCRIPTOR *) Context;
] Ia32IdtEntry = (IA32_IDT_ENTRY *)(Ia32Idtr->Base);
] MailboxLocation = (UINT64 *) (UINTN) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetLow +
]- (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
]+ (UINT32) (Ia32IdtEntry[DEBUG_MAILBOX_VECTOR].Bits.OffsetHigh << 16));
] mMailboxPointer = (DEBUG_AGENT_MAILBOX *)(UINTN)(*MailboxLocation);
] VerifyMailboxChecksum (mMailboxPointer);
] //
]--
]1.9.5.msysgit.0



------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors
network devices and physical & virtual servers, alerts via email & sms
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
_______________________________________________
edk2-devel mailing list
edk2-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel