Tsify lib/compilers #4609

jeremy-rifkin · 2023-01-17T03:12:49Z

No description provided.

jeremy-rifkin · 2023-01-17T06:19:49Z

lib/compilers/analysis-tool.ts

+            execute: false,
+            demangle: false,
+            libraryCode: false,
+            trim: false,


Are these the right defaults?

I think we need: demangle: true but everything else looks good to me. @partouf is that the right default for libraryCode - I think so.

both demangle and libraryCode and are defaulted to true in the UI, but so are directives and commentOnly, I think we leave the defaults to the UI and people using the API should do everything manually. Because then they'll know what might be filtered out - instead of having everything filtered out by default and then they don't know what they might be missing.

I don't really know what this is doing, but it's probably missing binaryObject if the goal is to init all possible filters.

please, do add it

BaseCompiler.getDefaultFilters is implemented as

compiler-explorer/lib/base-compiler.ts

Lines 2755 to 2768 in 784393b

getDefaultFilters() {

return {

binary: false,

execute: false,

demangle: true,

intel: true,

commentOnly: true,

directives: true,

labels: true,

optOutput: false,

libraryCode: false,

trim: false,

};

}

It looks like the return type should be ParseFiltersAndOutputOptions so I will update it

@dkm What should the default for binaryObject be?

I am pretty sure false. I think we could "just" try that and fix it up (in order to get this merged)

jeremy-rifkin · 2023-01-17T06:22:50Z

lib/compilers/java.ts

+                            {
+                                text: objResult.stdout,
+                            },
+                        ],


Unsure if this is correct, just made a best guess for how to put this in a ParsedAsmResult

Very good question .. this will need to be tested. But...I wonder how it worked before if this isn't quite right ...

I'm cool with finding out by merging and dealing with the consequences

With the regression in #4668 I'm wondering how this worked before 🤔

jeremy-rifkin · 2023-01-17T06:22:55Z

lib/compilers/java.ts

+                            {
+                                text: `<No output: javap returned ${objResult.code}>`,
+                            },
+                        ];


jeremy-rifkin · 2023-01-17T06:26:31Z

