Remove /RESOURCE flag documentation on ilasm.

[jkoritzinsky]

Update the documentation of the /RESOURCE flag to reflect that we only support linking in .obj resource files.

Original:

ILAsm depends on an external tool (CvtRes.exe) being in the same directory as ILAsm. We don't own CvtRes.exe (and it currently isn't open-sourced), so we need to figure out how we want to implement resource embedding via ILAsm. In the meantime, we should remove the /RESOURCE flag since it won't work at all right now.

@AaronRobinsonMSFT PTAL.

[AaronRobinsonMSFT]

@RussKeldorph I suggested we remove this option for right now so we don't get into unrealistic expectations about what is supported. I am suggesting we entirely remove the option and see if there is any community desire for it. Any thoughts/concerns with this?

[RussKeldorph]

@AaronRobinsonMSFT Makes sense.

[tannergooding]

I thought we were using this for the ilproj in CoreFX?

[tannergooding]

https://github.com/dotnet/corefx/blob/master/src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.ilproj

[tannergooding]

We likely need an alternative for embedding resources into the System.Runtime.CompilerServices.Unsafe assembly before this can be removed (since it will negatively impact a production assembly otherwise).

[AaronRobinsonMSFT]

@tannergooding That target doesn't pass the /RESOURCE. In fact it does what the /RESOURCE flag should do manually (i.e. launches CvtRes.exe).

https://github.com/dotnet/corefx/blob/d7991c184d59bf0a62799e777b916ac35a4a0bd0/src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.ilproj#L39

This change will have no impact on that scenario - at least how that target file is currently written. If we added that /RESOURCE flag that code could be dramatically simplified.

[tannergooding]

@AaronRobinsonMSFT, that target calls CvtRes.exe to convert our resource file into the obj file. As an output, the target sets IlasmResourceFile: https://github.com/dotnet/corefx/blob/master/src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.ilproj#L67

We then consume the IlasmResourceFile in the ILProj SDK, here: https://github.com/dotnet/coreclr/blob/master/src/.nuget/Microsoft.NET.Sdk.IL/targets/Microsoft.NET.Sdk.IL.targets#L81

If you remove the switch, you will be dropping all the following resource information from the production assembly:

FILEVERSION    4,7,18,55501
PRODUCTVERSION 0,0,0,0
FILEFLAGSMASK  0x3F
FILEFLAGS      0x0
FILEOS         VOS_UNKNOWN | VOS__WINDOWS32
FILETYPE       VFT_DLL
FILESUBTYPE    0x0
{
  BLOCK "VarFileInfo"
  {
    VALUE "Translation", 0x0, 1200
  }
  BLOCK "StringFileInfo"
  {
    BLOCK "000004b0"
    {
      VALUE "Comments",          "System.Runtime.CompilerServices.Unsafe"
      VALUE "CompanyName",       "Microsoft Corporation"
      VALUE "FileDescription",   "System.Runtime.CompilerServices.Unsafe"
      VALUE "FileVersion",       "4.7.18.55501"
      VALUE "InternalName",      "System.Runtime.CompilerServices.Unsafe.dll"
      VALUE "LegalCopyright",    "© Microsoft Corporation. All rights reserved."
      VALUE "OriginalFilename",  "System.Runtime.CompilerServices.Unsafe.dll"
      VALUE "ProductName",       "Microsoft® .NET Framework"
      VALUE "ProductVersion",    "4.7.0-dev.18555.1+826b542edcf397312e3d2a27b00c4b5b6fbf3aff"
      VALUE "Assembly Version",  "4.0.5.0"
    }
  }
}

[tannergooding]

AFAIK, there is nothing blocking ILASM from using the /RESOURCE flag today, even on Linux or Mac. The only blocker is generating the resource obj file, which currently requires CvtRes.exe (or some open source alternative, a brief search shows a few such tools).

[jkotas]

currently requires CvtRes.exe (or some open source alternative, a brief search shows a few such tools)

CvtRes.exe or equivalent are unnecessary complications in this pipeline. It should be pretty straightforward to take the .res file and attach it to the assembly, without going through intermediate .obj file. It is just pain to do in ilasm codebase in C/C++ - maybe we can do it as post-processing step in C#.

[AaronRobinsonMSFT]

@tannergooding In the second targets file you mention there is usage of the /RESOURCES flag, but in this PR we are removing the /RESOURCE flag. Judging from the argument processing in ILASM this is a bug, but either way, you are right this flag is being used. That is a problem so this PR needs to change.

which currently requires CvtRes.exe (or some open source alternative, a brief search shows a few such tools).

No one is arguing with that. The initial thought here was that no one was using that flag since it doesn't work out of the box at the moment - at least from the CORE_ROOT environment under a Check build. That points to the fact that the flag, at the moment, is broken and no one should b e relying on it. this is further demonstrated in the initially mentioned targets file that is manually calling CvtRes.exe.

CvtRes.exe or equivalent are unnecessary complications in this pipeline.

@jkotas Yep, I fully agree. I was hoping there was nothing using the current flag but alas there is.

@jkoritzinsky Care to take a stab at @jkotas suggestion and call the appropriate Win32 calls to fix this? Otherwise, it looks like we should leave this as is, but please create a bug that calls out the /RESOURCE flag should be properly implemented with a removal of the CvtRes.exe dependency.

[tannergooding]

@AaronRobinsonMSFT, the /RESOURCES flag doesn't work out of the box if you pass in a .res file, it works just fine if you pass in a .obj file (that was output from something like CvtRes).

More specifically, the ConvertResource function is the thing that doesn't entirely work today:

coreclr/src/dlls/mscorpe/ceefilegenwriter.cpp

Line 1040 in 030a3ea

HRESULT ConvertResource(const WCHAR * pszFilename, __in_ecount(cchTempFilename) WCHAR *pszTempFilename, size_t cchTempFilename, PEWriter &pewriter)

• It only works in the case where the input is already a .obj file, in which case it is effectively a no-op
• ConvertResources is called by emitResourceSection:

  coreclr/src/dlls/mscorpe/ceefilegenwriter.cpp

  Line 1118 in 030a3ea

  HRESULT CeeFileGenWriter::emitResourceSection()

[AaronRobinsonMSFT]

doesn't work out of the box if you pass in a .res file, it works just fine if you pass in a .obj file (that was output from something like CvtRes).

Ah. That is the part I didn't realize. The doc just says res file. I hadn't debugged the entire code path so wasn't aware of the this additional option. Okay then that seems like the correct thing to do here is update the documentation and the filed bug should indicate needed support for a res to obj converter - either in native or as @jkotas suggested in a post-process step. I don't know the cost for either solution though.

End result here is the flag needs to stay but should be documented properly.

Thanks for clarifying the underlying issue!

[jkoritzinsky]

I've updated the documentation of the switches in ilasm and reverted the removal of the switch.

[jkoritzinsky]

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

[jkoritzinsky]

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test