ping :)

In D149895#4321169, @Stoorx wrote:

Why? Is it a piece of dead-code or it is never used (and surely will never be)?

Please provide some reasoning.

This commit landed and fixes the build error: 14390ecb75b28cca2b151c70bb1718b36876f977. Will close this patch.

In D98437#4309972, @NathanielMcVicar wrote:

After the fixes there still appears to be a break on Windows builds https://lab.llvm.org/buildbot/#/builders/13/builds/34808/steps/6/logs/stdio

C:\buildbot\mlir-x64-windows-ninja\llvm-project\llvm\lib\Object\GOFFObjectFile.cpp(330) : error C2220: the following warning is treated as an error
C:\buildbot\mlir-x64-windows-ninja\llvm-project\llvm\lib\Object\GOFFObjectFile.cpp(330) : warning C4715: 'llvm::object::GOFFObjectFile::getSymbolType': not all control paths return a value

I posted a fix here: https://reviews.llvm.org/D149602

The preconditions for this patch have been committed: https://reviews.llvm.org/D148821
If there are no additional comments on this patch, I will commit it.

This updates the use of the charset convertor to latest changes from https://reviews.llvm.org/D148821

Update documentation for CCAssignToRegAndStack in WritingAnLLVMBackend.rst

In D125725#3523243, @mib wrote:

Hey @yusra.syeda ! It looks like your patch broke the macOS lldb bot: https://green.lab.llvm.org/green/job/lldb-cmake/

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/BinaryFormat/GOFF.h:23:23: error: missing '#include <_types/_uint8_t.h>'; 'uint8_t' must be declared before it is used
enum SubsectionKind : uint8_t {
                      ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/include/_types/_uint8_t.h:31:23: note: declaration here is not visible
typedef unsigned char uint8_t;

You're basically missing an include. Let me know if you need help reproducing / fixing this 🙂

apply clang format

Rebasing on latest main

Removed the FIXME

Removed unnecessary white space, empty constructors, and addressed minor comments

LGTM

In D103490#2825462, @kpn wrote:

Say, I can't find any file in the llvm repository named "GOFF*". They don't show up on Github, either, so I'm not sure what's going on. Where is the GOFF support in the LLVM tree currently? There's no "https://github.com/llvm/llvm-project/tree/main/llvm/lib/Object/GOFFObjectFile.cpp", for example.

Removed unused enum and a redundant if statement

Updated the unit test and cleaned up return statements

Update the getElementEsdId function

Removed unused functions, and addressed some more review comments

Address comments in the unit test

ping :)

Fixed typo

Changed variable type from int to size_t for RecordNum variable, fixed build error, and added size to enums

Moved function body of the getData function to GOFFObjectFile.cpp to avoid clang tidy warning

Added record number to the error messages

Fixed typo

Added more detail to error messages
Added missing include

Removed setters from llvm/include/llvm/Object/GOFF.h.
I will add more details to the error messages in the next update.

Fixed function naming style

Moved non trivial function out of header file and into llvm/lib/Object/GOFFObjectFile.cpp

Addressed comments and cleaned up code to adhere to LLVM style

Addressed some comments and added the following tests:
Check for invalid symbol types
Check for invalid external reference executable types
Check if a record is a continuation, then the previous record should be marked as continued
Check that the last continuation record is terminated correctly

Added the following tests:
Invalid cases of continutation records
Check for missing HDR record
Check for missing END record
Handling of concatenated GOFF object files

Ping :)

Clean up some formatting

Address comment regarding removing unnecessary variables
Address clang tidy suggestion

In D98437#2672051, @kpn wrote:

The convention here is to wait a week or so before a ping, and the same between pings.

Added more error checking

Double ping :) All comments have been addressed. Reviewers, please have a look.

Ping :)

Removed Mask variable in getBits function

Address more formatting comments

Addressed formatting comments

Added test for ESD continuation records, and for 2 symbols in the symbol table

Updated handling of continuation records
Addressed other comments in the patch

I created a smaller patch supporting just the HDR, ESD and END records here: https://reviews.llvm.org/D98437. Please continue the review in the new patch.

In D89071#2614095, @kpn wrote:

In D89071#2610685, @jhenderson wrote:

The majority of your code is still untested as far as I can see. There appear to be three test cases you have so far:

1. An invalid size for a GOFF object.
2. A valid size for a GOFF object.
3. That getSymbolName returns the name of a single symbol in the symbol table.

What about all the rest of the functionality that is included in this patch, including, but certainly not limited to, the following?

1. More than one symbol in the symbol table.
2. Other properties of symbols.
3. The various properties of records.
4. Relocations.
5. And so on...

For each bit of code you have written, consider whether a test would fail if that bit of code was broken in some way, or didn't exist. If no test fails, then that code needs a new test case of some form. There may also be other cases where testing is appropriate, e.g. where two separate aspects of the same system interact in some way, although those are harder to judge.

Here's an idea: I found, at least when running in batch, that the Binder (linker) will link an object consisting of nothing more than a HDR card, then ESD cards followed by an END card (meaning, just symbols). It will also link an object consisting of HDR, ESD, TXT, and END cards with zero relocations. Does it work that way when not running in batch? Because if it does then it might make sense to split this ticket up into a new ticket with just support for HDR+ESD+END cards.

That would make this patch smaller, and it would reduce the amount of tests that need to be written to get some initial GOFF support into the tree. The tests that @jhenderson requested would still be needed, but you'd only need the ones that were relevant to the smaller amount of code in the new ticket. A new ticket should refer back to this ticket because this ticket shows the direction you are going, and it has a bunch of comments that should probably be left for posterity. Later tickets can build on this foundation.

The LLVM community tends to prefer smaller patches over larger ones. Typically, anyway.

It's an idea. Thoughts?

In D89071#2486161, @jhenderson wrote:

I've not reviewed the testing yet, but my immediate thought is that there needs to be a lot more, handling all the different code paths.

@jhenderson can you please review the testing? Currently the tests construct a GOFF object with a valid sized record (80 bytes), an invalid sized record (!80 bytes), and obtains the symbol name from the ESD record. Testing for relocations will be added in a future patch.

Added size to ESDExecutable enum
Reformatted unit tests

Fix error message formatting

Apply clang format suggestion