Skip to content

Conversation

@uweigand
Copy link
Contributor

@uweigand uweigand commented Sep 2, 2021

  • Use little-endian byte order when writing integers to the buffer
    in pdbGetCvDebugInfo and pdbGetEmbeddedPdbDebugInfo

This showed up as a bug when bootstrapping the fsharp compiler natively on a big-endian machine (linux-s390x - IBM Z mainframe - which we're working on enabling for .NET 6). The compiler would create a .dll file with an invalid embedded PDB header, which would then cause the runtime to abort with a failed assertion when running that .dll. With this patch, native bootstrap of fsharp is successful on linux-s390x.

The implementation of "i32AsBytes" is copied from fsharp/absil/il.fs.

* Use little-endian byte order when writing integers to the buffer
  in pdbGetCvDebugInfo and pdbGetEmbeddedPdbDebugInfo
// Portable PDB Writer
//---------------------------------------------------------------------

let b0 n = (n &&& 0xFF)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can open Internal.Utilities.Library.Extras.Bits

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i32AsBytes could be moved from il.fs into lib.fs too, there are a few duplications of these

Copy link
Contributor Author

@uweigand uweigand Sep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Just to clarify, that module seems to contain the b0 .. b3 functions, but not i32AsBytes, so the latter would have to remain, right? EDIT: sorry, missed your update. I'll look into that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually OK, this is not the only duplication, we can clean that up later I think

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged - if you'd like to submit a PR consolidating b0 etc that would be cool

@dsyme
Copy link
Contributor

dsyme commented Sep 2, 2021

Great fix, thanks!

@dsyme dsyme merged commit efeb98e into dotnet:main Sep 2, 2021
@uweigand
Copy link
Contributor Author

uweigand commented Sep 2, 2021

Thanks! I'll prepare a separate PR to clean up the duplication.

As to this PR - it would be great to have this in .NET 6 as the problem prevents bootstrapping the compiler on s390x (and therefore prevents a source-build of the whole SDK to go through). Does this mean the PR will have to be backported to the dev17 branch, and if so, would that be possible?

@uweigand
Copy link
Contributor Author

uweigand commented Sep 3, 2021

As to this PR - it would be great to have this in .NET 6 as the problem prevents bootstrapping the compiler on s390x (and therefore prevents a source-build of the whole SDK to go through). Does this mean the PR will have to be backported to the dev17 branch, and if so, would that be possible?

Never mind, I see this has already happened automatically.

Thanks! I'll prepare a separate PR to clean up the duplication.

Actually, I could use a bit of advice here. It seems the duplication is not quite identical: the "b0" .. "b3" functions in lib.fs return "int" while a number of the duplicates return "byte". (Note that all copies of the 64-bit "dw0" .. "dw7" routines return "byte".)

If those are to be merged, should the result preferably be "int" or "byte" ? Either way seems to require a number of follow-on changes to fix the fall-out.

@uweigand uweigand deleted the bigendian-pdb-fix branch September 3, 2021 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants