PREfast annotations - IRQL, resources

Maria_Hamilton · December 10, 2010, 12:38pm

So I’m working through the “PREfast Annotations” document and writing notes for our team.
The IRQL stuff looks handy and we have a few lightweight smart classes to wrap fast mutex acquisition, I’m sure a lot of people do. Gosh I think perhaps it would be good to annotate these as an experiment…

… fifteen minutes later I’m Googling and find nothing but other programmers going, HOW DO I GET THIS TO WORK? doing really very much the same thing on the same code…

… e.g. here http://www.osronline.com/showThread.cfm?link=122049

that was 2007 - anyone made a breakthrough since then?

That’s it - happy shopping, people!

Pavel_A1 · December 10, 2010, 7:38pm

wrote in message news:xxxxx@ntdev…
> So I’m working through the “PREfast Annotations” document and writing
> notes for our team.
> The IRQL stuff looks handy and we have a few lightweight smart classes to
> wrap fast mutex acquisition, I’m sure a lot of people do. Gosh I think
> perhaps it would be good to annotate these as an experiment…
>
> … fifteen minutes later I’m Googling and find nothing but other
> programmers going, HOW DO I GET THIS TO WORK? doing really very much the
> same thing on the same code…
>
> … e.g. here http://www.osronline.com/showThread.cfm?link=122049
> - that was 2007 - anyone made a breakthrough since then?

AFAIK these prefast annotations are not completely documented and
only people with microsoft.com in their email address may be concerned
about doing something useful with them.

By the way the code snippet in that post is questionable. Consider:

void Lock() { KIRQL x; KeAcquireSpinLock( &lock, x ); oldIrql = x; }

- pa

Jeffrey_Tippet_MSFT · December 10, 2010, 9:30pm

The general trick to wrapping an API is to use the same annotations as it uses. Except that if you bundle its parameters into your own structure, you’ll need to use something like __drv_at() to pick them out again.

Here’s an example with KSPIN_LOCK.

C:\WdkPrefast>type test.c
#include <wdm.h>

// Sample functions that we call
__drv_requiresIRQL(PASSIVE_LEVEL)
void DoSomethingAtPassive();

__drv_requiresIRQL(DISPATCH_LEVEL)
void DoSomethingAtDispatch();

__drv_mustHold(KeSpinLockType)
void DoSomethingUnderLock();

BOOLEAN AllocateSomething();

// Our own custom lock structure
typedef struct _MYLOCK
{
KSPIN_LOCK Lock;
KIRQL OldIrql;

} MYLOCK, *PMYLOCK;

// Here’s our simple lock API:
// LockIt
// UnlockIt
// (Other functions, like initialization, are not relevant here)

__drv_maxIRQL(DISPATCH_LEVEL)
__drv_setsIRQL(DISPATCH_LEVEL)
__drv_at(Lock->Lock, __drv_acquiresExclusiveResource(KeSpinLockType))
__drv_at(Lock->OldIrql, __drv_savesIRQL)
void LockIt(
__inout PMYLOCK Lock
)
{
KeAcquireSpinLock(&Lock->Lock, &Lock->OldIrql);
}

__drv_requiresIRQL(DISPATCH_LEVEL)
__drv_at(Lock->Lock, __drv_releasesExclusiveResource(KeSpinLockType))
__drv_at(Lock->OldIrql, __drv_restoresIRQL)
void UnlockIt(
__inout PMYLOCK Lock
)
{
KeReleaseSpinLock(&Lock->Lock, Lock->OldIrql);
}

// Now some test cases.
// First test case ensures that PREFast is happy when we write good code.
__drv_requiresIRQL(PASSIVE_LEVEL)
void UseLockGood(
__in PMYLOCK Lock1,
__in PMYLOCK Lock2
)
{
DoSomethingAtPassive();

{ LockIt(Lock1);

DoSomethingAtDispatch();
DoSomethingUnderLock();

{ LockIt(Lock2);
DoSomethingAtDispatch();
} UnlockIt(Lock2);

DoSomethingAtDispatch();

} UnlockIt(Lock1);

DoSomethingAtPassive();
}

