Improve handling of nul-terminated string in Globals. #1350

galenh · 2024-08-15T23:49:11Z

When StringType size is calculated, save it to the underlying ArrayType.
Re-used existing DataTypes from Globals instead of re-creating them when collecting types.
Fix an inverted "<=" error.
Print linear arrays on a single line.

uxmal · 2024-08-16T09:24:12Z

Hello @galenh ; thanks for your PR. I have some comments that need to be addressed before accepting it. Also, as a new contributor, don't forget to add yourself to the AUTHORS file!

uxmal

I've finished the review. There are some issues that must be addressed before I can accept the PR. Notably, as it stands right now neither the unit tests nor the regression tests are passing.

Consider running:

msbuild /t:run_unit_tests /v:m /m /p:Platform=x64 /p:Configuration=Release|Debug
msbuild /t:run_regressions /v:m /m /p:Platform=x86 /p:Configuration=Release|Debug

to discover where the test failures are.

uxmal · 2024-08-16T09:27:35Z

_.vscode/launch.json

@@ -0,0 +1,27 @@
+{
+   // Use IntelliSense to find out which attributes exist for C# debugging


Can you explain why this file is here, and not in .vscode? I'm not familiar with the significance of the leading underscore.

uxmal · 2024-08-16T09:29:34Z

src/Core/Output/AbsynCodeFormatter.cs

@@ -100,13 +100,15 @@ private string UnsignedRepresentation(Constant u)
            else
            {
                var sb = new StringBuilder();
+#if USE_TILDA_IF_SMALLER                


This looks like a user preference, and it shouldn't be coded using #if / #endif. Instead consider opening an issue to add support for changing the formatting of unsigned numbers, controlled by user preference. There we can discuss how this would best be accomplished.

uxmal · 2024-08-16T09:33:52Z

src/Core/Output/GlobalDataWriter.cs

@@ -205,19 +205,17 @@ public bool VisitArray(ArrayType at)
            fmt.Terminate();
            fmt.Indent();
            fmt.Write("{");
-            fmt.Terminate();
-            fmt.Indentation += fmt.TabSize;
+            fmt.Write(" ");


You've changed the rendering of arrays so that they are rendered on a single line. What happens when an array of 1000 entries is encountered? I prefer the current state of affairs, until Reko can prettify the output source.

In addition, you haven't changed the GlobalDataWriterTests.cs unit tests to match, so the CI pipeline will fail. Make sure there is a unit tests to cover this case.

There is the beginning of a code prettifier in Core\Output\PrettyPrinter.cs that should be able to format arrays nicely. Consider opening an issue and using that class to make data output prettier.

uxmal · 2024-08-16T09:38:51Z

src/Core/Output/TypedDataDumper.cs

@@ -182,7 +182,7 @@ public void VisitString(StringType str)
                    while (rdr.TryReadByte(out byte b))
                    {
                        //$REVIEW: assumes ASCII.
-                        if (0x20 <= b && b < 0x7F)
+                        if (0x20 >= b && b < 0x7F)


You've introduced a bug. This expression will print out ASCII control characters (those whose code points are < 0x20) which is not the intent of this code. Running the unit tests would have caught this mistake. Please revert this change.

uxmal · 2024-08-16T09:48:32Z

src/Core/Program.cs

@@ -669,6 +669,7 @@ public uint GetDataSize(IProcessorArchitecture arch, Address addr, DataType dt)
                    return 0;
                while (rdr.IsValid && !rdr.ReadNullCharTerminator(strDt.ElementType))
                    ;
+                strDt.Length = (int) (rdr.Address - addr);


I'm a little unhappy about mutating the string data type when it's being used in what semantically is a read-only query type function. Consider looking at setting the size when the string is created.

uxmal · 2024-08-16T09:51:01Z

src/Decompiler/Typing/TypeCollector.cs

@@ -109,7 +109,11 @@ public void CollectUserGlobalVariableTypes()
            {
                if (ud.Value.DataType != null)
                {
-                    var dt = ud.Value.DataType.Accept(deser);
+                    var dt = program.FindGlobalField(ud.Key);
+                    if (dt == null)


Consider using the dt is null syntax here.

uxmal · 2024-08-16T09:51:38Z

src/Reko-decompiler.sln

SLN files are notoriously hard to view the diff on. What is the nature of the changes you've made here?

uxmal · 2024-08-16T09:54:02Z

src/binall.cmd

This looks like a private cmd file you've added. What is the purpose of "binall"? Could it be accomplished using Reko's release script? What is a LasertType\LaserImage?

uxmal · 2024-08-16T09:55:15Z

src/buildall.cmd

I'd prefer it if this was placed in the src/tools directory, to avoid clutter in the src directory.

uxmal · 2024-08-16T09:55:46Z

src/cleanall.cmd

Have you considered using git clean -dfX instead of this?

uxmal

I've added a few more comments. The changes are starting to become overwhelming, and could benefit from being broken up into separate, more focused PR's

uxmal · 2024-08-16T10:07:50Z

src/Core/Output/TypedDataDumper.cs

@@ -41,11 +41,99 @@ public TypedDataDumper(EndianImageReader rdr, uint cbSize, Formatter stm)
        public void VisitArray(ArrayType at)
        {
            var addrEnd = rdr.Address + cbSize;
-            for (int i = 0; at.IsUnbounded || i < at.Length; ++i)
+
+            if (at.ElementType.Domain < Domain.Composite) 


Relying on an enum's value in this way is fragile. Consider exposing a DataType.IsComposite property in a separate PR` to accomplish this.

uxmal · 2024-08-16T10:09:28Z

src/Core/Output/TypedDataDumper.cs

-                if (!rdr.IsValid || addrEnd <= rdr.Address)
-                    return;
-                at.ElementType.Accept(this);
+                int offset = 0;


There appears to be a lot of duplication here; could this code and the code in VisitPrimitiveType be harmonized?

uxmal · 2024-08-16T10:10:20Z

src/Core/Output/TypedDataDumper.cs

+                        Debug.Assert(i < size);
+                    }
+                    if (inStringLiteral)
+                    {


Please add a unit tests to show how this is an improvement.

galenh added 3 commits August 15, 2024 16:44

Improve handling of nul-terminated string in Globals.

7953bb7

Disable Tildas, especially ~0x00 for 0xff.

a134333

Build changes.

c680779

galenh added 2 commits August 16, 2024 02:36

Better array printing to disassembler.

9a4086b

Build changes.

0b902e9

uxmal requested changes Aug 16, 2024

View reviewed changes

uxmal reviewed Aug 16, 2024

View reviewed changes

Improve logging

a33145c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of nul-terminated string in Globals. #1350

Improve handling of nul-terminated string in Globals. #1350

galenh commented Aug 15, 2024

uxmal commented Aug 16, 2024

uxmal left a comment

uxmal Aug 16, 2024

uxmal Aug 16, 2024

uxmal Aug 16, 2024

uxmal Aug 16, 2024

uxmal Aug 16, 2024

uxmal Aug 16, 2024

uxmal Aug 16, 2024

uxmal Aug 16, 2024

uxmal Aug 16, 2024

uxmal Aug 16, 2024

uxmal left a comment

uxmal Aug 16, 2024

uxmal Aug 16, 2024

uxmal Aug 16, 2024

		@@ -0,0 +1,27 @@
		{
		// Use IntelliSense to find out which attributes exist for C# debugging

Improve handling of nul-terminated string in Globals. #1350

Are you sure you want to change the base?

Improve handling of nul-terminated string in Globals. #1350

Conversation

galenh commented Aug 15, 2024

uxmal commented Aug 16, 2024

uxmal 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

uxmal 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