Bug 6514

Created attachment 8042 [details]
ASL code to reproduce bug


ACPICA AML interpreter (acpiexec) should be ran with -s flag (slack mode).

Output of the demo:

Executing \MAIN
[ACPI Debug]  Integer: 0x00000000000000F1
Outstanding: 0x1 allocations after execution
Execution of \MAIN returned object 00326E38 Buflen 10
  [Integer] = 00000000000000F1

DESCRIPTION: 

In ACPICA AML interpreter Slack mode outstanding allocations
are detected when AML-code looks like the following lines:

Method (SBYT, 1)
{
    Store (Arg0, Debug)
}

Method(MAIN)
{
    Or (0xf0, 0x01, Local0)
    SBYT (Local0)
}

The problem was initially held up by FreeBSD:
> -----Original Message-----
> From: Nate Lawson [mailto:nate@root.org]
> Sent: Monday, February 13, 2006 2:16 PM
> To: Jung-uk Kim
> Cc: Moore, Robert
> Subject: Re: ACPI-CA memory leak
> 
> Judging from your patch, my thought is that the store from Local1 to
> DATS is not being reference-counted properly and so the local object is
> never freed.

The above proposed example reproduces a very probable way of that case.

Both optional storing to Local0 and manipulation with it in a called
Method (SBYT) are essential here.

Note: there is no any outstanding allocations when
      Store (Or (0xf0, 0x01), Local0) is used instead
      of Or(0xf0, 0x01, Local0).

Looks like the following code in the function AcpiDsDoImplicitReturn
(file interpreter/dispatcher/dsutils.c) takes effect in our case:

    /*
     * Delete any "stale" implicit return value first. However, in
     * complex statements, the implicit return value can be
     * bubbled up several levels, so we don't clear the value if it
     * is the same as the ReturnDesc.
     */
    if (WalkState->ImplicitReturnObj)
    {
        if (WalkState->ImplicitReturnObj == ReturnDesc)
        {
            return (TRUE);
        }
        AcpiDsClearImplicitReturn (WalkState);
    }

The RefCount of the Local0 object (from the example) incremented for
MAIN--WalkState->ImplicitReturnObj was additionally incremented for
the terminated method SBYT--WalkState->ImplicitReturnObj (ReturnDesc).
So, after returning from the called method (SBYT) to the current method
(MAIN) the previous MAIN--WalkState->ImplicitReturnObj should explicitly
be cleared through the call to AcpiDsClearImplicitReturn.

The following patch can solve this problem:

--- dsutils.c.orig	2006-03-28 12:48:09.146714200 +0400
+++ dsutils.c	2006-03-28 12:48:37.087169200 +0400
@@ -228,8 +228,4 @@
     if (WalkState->ImplicitReturnObj)
     {
-        if (WalkState->ImplicitReturnObj == ReturnDesc)
-        {
-            return (TRUE);
-        }
         AcpiDsClearImplicitReturn (WalkState);
     }

Although ASL tests are not specifically designed to cover Slack mode issues,
this problem evidently is touched by them. There are different numbers of
outstanding allocations on Slack and non-Slack mode. After this patch some
Slack numbers became less than before.

However there is no certainty now that the proposed update is the exact
solution of the problem. This bug should be additionally investigated along
with other events of outstanding allocations.

INTERNAL BUG NUMBER:

   211

Additional consideration.

The same result (outstanding allocation) takes place if
explicit Return operator will be used in SBYT method above:

Method (SBYT, 1)
{
-    Store (Arg0, Debug)
+    Return (Arg0)
}

Evidently the RefCount of the original Local0 object (passed
into SBYT) incremented for SBYT--WalkState->ReturnDesc gives
the same negative effect.