// Test the IRQL-checking (acquiring a lock raises us to DISPATCH).
__drv_requiresIRQL(PASSIVE_LEVEL)
void UseLockBadIrql(
__in PMYLOCK Lock
)
{
{ LockIt(Lock);

// Bug! IRQL is now DISPATCH_LEVEL
DoSomethingAtPassive();

} UnlockIt(Lock);

// Bug! IRQL is now PASSIVE_LEVEL
DoSomethingAtDispatch();
}

// Test that PREFast enforces a balanced acquire-release pattern
__drv_requiresIRQL(PASSIVE_LEVEL)
void UseLockForgetToUnlock(
__in PMYLOCK Lock
)
{
{ LockIt(Lock);

if (!AllocateSomething())
{
// Bug! Forgot to release lock
return;
}

} UnlockIt(Lock);
}

// Test that PREFast enforces we hold the lock
__drv_requiresIRQL(PASSIVE_LEVEL)
void UseLockForgetToHoldLock()
{
// Bug! Not holding any lock while making this call
DoSomethingUnderLock();
}

// Test some other silly errors
__drv_requiresIRQL(PASSIVE_LEVEL)
void UseLockMiscErrors(
__in PMYLOCK Lock
)
{
{ LockIt(Lock);

// Bug! Locked the lock twice
{ LockIt(Lock);

DoSomethingAtDispatch();

} UnlockIt(Lock);

// Bug! Unlocked a lock that’s not held
} UnlockIt(Lock);

// Bug! Used the API wrongly
LockIt(NULL);
}

Phew. Now to prove that this does what we want:

C:\WdkPrefast>oacr clean all && build -cZP && oacr check all && oacr list all

Warning list
------------

Projects :

c:\users\jtippet\desktop\cpp\wdkprefast\test.c (87): OACR warning 28156: The actual IRQL 2 is inconsistent with the required IRQL 0: The value at exit was not set to the expected value.
FUNCTION: UseLockBadIrql (80)
PATH: 84 87

c:\users\jtippet\desktop\cpp\wdkprefast\test.c (92): OACR warning 28156: The actual IRQL 0 is inconsistent with the required IRQL 2: The value at exit was not set to the expected value.
FUNCTION: UseLockBadIrql (80)
PATH: 84 87 89 92

c:\users\jtippet\desktop\cpp\wdkprefast\test.c (97): OACR warning 28167: The function ‘UseLockForgetToUnlock’ changes the IRQL and does not restore the IRQL before it exits. It should be annotated to reflect the change or the IRQL should be restored. IRQL was last set to 2 at line 101.
FUNCTION: UseLockForgetToUnlock (97)
PATH: 101 103 106

c:\users\jtippet\desktop\cpp\wdkprefast\test.c (117): OACR warning 28191: At least one resource of the class ‘KeSpinLockType’ must be held when this function is called.
FUNCTION: UseLockForgetToHoldLock (114)
PATH: 117

c:\users\jtippet\desktop\cpp\wdkprefast\test.c (122): OACR warning 28167: The function ‘UseLockMiscErrors’ changes the IRQL and does not restore the IRQL before it exits. It should be annotated to reflect the change or the IRQL should be restored. IRQL was last set to 2 at line 139.
FUNCTION: UseLockMiscErrors (122)
PATH: 126 129 131 133 136 139

c:\users\jtippet\desktop\cpp\wdkprefast\test.c (129): OACR warning 28109: The KeSpinLockType ‘argument 1->Lock’ cannot be held at the time ‘LockIt’ is called.
FUNCTION: UseLockMiscErrors (122)
PATH: 126 129

c:\users\jtippet\desktop\cpp\wdkprefast\test.c (136): OACR warning 28156: The actual IRQL 0 is inconsistent with the required IRQL 2: The value at exit was not set to the expected value.
FUNCTION: UseLockMiscErrors (122)
PATH: 126 129 131 133 136

c:\users\jtippet\desktop\cpp\wdkprefast\test.c (139): OACR warning 28103: Leaking the KeSpinLockType stored in ‘argument 1->Lock’.
FUNCTION: UseLockMiscErrors (122)
PATH: 126 129 131 133 136 139

