Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

When compiling/decompiling, some external references are lost #659

Open
1alessandro1 opened this issue Jan 8, 2021 · 14 comments
Open

When compiling/decompiling, some external references are lost #659

1alessandro1 opened this issue Jan 8, 2021 · 14 comments

Comments

@1alessandro1
Copy link

1alessandro1 commented Jan 8, 2021 •
edited

Compiler version was built today, about 10 minutes ago:


/*
 * Intel ACPI Component Architecture
 * AML/ASL+ Disassembler version 20210105 (64-bit version)
 * Copyright (c) 2000 - 2021 Intel Corporation
 *

Consider this SSDT with the following external references:

image

When compiling/decompiling, some of them get lost, generating compilation errors when the file gets first decompiled and then recompiled again

(Before compiling/decompiling)
image

After, when the compiled file is decompiled and recompiled again after 0 changes and 0 removals by the user, but the decompiler lost 4 External references, causing 4 errors

image
@SchmErik
Copy link
Contributor

please post the ssdt that exhibits this problem. Thanks!

@foskvs
Copy link

foskvs commented Jan 13, 2021

This is a smaller SSDT that has the same issue (I'll leave it as an attachment below).

Apparently, external references to MethodObj are deleted during the compilation phase if the said method isn't explicitly called in the code. I'm pretty sure this happens during the compilation because if I look at the raw binary data of SSDT.aml I don't see all the external references.
This doesn't happen with other object types (I tried with UnknownObj and the external reference is kept by the compiler).

If I try to decompile SSDT.aml and then re-compile it, I get the Illegal forward reference error.
However, if I force the creation of the AML file (iasl -f SSDT.dsl) and I execute it with acpiexec along with DSDT.aml it runs just fine:

% acpiexec *.aml
acpi_sem_0
acpi_sem_1
acpi_sem_2
acpi_sem_3
acpi_sem_4
acpi_sem_5
acpi_sem_6
acpi_sem_7
acpi_sem_8
acpi_sem_9
acpi_sem_10
acpi_sem_11
acpi_sem_12
acpi_sem_13
acpi_sem_14
acpi_sem_15
acpi_sem_16

Intel ACPI Component Architecture
AML Execution/Debug Utility version 20201217
Copyright (c) 2000 - 2020 Intel Corporation

Input file DSDT.aml, Length 0x77 (119) bytes

Input file SSDT.aml, Length 0x242 (578) bytes

ACPI: RSDP 0x0000000101AB7000 000024 (v02 Intel )
ACPI: XSDT 0x00007FD2AAC06060 00003C (v00 Intel  AcpiExec 00001001 INTL 20201217)
ACPI: FACP 0x0000000101AB6EA0 000114 (v05 Intel  AcpiExec 00001001 INTL 20201217)
ACPI: DSDT 0x00007FD2AAC060A0 000077 (v02 TSTVID DSDTID   00000001 INTL 20201217)
ACPI: FACS 0x0000000101AB7028 000040
ACPI: SSDT 0x00007FD2AAC06130 000242 (v02 TSTVID SSDTID   00000001 INTL 20201217)

ACPI table initialization:
Table [DSDT: DSDTID  ] (id 01) -    8 Objects with   3 Devices,   0 Regions,    2 Methods (0/2/0 Serial/Non/Cvt)
ACPI Debug:  "DDD0.MMM0 already exists: 0000000000000000"
ACPI Debug:  "DDD0.MMM1 already exists"
ACPI Debug:  "DDD1.MMM0 doesn't exist"
ACPI Debug:  "DDD1.MMM1 doesn't exist"
Table [SSDT: SSDTID  ] (id 02) -    2 Objects with   0 Devices,   0 Regions,    2 Methods (0/2/0 Serial/Non/Cvt)
ACPI: 2 ACPI AML tables successfully acquired and loaded

Final data object initialization: Namespace contains 19 (0x13) objects
Initializing General Purpose Events (GPEs):
    Initialized GPE 00 to 7F [_GPE] 16 regs on interrupt 0x0 (SCI)
    Initialized GPE 80 to FF [_GPE] 16 regs on interrupt 0x0 (SCI)
Initializing Device/Processor/Thermal objects and executing _INI/_STA methods:
    Executed 0 _INI methods requiring 0 _STA executions (examined 5 objects)

I think the following code complies with the ACPI Specification (correct me if I'm wrong):

If (!CondRefOf (\_SB.PCI0.DDD0.MMM0))
{
    Method (MMM0, 0, NotSerialized)
    {
        Return (One)
    }
}

My suggestion is:

  1. Keep all the external references during the compilation phase.
  2. Remove the Illegal forward reference error if it is caused by a conditional object reference (CondRefOf).
    Acpi_Tables_git.zip

@1alessandro1
Copy link
Author

@SchmErik any updates on the topic?

@SchmErik
Copy link
Contributor

SchmErik commented Jan 19, 2021 •
edited

I think the following code complies with the ACPI Specification (correct me if I'm wrong):

If (!CondRefOf (\_SB.PCI0.DDD0.MMM0))
{
    Method (MMM0, 0, NotSerialized)
    {
        Return (One)
    }
}

What happens when you remove the ! in the disassembled code so it looks like this?

If (CondRefOf (\_SB.PCI0.DDD0.MMM0))
{
    Method (MMM0, 0, NotSerialized)
    {
        Return (One)
    }
}

@foskvs
Copy link

foskvs commented Jan 20, 2021

What happens when you remove the ! in the disassembled code so it looks like this?

If (CondRefOf (\_SB.PCI0.DDD0.MMM0))
{
    Method (MMM0, 0, NotSerialized)
    {
        Return (One)
    }
}

If I remove the logical not operator I get the following error at runtime:

ACPI table initialization:
Table [DSDT: DSDTID  ] (id 01) -    8 Objects with   3 Devices,   0 Regions,    2 Methods (0/2/0 Serial/Non/Cvt)
ACPI Debug:  "DDD0.MMM0 doesn't exist"
Firmware Error (ACPI): Failure creating named object [\_SB.PCI0.DDD0.MMM0], AE_ALREADY_EXISTS (20201217/dswload2-480)
ACPI Error: AE_ALREADY_EXISTS, During name lookup/catalog (20201217/psobject-372)
ACPI: Skipping parse of AML opcode: Method (0x0014)

This happens because \_SB.PCI0.DDD0.MMM0 is already defined elsewhere (in DSDT.aml).
The external references disappear after I compile the SSDT.

If I compile and then decompile the SSDT, I still get the "Illegal forward reference" error.

The purpose of the SSDT @1alessandro1 is using is to define a Method if it isn't already defined.
(Some sort of:

#ifndef X
#define X
#endif

in C).

I more and more think that my suggestion 1 (keep all external references) is incorrect.
Digging into the source code I found this comment (source/compiler/aslexternal.c, line 585):

/*
 * Loop again to remove MethodObj Externals for which
 * a MethodCall was not found (dead external reference)
 */