The mentioned above fragment of AcpiDsDoImplicitReturn resolves
Implicit Return processing issue when the same Object could become
the Result Object of several successive AML operators ("bubbled up
several levels"). In that case would be superfluous decrementing Object's
RefCount (by means AcpiDsClearImplicitReturn) to increment it then:

    /* Save the implicit return value, add a reference if requested */

    WalkState->ImplicitReturnObj = ReturnDesc;
    if (AddReference)
    {
        AcpiUtAddReference (ReturnDesc);
    }

The AddReference parameter of AcpiDsDoImplicitReturn is FALSE
above when this function is invoked after an AML method return so
that RefCount of the previous Implicit Return Object is decremented
and can become Zero (the Object will be deleted if any) but RefCount
of the new Implicit Return Object will not be incremented. This is
right for a called method Return Object in the most cases but does
not work when it is the same as the previous Implicit Return Object.

That additional issue could be fixed by the following patch:

diff -r -u source3/components/interpreter/dispatcher/dsutils.c 
source/components/interpreter/dispatcher/dsutils.c
--- source3/components/interpreter/dispatcher/dsutils.c	2006-04-24 
12:25:15.000000000 +0400
+++ source/components/interpreter/dispatcher/dsutils.c	2006-05-06 
11:00:06.979158800 +0400
@@ -228,6 +228,10 @@
     {
         if (WalkState->ImplicitReturnObj == ReturnDesc)
         {
+            if (!AddReference)
+            { /* Delete additional reference corresponding to ReturnDesc */
+                AcpiUtRemoveReference (ReturnDesc);
+            }
             return (TRUE);
         }
         AcpiDsClearImplicitReturn (WalkState);

Created attachment 8043 [details]
Proposed patch

The update was tested by the ASLTS test suite runs
(all the test cases) on the following systems:

- CYGWIN_NT-5.1 1.5.18(0.132/4/2) 2005-07-02 20:30 i686 unknown unknown Cygwin
- Linux 2.6.17-rc1-mm2 #1 SMP Thu Apr 13 14:03:28 MSD 2006 i686 i686 i386
GNU/Linux

for the following (all) modes supported by the ASLTS test suite:

- 32-bit norm mode
- 64-bit norm mode
- 32-bit slack mode
- 32-bit slack mode

The tests were run (on the systems described above) before update,
then the tests were run on those systems after update. Then results
of two runs were compared. All is Ok (the numbers of outstanding
allocations after update in norm mode are the same and in slack mode
are equal to or less than before update, no new errors).

Additional consideration 2.

Actually a relevant to the proposed patch action is
provided for normal Return Object processing in the function
AcpiDsRestartControlMethod (file interpreter/dispatcher/dsmethod.c)
if "the return value will not be used by the calling method":

        if (WalkState->ReturnUsed)
        {
            /* Save the return value from the previous method */
...
        else if (!AcpiDsDoImplicitReturn (ReturnDesc, WalkState, FALSE))
        {
            /*
             * Delete the return value if it will not be used by the
             * calling method
             */
            AcpiUtRemoveReference (ReturnDesc);
        }

So the following patch concerned with AcpiDsRestartControlMethod
and resolving the problem too looks like more preferable:

diff -r -u source3/components/interpreter/dispatcher/dsmethod.c 
source/components/interpreter/dispatcher/dsmethod.c
--- source3/components/interpreter/dispatcher/dsmethod.c	2006-04-24 
12:25:14.000000000 +0400
+++ source/components/interpreter/dispatcher/dsmethod.c	2006-05-08 
14:36:20.791169200 +0400
@@ -492,6 +492,7 @@
     ACPI_OPERAND_OBJECT     *ReturnDesc)
 {
     ACPI_STATUS             Status;
+    UINT32                  IsImplicitSelfsame;
 
 
     ACPI_FUNCTION_TRACE_PTR (DsRestartControlMethod, WalkState);
@@ -511,6 +512,9 @@
 
     if (ReturnDesc)
     {
+        /* Opportunely calculate Implicit Return specific condition */
+        IsImplicitSelfsame = (WalkState->ImplicitReturnObj == ReturnDesc);
+
         /* Are we actually going to use the return value? */
 
         if (WalkState->ReturnUsed)
@@ -539,7 +543,12 @@
          * NOTE: this is optional because the ASL language does not actually
          * support this behavior.
          */
-        else if (!AcpiDsDoImplicitReturn (ReturnDesc, WalkState, FALSE))
+        else if (!AcpiDsDoImplicitReturn (ReturnDesc, WalkState, FALSE) ||
+                /*
+                 * Take into account "complex statements"
+                 * processing in AcpiDsDoImplicitReturn
+                 */
+                IsImplicitSelfsame)
         {
             /*
              * Delete the return value if it will not be used by the

Created attachment 8044 [details]
Proposed patch 2

Essentials of patch 2 integrated and released in ACPICA version 20060512

ACPICA 20060608 shipped in 2.6.17-git9, closed. 

*** Bug 7163 has been marked as a duplicate of this bug. ***

re-opening because we need a version of this patch for 2.6.16/2.6.17 
 

Created attachment 9059 [details]
Patch for 2.6.16/2.6.17 

Extracted from patch-2.6.18.rc1

Thanks for also adding the backport. It's very much appreciated!
If there pop up similar things, it would be nice if you can add me into CC list 
if I am not already. It's very easy to oversee such important fixes.

Thanks.

Fixed since 2.6.18, patch for older kernels available.