Each of the //Bug! comments gets detected by PREFast. There are no false positives.

For the sake of folks trying to reproduce this at home:
C:\WdkPrefast>type sources
TARGETNAME=prefasttest
TARGETTYPE=DRIVER_LIBRARY

MSC_WARNING_LEVEL=/WX /W4

SOURCES=<br> test.c</all></wdm.h>

Peter_Viscarola_OSR · December 11, 2010, 11:12am

Thank you, Mr. Tippet, for providing that exceptionally clear example. Might you consider passing it along to the WDK Team for inclusion in the WDK? I think that’d be helpful to a lot of people.

Having said that, I’m soooo conflicted over these annotations. They’re annoying to provide (after all, it’s a bit like writing your code twice) and they clutter the code with ugly goop that is essentially unreadable. OTOH, I agree that they could definitely contribute to code quality.

I sooo wish some of these checks transformed into run-time validations. In my opinion, there’s waaaay (make that way, way, way, way, way) too much emphasis on static analysis (which has significant inherent limitations) at the expense of run-time validation. Balance, my friends, is a good thing.

Peter
OSR

mm1 · December 11, 2010, 1:15pm

+1

mm

-----Original Message-----
From: xxxxx@lists.osr.com
[mailto:xxxxx@lists.osr.com] On Behalf Of xxxxx@osr.com
Sent: Saturday, December 11, 2010 11:12 AM
To: Windows System Software Devs Interest List
Subject: RE:[ntdev] PREfast annotations - IRQL, resources

Thank you, Mr. Tippet, for providing that exceptionally clear example.
Might you consider passing it along to the WDK Team for inclusion in the
WDK? I think that’d be helpful to a lot of people.

Having said that, I’m soooo conflicted over these annotations. They’re
annoying to provide (after all, it’s a bit like writing your code twice) and
they clutter the code with ugly goop that is essentially unreadable. OTOH,
I agree that they could definitely contribute to code quality.

I sooo wish some of these checks transformed into run-time validations. In
my opinion, there’s waaaay (make that way, way, way, way, way) too much
emphasis on static analysis (which has significant inherent limitations) at
the expense of run-time validation. Balance, my friends, is a good thing.

Peter
OSR

NTDEV is sponsored by OSR

For our schedule of WDF, WDM, debugging and other seminars visit:
http://www.osr.com/seminars

To unsubscribe, visit the List Server section of OSR Online at
http://www.osronline.com/page.cfm?name=ListServer

David_R_Cattley · December 12, 2010, 8:35am

They’re annoying to provide (after all, it’s a bit like writing your code
twice) and they clutter the code with ugly goop that is essentially
unreadable. OTOH, I agree that they could definitely contribute to code
quality.

They are, in fact, somewhat of a self-inflicted code review. Just writing
them make the code better in that you have to think very carefully about it
all. I would be very jazzed by a simple extension to the Visual Studio
syntax handling that basically had an View option to “hide annotations” /
“show annotations”. Straight ‘text’ based source-code files have become
rather overloaded [aka, sewer] trying to hold and convey the information
needed by authors, maintainers, reviewers, repositories, analysis tools, and
compilers to create, maintain, validate, manage and translate the ‘source’.
That camel’s back is under a lot of stress.

I sooo wish some of these checks transformed into run-time validations.

You mean akin to CLR annotations generating whole swaths of code (or hooks
into code, etc.)? That would be very powerful especially if it was
extensible to allow *me* to model *my* problem’s semantic verification at
the level of *my* abstraction and not just the OS’s idea of what is
interesting. But that would take the explicit cooperation of the Compiler
Dudes. Line that request up just behind guaranteed predictable C++
emission behavior (oh sh_t, now I have done it) and set expectations
accordingly.

Cheers,
Dave Cattley

Pavel_A1 · December 12, 2010, 8:46am

Should we (ISVs) use these annotations in same manner as seen in the WDK headers?

IMHO WDK headers must be annotated because we (and neither prefast & sdv?) do not see the implementation of the DDIs, so all we have for the DDIs is annotations.