So I think it is an intended behaviour.

Regarding my second suggestion, I tried with a few older versions of iasl. Here are the results:
With all versions: dead external references are removed.

With versions >= 20200430: I get the "Illegal forward reference error".
With versions <= 20200326: the SSDT compiles without any error.

I'm not sure if it can be considered a regression.

I have a possible solution:

diff acpica-master/source/compiler/aslxref.c acpica-master-2/source/compiler/aslxref.c
801,804c801,802
<             if (!(Op->Asl.Parent->Asl.ParseOpcode == PARSEOP_CONDREFOF)){
<                 AslError (ASL_ERROR, ASL_MSG_ILLEGAL_FORWARD_REF, Op,
<                           Op->Asl.ExternalName);
<             }
---
>             AslError (ASL_ERROR, ASL_MSG_ILLEGAL_FORWARD_REF, Op,
>                 Op->Asl.ExternalName);

But I don't know if this causes new bugs, so you may have a better solution.
If you need additional information feel free to ask.

@SchmErik
Copy link
Contributor

What happens when you remove the ! in the disassembled code so it looks like this?

If (CondRefOf (\_SB.PCI0.DDD0.MMM0))
{
    Method (MMM0, 0, NotSerialized)
    {
        Return (One)
    }
}

If I remove the logical not operator I get the following error at runtime:

Sorry, I should have been more clear - if you remove the ! in the disassembly, do you get a compilation error? I know that this will change runtime behavior. I'm only asking about compilation right now.

@foskvs
Copy link

foskvs commented Jan 20, 2021

Yes, I still get the “Illegal forward reference” compilation error.

@1alessandro1
Copy link
Author

Any updates on the topic?

@1alessandro1
Copy link
Author

@SchmErik What could be causing this issue?

@SchmErik
Copy link
Contributor

SchmErik commented Feb 1, 2021

sorry about the lack of response. The solution is to change the compiler to retain the externals. It would be nice to drop externals that are actually referenced. In other words, code like

External (EXT0, MethodObj)
if (CondRefOf (EXT0))
{
  ...
}

should indicate to the compiler that EXT0 is referenced and the EXT0 external declaration should not be tossed during compilation. The code that's removing this is located in aslexternal.c because it's technically not a method invocation, it gets removed. We need to change it to drop externals that are unreferenced rather than dropping external methods that are never invoked. I'll hopefully have this completed today..

@SchmErik
Copy link
Contributor

SchmErik commented Feb 2, 2021

Fix available here:

#663

Let me know if you see any issues

@1alessandro1 1alessandro1 mentioned this issue Feb 2, 2021
Merged
@1alessandro1
Copy link
Author

When #663 is merged this can be closed, awesome job!

@SchmErik
Copy link
Contributor

SchmErik commented Mar 2, 2021

I found an issue with the code in #663 so I'm going to revert it. I need some time to sort out how to solve this issue in a way that simplifies the logic while not breaking existing test cases. I'm going to start by adding changes so that iASL will unconditionally keep external method declarations. This will make things a little more simple... Feel free to use the code in #663 for your purposes right now though... I'll be gone this week but I'll be back next week.

@1alessandro1
Copy link
Author

1alessandro1 commented Mar 13, 2021 •
edited

I'll be gone this week but I'll be back next week.

Hi, since the changes were reverted, I'd like to know if you found a good fix for this weird bug

Keep us updated if you can, thanks

Ale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@SchmErik @foskvs @1alessandro1