ircmaxell / php-compiler

A compiler. For PHP
MIT License
794 stars 33 forks source link

MemoryManager/PHP.php not compatible with base class it extends #22

Closed driusan closed 5 years ago

driusan commented 5 years ago

The malloc, realloc, and free definitions in lib/JIT/MemoryManager/PHP.php take gccjit argument types as parameters and return gccjit type values, but the class extends lib/JIT/MemoryManager.php which use LLVM values for its types.

(I'd take a stab at updating it to use LLVM, but it's not clear to me if that's the desired approach, or if there's a plan to add some sort of shim so that both could be supported.)

ircmaxell commented 5 years ago

There is no plan for a shim. Feel free to get it updated to use LLVM directly. Good luck, and let me know what you find along the way that I need to document better...

I just pushed a new build which cleans up the macros a bit, and make be of some help in that effort...

driusan commented 5 years ago

How is (was?) MemoryManager\PHP used? I can't find any references to it with git grep.. the only reference I see to any new that instantiates a MemoryManager object is $this->memory = new Builtin\MemoryManager\Native($this, $loadType); in JIT\Context.

I was going to try just removing the function declarations in MemoryManager/PHP since the parent class already has LLVM implementations of them, but I'm starting to think maybe the whole class itself can go (and then PhanParamSignatureMismatch and PhanParamRealSignatureMismatchTooManyRequiredParameters class errors would be fixed..)

ircmaxell commented 5 years ago

So basically the PHP memory manager implemented malloc/realloc/free through PHP's emalloc, erealloc and efree instead of the stdlib versions.

There was a static function in MemoryManager that chose between the two implementations:

    public static function load(Context $context, int $loadType): self {
        if ($loadType === Builtin::LOAD_TYPE_STANDALONE) {
            return new MemoryManager\Native($context, $loadType);
        }
        return new MemoryManager\PHP($context, $loadType);
    }

So the way MemoryManager is implemented now, is it's expecting 3 internal functions to be defined in LLVM:

inline function __mm__malloc(size_t): int8*;
inline function __mm__realloc(int8*, size_t): int8*;
inline function __mm__free(int8*): void;

In the native version, these are just thin proxies over malloc/realloc/free. But for PHP, they should instead import and proxy to emalloc/erealloc/efree.

On another note, would it be useful to have a chat room for some of this discussion? Should I setup an IRC or Slack channel for it?

driusan commented 5 years ago

It should be easy enough to copy Native.pre to PHP.pre and change the class name and call sites to eX (which I think should fix this issue, but I'm still not sure how to test it..), but I can't find any reference to that load function in the MemoryManager. Should I put it back and use it instead of the explicit Native constructor in JIT/Context?

ircmaxell commented 5 years ago

Yeah, for now I think that works (add the static method back in). Longer term I'd like to be a bit smarter about it, but there's a lot to do between now and then ;)

driusan commented 5 years ago

Could you take a look at https://github.com/driusan/php-compiler/commit/f749d15abbe93e0b659d4bc1e2b7e0507544587b and let me know if you see something wrong? It's not emitting any code for the memory manager, it's just giving me "Warning: gzdecode(): data error in /home/driusan/Code/php-compiler/vendor/pre/plugin/source/functions.php on line 59" and emitting a zero byte file, and I don't know enough about \Pre\Plugin to debug. I don't see any obvious syntax errors.

driusan commented 5 years ago

@ircmaxell I think there's a problem with a macro somewhere. I've managed to track down my problems compiling MemoryManager.pre to this line: https://github.com/driusan/php-compiler/blob/PHPMemoryManager/lib/JIT/Builtin/MemoryManager.pre#L45 and this line: https://github.com/driusan/php-compiler/blob/PHPMemoryManager/lib/JIT/Builtin/MemoryManager.pre#L45

(ie trying to add the "extra" value to the malloc.) If I comment those out, MemoryManager.pre compiles for me, but without them I get the "Warning: gzdecode(): data error in /home/driusan/Code/php-compiler/vendor/pre/plugin/source/functions.php on line 58" error with the pre\plugin that comes from composer install, and a not much better error about internals of the macro system: " Fatal error: Uncaught RuntimeException: defer failed due to Warning: count(): Parameter must be an array or an object that implements Countable in /home/driusan/Code/php-compiler/vendor/pre/plugin/hidden/vendor/yay/yay/src/Ast.php on line 137

Fatal error: Uncaught TypeError: array_values() expects parameter 1 to be array, object given in /home/driusan/Code/php-compiler/vendor/pre/plugin/hidden/vendor/yay/yay/src/Expansion.php:384 Stack trace: [...]" if I switch to dev-master.

Do you have any ideas where to dig into that?

ircmaxell commented 5 years ago

I have had similar issues, resolved by I don't know what. I do know yay can be a bit finicky there. I've had times where just rewriting the macro helped (literally same content, just copy/paste over top of it). No idea why. May try to loop @marcioAlmada in for some help...

driusan commented 5 years ago

Fixed by #60