But we do have our own code. Can we just provide simple
“role-based” annotations on key functions (like DRIVER_ENTRY,DRIVER_ADD_DEVICE, EVT_WDF_DRIVER_DEVICE_ADD ) and expect that the analyser will catch up from there?
Because, it already “knows” what is KeAcquireSpinLock, KeReleaseSpinLock etc ?

About __drv_mustHold(KeSpinLockType): does this mean that the function should run under *any* spinlock, or some specific one? The latter is useful, the former - hardly.

Regards,
–pa

Don_Burn_1 · December 12, 2010, 9:14am

Well annotations allow PREfast to work better, so annotating our code
means that it should do a better job. The flip side of this is the
annotations have become so complex that all you may really be doing is
debugging the annotations not finding code bugs. Personally, I use a
conservative annotation approach, definitely annotating the arguments
and big things like checking return and IRQL. I don’t go overboard with
the rest.

I do cast my vote to Peter’s suggestion of providing runtime checks for
the annotations, this would probably encourage me to do more annotating
and it definitely would be a useful debugging tool.

I also support Dave Cattley’s suggestion of making Visual Studio have a
way of hiding the annotations.

Don Burn (MVP, Windows DKD)
Windows Filesystem and Driver Consulting
Website: http://www.windrvr.com
Blog: http://msmvps.com/blogs/WinDrvr

“xxxxx@fastmail.fm” wrote in message
news:xxxxx@ntdev:

> Should we (ISVs) use these annotations in same manner as seen in the WDK headers?
>
> IMHO WDK headers must be annotated because we (and neither prefast & sdv?) do not see the implementation of the DDIs, so all we have for the DDIs is annotations.
>
> But we do have our own code. Can we just provide simple
> “role-based” annotations on key functions (like DRIVER_ENTRY,DRIVER_ADD_DEVICE, EVT_WDF_DRIVER_DEVICE_ADD ) and expect that the analyser will catch up from there?
> Because, it already “knows” what is KeAcquireSpinLock, KeReleaseSpinLock etc ?
>
> About __drv_mustHold(KeSpinLockType): does this mean that the function should run under any spinlock, or some specific one? The latter is useful, the former - hardly.
>
> Regards,
> --pa

Pavel_A1 · December 12, 2010, 9:28am

> I also support Dave Cattley’s suggestion of making Visual Studio have a
way of hiding the annotations.

+1

Can this be simply defined as: hide all function and argument attributes that begin with __ ?
That will hide also __in, __out, and some usual __declspec’s though; a whitelist may be needed.

Regards,
– pa

mm1 · December 12, 2010, 10:18am

>They are, in fact, somewhat of a self-inflicted code review. Just writing
them make the code better in that you have to think very carefully about it
all.

True, though I would argue that at present, a little too much though is
required to get things working not uncommonly and sometimes then process is
just maddening. Also, maintaining code that someone else who wasn’t
interested in spending too much time thinking about this wrote is really,
really awful - unreadable AND not annotated correctly/usefully AND you
likely won’t know that unless you read through said unreadable code.

I’ve barely sort of kind of gotten this (hide annotations) to work in
SlickEdit through brute force by creating a copy of each of the 42 or so
different files that define annotations (prefast, SECURE_CRT, deprecated
functions, et. c.) and then creating a compiler tag set from that. It still
doesn’t work correctly, I think because it is in general
very difficult to convince slickedit that some of these constructs aren’t
functions. This I think is the same problem that affects Doxygen (which
came up in a thread a few weeks ago) and most any other source tool that
I’ve tried, since they can’t use the compiler to sort this stuff out.

‘scanf:’

MSDN:

int scanf(const char *format [, argument]… );
int _scanf_l(const char *format, locale_t locale [,argument]…);
int wscanf(const wchar_t *format [,argument]…);
int _wscanf_l(const wchar_t *format,locale_t locale [,argument]… );