lib/compilers/nvcc.ts

        const maxSize = this.env.ceProps('max-asm-size', 64 * 1024 * 1024);
        const optPromise = result.hasOptOutput ? this.processOptOutput(result.optPath) : Promise.resolve('');
        const asmPromise = (
            filters.binary
-                ? this.objdump(outputFilename, {}, maxSize, filters.intel, filters.demangle, filters)
+                ? this.objdump(outputFilename, {}, maxSize, filters.intel, filters.demangle, false, false, filters)


Typescript uncovered a bug here, staticReloc and dynamicReloc weren't being passed. Is passing false for both appropriate?

I think so: @dkm I think is the one to know for certain.

Yes, passing false is valid in case of doubt. Using true will add extra parameters to objdumper, which may or may not be a good idea (don't know anything about nvcc tools)

jeremy-rifkin · 2023-01-17T06:28:20Z

lib/compilers/pascal.ts

        const dirPath = path.dirname(outputFilename);
        const execBinary = this.getExecutableFilename(dirPath);
        if (await utils.fileExists(execBinary)) {
-            return super.objdump(execBinary, result, maxSize, intelAsm, demangle);
+            return super.objdump(execBinary, result, maxSize, intelAsm, demangle, staticReloc, dynamicReloc, filters);


Same here, three arguments weren't being passed and also weren't present in the parameter list. I went ahead and just added them to the parameters / forwarded.

jeremy-rifkin · 2023-01-17T06:30:27Z

lib/compilers/ppci.ts

                return exec.execute(python, python_args, options);
            }
-            logger.error(`Invalid ppci path ${compiler}`);
+            assert(false, `Invalid ppci path ${compiler}`);


I think an assert here is appropriate. Something to kill the code path is needed otherwise TS isn't happy that undefined is being returned.

How is assert(false) better than throw new Error() in this case?

I think that'd be more or less equivalent. assert does ultimately throw a new Error internally. So it's just whatever extra diagnostics assert adds in.

Got it. Maybe we should make a fatalError(); that makes it clear? (I just have a vague dislike of assert(false) but that's mainly C PTSD..:)

jeremy-rifkin · 2023-01-17T06:30:45Z

lib/compilers/ptxas.ts

                const match = line.replace(/\x1B\[[\d;]*[Km]/g, '').match(re);
                if (match) {
                    lineObj.text = `<source>:${match[1]} ${match[2].trim()}`;
                    lineObj.tag = {
+                        severity: 0,


Not sure if this is correct, some severity is needed for the tag

jeremy-rifkin · 2023-01-17T06:31:49Z

lib/compilers/typescript-native.ts

+                        text: 'Failed to run compiler to get MLIR code',
+                    },
+                ],
+            };


Not sure if this is correct, made a best effort to get things into the correct form

lib/compilers/win32.ts

jeremy-rifkin · 2023-01-17T06:33:36Z

types/asmresult/asmresult.interfaces.ts

@@ -37,7 +37,7 @@ export type ParsedAsmResult = {
    parsingTime?: string;
    filteredCount?: number;
    externalParserUsed?: boolean;
-    objdumpTime?: number;
+    objdumpTime?: number | string;


Apparently it's set to a string in one of the compilers

Oh that's odd - which compiler? Seems like it ought not to be (maybe a TODO?)

java was the only compiler erroring however other compilers do similar assignments, e.g. clang does result.objdumpTime = objResult.execTime; but result is any (objResult.execTime is a string). I'm not sure how this type is used throughout the codebase, I'll go ahead and add a TODO.

Its set to a string, but the frontend always takes that into account. There was a good reason for it, but I really can’t remember why. But it is what it is. We can change it later on when we have a clear view of what everything is and should be

jeremy-rifkin · 2023-01-17T06:39:41Z

Ok, not sure what is causing that test to fail. Will investigate more tomorrow.

jeremy-rifkin · 2023-01-17T22:50:32Z

Ah shoot lots of merge conflicts because I did npm run lint on main, should have waited

mattgodbolt

ran out of time at 32/78 files reviewd. looking good though!

mattgodbolt · 2023-01-24T03:36:41Z

lib/base-compiler.ts

@@ -98,7 +98,7 @@ export class BaseCompiler implements ICompiler {
    protected toolchainPath: any;
    public possibleArguments: CompilerArguments;
    protected possibleTools: ITool[];
-    protected demanglerClass: any;
+    protected demanglerClass: any; // TODO(jeremy-rifkin): Will be able to do better once #4607 lands


#4607 has landed now (yay)

mattgodbolt · 2023-01-24T04:23:37Z

lib/base-compiler.ts

@@ -1254,7 +1254,7 @@ export class BaseCompiler implements ICompiler {
        }
    }

-    getExecutableFilename(dirPath, outputFilebase, key?) {
+    getExecutableFilename(dirPath: string, outputFilebase: string, key?) {


do we know what the key? type is? looks like : any from getOutputFilename though I suspect it's possible to imrpove on that...though not in this PR necessarily

I think key "should" be something that looks like the result of https://github.com/compiler-explorer/compiler-explorer/blob/main/lib/base-compiler.ts#L1681 and in some cases what's returned by https://github.com/compiler-explorer/compiler-explorer/blob/main/lib/base-compiler.ts#L1685 - in theory at least

I'll probably defer this to another PR, this PR is a big enough review already :)

mattgodbolt · 2023-01-24T04:26:21Z

lib/compilers/analysis-tool.ts

+            execute: false,
+            demangle: false,
+            libraryCode: false,
+            trim: false,


I think we need: demangle: true but everything else looks good to me. @partouf is that the right default for libraryCode - I think so.

mattgodbolt · 2023-01-24T04:47:29Z

lib/compilers/jakt.ts

@@ -49,8 +50,8 @@ export class JaktCompiler extends BaseCompiler {
        maxSize: number,
        intelAsm,


aren't these also boolean too?

Not sure, this matches the definition in BaseCompiler at the moment

mattgodbolt · 2023-01-24T04:50:22Z

lib/compilers/java.ts

+                            {
+                                text: objResult.stdout,
+                            },
+                        ],


Very good question .. this will need to be tested. But...I wonder how it worked before if this isn't quite right ...

partouf · 2023-01-27T14:42:16Z

I'm really not happy about all the constructors getting passed the type CompilerInfo & Record<string, any> - if we can't yet solidify the CompilerInfo type, then at least make a separate type CompilerInfo = IncompleteCompilerInfo & Record<string, any> or something so that we don't have to go back and change all the constructors again and have just 1 spot to change it.

jeremy-rifkin · 2023-01-28T07:31:30Z

@partouf You're absolutely right, I'm not sure why I did that 😄 I went ahead and fixed the CompilerInfo type

mattgodbolt · 2023-01-29T17:40:03Z

I realised I just did a couple of these in separate PRs; whoops. Will review this instead 👍

mattgodbolt

LGTM! I think we should fail forward. I'm a bit confused about the TODOs that refer their own PR number...are those planned to be done before merging? If so: great, if not then they will refer to a closed issue :)

mattgodbolt · 2023-01-29T17:41:34Z

lib/base-compiler.ts

@@ -99,7 +99,8 @@ export class BaseCompiler implements ICompiler {
    protected toolchainPath: any;
    public possibleArguments: CompilerArguments;
    protected possibleTools: ITool[];
-    protected demanglerClass: any;
+    // TODO(jeremy-rifkin): Will be able to do better once #4607 lands


#4607 has landed (but the TODO stands)

Ah shoot I wasn't paying attention here, ty!

mattgodbolt · 2023-01-29T17:41:55Z

lib/base-compiler.ts

@@ -1254,7 +1254,7 @@ export class BaseCompiler implements ICompiler {
        }
    }

-    getExecutableFilename(dirPath, outputFilebase, key?) {
+    getExecutableFilename(dirPath: string, outputFilebase: string, key?) {


mattgodbolt · 2023-01-29T17:43:10Z

lib/base-compiler.ts

@@ -2225,7 +2231,9 @@ export class BaseCompiler implements ICompiler {

        if (!bypassCache) {
            const cacheRetreiveTimeStart = process.hrtime.bigint();
-            const result = await this.env.cacheGet(key);
+            // TODO: Because key coantains a CompilerInfo which contains a function member it can't be assigned to a


what is the "todo" part? This seems like a reason to cast to any for sure, but I'm not sure what the action to take is.

TODO to eliminate the ugly any cast, key should be cacheable (if it's not that's a big problem), it's just a matter of getting the types lined up

I'll update it to be more clear

mattgodbolt · 2023-01-29T17:43:50Z

lib/compilers/analysis-tool.ts

+            execute: false,
+            demangle: false,
+            libraryCode: false,
+            trim: false,


I am pretty sure false. I think we could "just" try that and fix it up (in order to get this merged)

mattgodbolt · 2023-01-29T17:45:00Z

lib/compilers/jakt.ts

@@ -49,8 +50,8 @@ export class JaktCompiler extends BaseCompiler {
        maxSize: number,
        intelAsm,


mattgodbolt · 2023-01-29T17:45:26Z

lib/compilers/java.ts

+                            {
+                                text: objResult.stdout,
+                            },
+                        ],


I'm cool with finding out by merging and dealing with the consequences

mattgodbolt · 2023-01-29T17:46:16Z

lib/compilers/ldc.ts

        // These options make LDC produce an AST dump in a separate file `<inputFilename>.cg`.
        const newOptions = options.concat('-vcg-ast');
        const execOptions = this.getDefaultExecOptions();
+        // TODO(#4609) generateAST expects to return a ResultLine[] not a string


This TODO refers to itself? Should be a separate issue unless you plan o nfixing it before merging?

I was trying to do the same thing we did in #4490, I can make a separate issue for this though

mattgodbolt · 2023-01-29T17:48:14Z

lib/compilers/ppci.ts

                return exec.execute(python, python_args, options);
            }
-            logger.error(`Invalid ppci path ${compiler}`);
+            assert(false, `Invalid ppci path ${compiler}`);


How is assert(false) better than throw new Error() in this case?

mattgodbolt · 2023-01-29T17:48:34Z

lib/compilers/typescript-native.ts

+                        text: 'Failed to run compiler to get MLIR code',
+                    },
+                ],
+            };


mattgodbolt · 2023-01-29T17:49:34Z

types/asmresult/asmresult.interfaces.ts

@@ -37,7 +37,8 @@ export type ParsedAsmResult = {
    parsingTime?: string;
    filteredCount?: number;
    externalParserUsed?: boolean;
-    objdumpTime?: number;
+    // TODO(#4609) A few compilers seem to assign strings here. It might be ok but we should look into it more.


Another self-referencing TODO?

mattgodbolt · 2023-01-29T19:45:13Z

woo!

jeremy-rifkin added 2 commits January 16, 2023 22:12

First pass for converting lib/compilers

fb9d3ce

Second pass

000957c

github-actions bot added lang-circt lang-dotnet lang-fortran lang-jakt lang-mlir lang-racket https://racket-lang.org/ lang-toit labels Jan 17, 2023

jeremy-rifkin added 3 commits January 17, 2023 01:04

Fix java.ts and ldc.ts

786aa63

Lint and format

2f0657f

Fix compile

e73584a

jeremy-rifkin marked this pull request as ready for review January 17, 2023 06:18

jeremy-rifkin commented Jan 17, 2023

View reviewed changes

jeremy-rifkin requested review from mattgodbolt, partouf, dkm and RubenRBS January 17, 2023 06:53

Fix isNaN handling

76f8595

jeremy-rifkin force-pushed the jr/tsify/compilers branch from 449865a to 76f8595 Compare January 17, 2023 22:48

Merge branch 'main' into jr/tsify/compilers

275ad83

mattgodbolt reviewed Jan 24, 2023

View reviewed changes

jeremy-rifkin mentioned this pull request Jan 26, 2023

Backend TypeScript conversion tracking thread #4645

Closed

jeremy-rifkin added 4 commits January 27, 2023 19:20

Filter stuff

75104c6

getSharedLibraryLinks stuff

23867a8

Add quick TODO

9454618

Merge branch 'main' into jr/tsify/compilers

6b08e7d

jeremy-rifkin added 5 commits January 27, 2023 21:18

Fix compile

29258d8

Integrate demangler type

5b8f2ae

Fix compile part 2

a66dda4

Lint fix

206af29

CompilerInfo work

4f590c6

github-actions bot added the ui label Jan 28, 2023

jeremy-rifkin requested a review from mattgodbolt January 28, 2023 07:40

mattgodbolt approved these changes Jan 29, 2023

View reviewed changes

jeremy-rifkin mentioned this pull request Jan 29, 2023

LDC generateAST returns the wrong type #4654

Closed

Matt's comments

50f50c3

jeremy-rifkin mentioned this pull request Jan 29, 2023

Solidify ParsedAsmResult.objdumpTime's type #4655

Open

jeremy-rifkin merged commit 6a6efae into main Jan 29, 2023

jeremy-rifkin deleted the jr/tsify/compilers branch January 29, 2023 18:22

jeremy-rifkin mentioned this pull request Jan 31, 2023

Fix internal error on Java compilation #4668

Merged

This was referenced Feb 1, 2023

[BUG]: F# compiler not working at all #4675

Closed

[BUG]: Java/Kotlin support broken #4677

Closed

	getDefaultFilters() {
	return {
	binary: false,
	execute: false,
	demangle: true,
	intel: true,
	commentOnly: true,
	directives: true,
	labels: true,
	optOutput: false,
	libraryCode: false,
	trim: false,
	};
	}

		@@ -49,8 +50,8 @@ export class JaktCompiler extends BaseCompiler {
		maxSize: number,
		intelAsm,

Tsify lib/compilers #4609

Tsify lib/compilers #4609

Conversation

jeremy-rifkin commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

partouf Jan 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremy-rifkin commented Jan 17, 2023

jeremy-rifkin commented Jan 17, 2023

mattgodbolt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

partouf commented Jan 27, 2023 • edited

jeremy-rifkin commented Jan 28, 2023

mattgodbolt commented Jan 29, 2023

mattgodbolt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattgodbolt commented Jan 29, 2023

partouf Jan 27, 2023 •

edited

partouf commented Jan 27, 2023 •

edited