$ grep -E ‘scanf’ stdio.h
Check_return _CRT_INSECURE_DEPRECATE(fscanf_s) _CRTIMP int __cdecl
fscanf(Inout FILE * _File, In_z Scanf_format_string const char *
_Format, …);
Check_return_opt _CRT_INSECURE_DEPRECATE(_fscanf_s_l) _CRTIMP int __cdecl
_fscanf_l(Inout FILE * _File, In_z Scanf_format_string const char *
_Form
at, In_opt _locale_t _Locale, …);
Check_return_opt _CRTIMP int __cdecl fscanf_s(Inout FILE * _File, In_z
Scanf_s_format_string const char * _Format, …);
Check_return_opt _CRTIMP int __cdecl _fscanf_s_l(Inout FILE * _File,
In_z Scanf_s_format_string const char * _Format, In_opt _locale_t
_Locale, .
…);
Check_return _CRT_INSECURE_DEPRECATE(scanf_s) _CRTIMP int __cdecl
scanf(In_z Scanf_format_string const char * _Format, …);
Check_return_opt _CRT_INSECURE_DEPRECATE(_scanf_s_l) _CRTIMP int __cdecl
_scanf_l(In_z Scanf_format_string const char * _Format, In_opt
_locale_t _
Locale, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl scanf_s(In_z
Scanf_s_format_string const char * _Format, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl _scanf_s_l(In_z
Scanf_s_format_string const char * _Format, In_opt _locale_t _Locale,
…);
Check_return _CRT_INSECURE_DEPRECATE(sscanf_s) _CRTIMP int __cdecl
sscanf(In_z const char * _Src, In_z Scanf_format_string const char *
_Format, …
.);
Check_return_opt _CRT_INSECURE_DEPRECATE(_sscanf_s_l) _CRTIMP int __cdecl
_sscanf_l(In_z const char * _Src, In_z Scanf_format_string const char
* _
Format, In_opt _locale_t _Locale, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl sscanf_s(In_z const
char * _Src, In_z Scanf_s_format_string const char * _Format, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl _sscanf_s_l(In_z const
char * _Src, In_z Scanf_s_format_string const char * _Format, In_opt
_loc
ale_t _Locale, …);
Check_return_opt _CRT_INSECURE_DEPRECATE(_snscanf_s) _CRTIMP int __cdecl
_snscanf(In_bytecount(_MaxCount) Pre_z const char * _Src, In size_t
_MaxCo
unt, In_z Scanf_format_string const char * _Format, …);
Check_return_opt _CRT_INSECURE_DEPRECATE(_snscanf_s_l) _CRTIMP int __cdecl
_snscanf_l(In_bytecount(_MaxCount) Pre_z const char * _Src, In size_t
_M
axCount, In_z Scanf_format_string const char * _Format, In_opt
_locale_t _Locale, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl
_snscanf_s(In_bytecount(_MaxCount) Pre_z const char * _Src, In size_t
_MaxCount, In_z Scanf_s_f
ormat_string const char * _Format, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl
_snscanf_s_l(In_bytecount(_MaxCount) Pre_z const char * _Src, In
size_t _MaxCount, In_z _Scanf_s
format_string const char * _Format, In_opt _locale_t _Locale, …);
Check_return _CRT_INSECURE_DEPRECATE(fwscanf_s) _CRTIMP int __cdecl
fwscanf(Inout FILE * _File, In_z Scanf_format_string const wchar_t *
_Format, .
…);
Check_return_opt _CRT_INSECURE_DEPRECATE(_fwscanf_s_l) _CRTIMP int __cdecl
_fwscanf_l(Inout FILE * _File, In_z Scanf_format_string const wchar_t
*
_Format, In_opt _locale_t _Locale, …);
Check_return_opt _CRTIMP int __cdecl fwscanf_s(Inout FILE * _File,
In_z Scanf_s_format_string const wchar_t * _Format, …);
Check_return_opt _CRTIMP int __cdecl _fwscanf_s_l(Inout FILE * _File,
In_z Scanf_s_format_string const wchar_t * _Format, In_opt _locale_t
_Local
e, …);
Check_return _CRT_INSECURE_DEPRECATE(swscanf_s) _CRTIMP int __cdecl
swscanf(In_z const wchar_t * _Src, In_z Scanf_format_string const
wchar_t * _Fo
rmat, …);
Check_return_opt _CRT_INSECURE_DEPRECATE(_swscanf_s_l) _CRTIMP int __cdecl
_swscanf_l(In_z const wchar_t * _Src, In_z Scanf_format_string const
wch
ar_t * _Format, In_opt _locale_t _Locale, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl swscanf_s(In_z const
wchar_t *_Src, In_z Scanf_s_format_string const wchar_t * _Format, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl _swscanf_s_l(In_z const
wchar_t * _Src, In_z Scanf_s_format_string const wchar_t * _Format,
In_op
t _locale_t _Locale, …);
Check_return_opt _CRT_INSECURE_DEPRECATE(_snwscanf_s) _CRTIMP int __cdecl
_snwscanf(In_count(_MaxCount) Pre_z const wchar_t * _Src, In size_t
_MaxC
ount, In_z Scanf_format_string const wchar_t * _Format, …);
Check_return_opt _CRT_INSECURE_DEPRECATE(_snwscanf_s_l) _CRTIMP int
__cdecl _snwscanf_l(In_count(_MaxCount) Pre_z const wchar_t * _Src, In
size_t _
MaxCount, In_z Scanf_format_string const wchar_t * _Format, In_opt
_locale_t _Locale, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl
_snwscanf_s(In_count(_MaxCount) Pre_z const wchar_t * _Src, In size_t
_MaxCount, In_z Scanf_s_f
ormat_string const wchar_t * _Format, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl
_snwscanf_s_l(In_count(_MaxCount) Pre_z const wchar_t * _Src, In
size_t _MaxCount, In_z _Scanf_s
format_string const wchar_t * _Format, In_opt _locale_t _Locale, …);
Check_return _CRT_INSECURE_DEPRECATE(wscanf_s) _CRTIMP int __cdecl
wscanf(In_z Scanf_format_string const wchar_t * _Format, …);
Check_return_opt _CRT_INSECURE_DEPRECATE(_wscanf_s_l) _CRTIMP int __cdecl
_wscanf_l(In_z Scanf_format_string const wchar_t * _Format, In_opt
_local
e_t _Locale, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl wscanf_s(In_z
Scanf_s_format_string const wchar_t * _Format, …);
Check_return_opt _CRTIMP_ALTERNATIVE int __cdecl _wscanf_s_l(In_z
Scanf_s_format_string const wchar_t * _Format, In_opt _locale_t _Locale,
…);

mm

-----Original Message-----
From: xxxxx@lists.osr.com
[mailto:xxxxx@lists.osr.com] On Behalf Of David R. Cattley
Sent: Sunday, December 12, 2010 8:35 AM
To: Windows System Software Devs Interest List
Subject: RE: [ntdev] PREfast annotations - IRQL, resources

They’re annoying to provide (after all, it’s a bit like writing your code
twice) and they clutter the code with ugly goop that is essentially
unreadable. OTOH, I agree that they could definitely contribute to code
quality.

They are, in fact, somewhat of a self-inflicted code review. Just writing
them make the code better in that you have to think very carefully about it
all. I would be very jazzed by a simple extension to the Visual Studio
syntax handling that basically had an View option to “hide annotations” /
“show annotations”. Straight ‘text’ based source-code files have become
rather overloaded [aka, sewer] trying to hold and convey the information
needed by authors, maintainers, reviewers, repositories, analysis tools, and
compilers to create, maintain, validate, manage and translate the ‘source’.
That camel’s back is under a lot of stress.

I sooo wish some of these checks transformed into run-time validations.

You mean akin to CLR annotations generating whole swaths of code (or hooks
into code, etc.)? That would be very powerful especially if it was
extensible to allow *me* to model *my* problem’s semantic verification at
the level of *my* abstraction and not just the OS’s idea of what is
interesting. But that would take the explicit cooperation of the Compiler
Dudes. Line that request up just behind guaranteed predictable C++
emission behavior (oh sh_t, now I have done it) and set expectations
accordingly.

Cheers,
Dave Cattley

NTDEV is sponsored by OSR

For our schedule of WDF, WDM, debugging and other seminars visit:
http://www.osr.com/seminars

To unsubscribe, visit the List Server section of OSR Online at
http://www.osronline.com/page.cfm?name=ListServer

Maxim_S_Shatskih · December 13, 2010, 7:40am

> Having said that, I’m soooo conflicted over these annotations. They’re annoying to provide (after all,

it’s a bit like writing your code twice) and they clutter the code with ugly goop that is essentially
unreadable.

Yes.

That’s why I only annotate all kinds of MyLock/MyUnlock functions, and nothing else.

–
Maxim S. Shatskih
Windows DDK MVP
xxxxx@storagecraft.com
http://www.storagecraft.com

Maria_Hamilton · December 13, 2010, 11:27am

Well thanks everyone. That should help us move a bit further up the learning curve.

OSR_Community_User · December 13, 2010, 4:43pm

Part of the problem is that code examination only tells you what the current
code is doing, not what your intention is. It’s a bit like the ‘const’
declaration in ordinary C/C++; while this week, you might not be modifying
something, by saying ‘const’ you block, for all time, the ability of the
programmer to write code that could (accidentally) modify the behavior. It
is a lot harder for PREFast to deduce your intention from behavior than by
explicit declaration.

I go for +1 on runtime enforcement (when feasible), although in some cases
you could easily do something like an ASSERT_IRQL(PASSIVE_LEVEL) to do this
(although it merely duplicates what is in the annotation), but it would have
to throw an exception or bugcheck if the constraint was violated.

Since I don’t use the toy editor that comes with VS, what the editor does is
pretty irrelevant.
joe

-----Original Message-----
From: xxxxx@lists.osr.com
[mailto:xxxxx@lists.osr.com] On Behalf Of Don Burn
Sent: Sunday, December 12, 2010 9:14 AM
To: Windows System Software Devs Interest List
Subject: RE:[ntdev] PREfast annotations - IRQL, resources

Well annotations allow PREfast to work better, so annotating our code means
that it should do a better job. The flip side of this is the annotations
have become so complex that all you may really be doing is debugging the
annotations not finding code bugs. Personally, I use a conservative
annotation approach, definitely annotating the arguments and big things like
checking return and IRQL. I don’t go overboard with the rest.

I do cast my vote to Peter’s suggestion of providing runtime checks for the
annotations, this would probably encourage me to do more annotating and it
definitely would be a useful debugging tool.

I also support Dave Cattley’s suggestion of making Visual Studio have a way
of hiding the annotations.

Don Burn (MVP, Windows DKD)
Windows Filesystem and Driver Consulting
Website: http://www.windrvr.com
Blog: http://msmvps.com/blogs/WinDrvr

“xxxxx@fastmail.fm” wrote in message
news:xxxxx@ntdev:

> Should we (ISVs) use these annotations in same manner as seen in the WDK
headers?
>
> IMHO WDK headers must be annotated because we (and neither prefast & sdv?)
do not see the implementation of the DDIs, so all we have for the DDIs is
annotations.
>
> But we do have our own code. Can we just provide simple
> “role-based” annotations on key functions (like
DRIVER_ENTRY,DRIVER_ADD_DEVICE, EVT_WDF_DRIVER_DEVICE_ADD ) and expect that
the analyser will catch up from there?
> Because, it already “knows” what is KeAcquireSpinLock, KeReleaseSpinLock
etc ?
>
> About __drv_mustHold(KeSpinLockType): does this mean that the function
should run under any spinlock, or some specific one? The latter is useful,
the former - hardly.
>
> Regards,
> --pa

—
NTDEV is sponsored by OSR

For our schedule of WDF, WDM, debugging and other seminars visit:
http://www.osr.com/seminars

To unsubscribe, visit the List Server section of OSR Online at
http://www.osronline.com/page.cfm?name=ListServer

–
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Maria_Hamilton · December 14, 2010, 5:56am

But did you guys notice the way the example was C and not a C++ wrapper class?
Still trying to get my head round how you tell PREfast that something is being saved in a member variable of the class and enjoying watching the PREfast compiler crash.

Maria_Hamilton · December 14, 2010, 6:48am

Anyone up to explaining why
__drv_at(this, __drv_savesIRQL) is accepted by PREfast fine but
__drv_at(this, __drv_restoresIRQL) gets a 28266 error, “was not applied to an argument or the return”
?
Do I need another coffee? At the moment it seems odd that you can’t use them in the same way…

Pavel_A1 · December 14, 2010, 8:35am

> But did you guys notice the way the example was C and not a C++ wrapper class?

Can you please point to any place in WDK documentation or papers posted on WHDC where it says that C++ is supported by these tools?

/* YES PEOPLE USE C++ in drivers. Me too. Let’s not start a war */

–pa

Maria_Hamilton · December 14, 2010, 9:00am

http://msdn.microsoft.com/en-us/library/ff542168(VS.85).aspx
“__drv_at(expression, anno_list)
Indicates that anno_list is to be applied to the object named by expression, which must yield an l-Value. This form is often used with this to annotate the C++ this object or members of that class, but any l-value can be used. It is often used on the function body to annotate complex objects with respect to a parameter.”

Maria_Hamilton · December 14, 2010, 10:01am

Answers own question: always remember to check the version of the WDK that’s being picked up… particularly if you see something that looks like a bug **which might have been fixed in the latest version**

Anyone up to explaining why
__drv_at(this, __drv_savesIRQL) is accepted by PREfast fine but
__drv_at(this, __drv_restoresIRQL) gets a 28266 error, “was not applied to an
argument or the return”
?
Do I need another coffee? At the moment it seems odd that you can’t use them in
the same way…

Maria_Hamilton · December 14, 2010, 11:31am

Though I do have questions on the extent to which PREfast annotations do support C++.
Let me produce a code snippet - annotated with the warnings I get - so perhaps someone can comment (it’s easy to miss things when you’re still learning).

(If anyone is interested in what success I had annotating the fast mutex wrapper class just ask by the way).

// Here we go, tiny class to acquire & release spin lock

class CWrapSpinLock
{
KLOCK_QUEUE_HANDLE m_hLockQ;
public:
__drv_maxIRQL(DISPATCH_LEVEL)
__drv_savesIRQLGlobal(QueuedSpinLock, this->m_hLockQ)
__drv_setsIRQL(DISPATCH_LEVEL)
__drv_at(this->m_hLockQ, __inout __drv_acquiresExclusiveResource(KeQueuedSpinLockType))
CWrapSpinLock(__inout KSPIN_LOCK& spinLock)
{
KeAcquireInStackQueuedSpinLock(&spinLock, &m_hLockQ);
}

__drv_requiresIRQL(DISPATCH_LEVEL)
__drv_restoresIRQLGlobal(QueuedSpinLock, this->m_hLockQ)
__drv_at(this->m_hLockQ, __inout __drv_releasesExclusiveResource(KeQueuedSpinLockType))
~CWrapSpinLock()
{
KeReleaseInStackQueuedSpinLock(&m_hLockQ);
}
};

void mytest() // warning 28167: The function ‘mytest’ changes the IRQL and
// does not restore the IRQL before it exits.
// It should be annotated to reflect the change or the IRQL should be restored.
// IRQL was last set at line -1.
{
KSPIN_LOCK mQueueLock; // Locks push and pop operations on the queue
KeInitializeSpinLock( &mQueueLock );
CWrapSpinLock locked(mQueueLock); // warning 28103: Leaking the KeQueuedSpinLockType stored in ‘this->m_hLockQ’.

}

Now iif I replace the CWrapSpinLock locked… line with the declaration of the one class member variable followed by the constructor code followed by the destructor code (if you get my drift) - no warnings. So what gives?

Doron_Holan · December 14, 2010, 2:00pm

note that CWrapSpinLock can *only* be stack based. the KLOCK_QUEUE_HANDLE * that is passed to each caller of KeAcquireInStackQueuedSpinLock must be a unique pointer value because when the lock is not free and you are waiting, the queue handle is placed into a linked list of waiters. if you pass the same pointer in from multiple callers, you will corrupt that list.

d