This is an archive of the discontinued LLVM Phabricator instance.

[CodeView] Clamp Frontend version
ClosedPublic

Authored by modimo on Dec 23 2021, 2:11 PM.

Details

Summary

D43002 introduced a test debug-info-objname.cpp that outputted the current compiler version into CodeView. Internally we appended a date to the patch version and overflowed the 16-bits allocated to that space. This change clamps the Frontend version outputted values to 16-bits like rGd1185fc081ead71a8bf239ff1814f5ff73084c15 did for the Backend version.

Testing:
ninja check-all
newly added tests correctly clamps and no longer asserts when trying to output the field

Diff Detail

Unit TestsFailed

Event Timeline

modimo created this revision.Dec 23 2021, 2:11 PM
modimo requested review of this revision.Dec 23 2021, 2:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2021, 2:11 PM
modimo retitled this revision from [CodeView] Clamp Frontend Version to [CodeView] Clamp Frontend version.Dec 23 2021, 2:14 PM
modimo edited the summary of this revision. (Show Details)
modimo added reviewers: aganea, amccarth.
modimo edited the summary of this revision. (Show Details)
modimo updated this revision to Diff 396092.Dec 23 2021, 2:18 PM

Formatting

modimo updated this revision to Diff 396110.Dec 23 2021, 3:08 PM

Remove variable backend version check

aganea accepted this revision.EditedDec 24 2021, 11:02 AM

LGTM, thanks!

I guess you could do 4.0.2021.1223 if you want the version to be properly stamped?

This revision is now accepted and ready to land.Dec 24 2021, 11:02 AM

LGTM, thanks!

Thanks for reviewing.

I guess you could do 4.0.2021.1223 if you want the version to be properly stamped?

Trying out with -DLLVM_VERSION_PATCH=2021.1223 I'm only seeing 14.0.2021 showing up in versioning so it looks like the rest of the compiler needs to be plumbed through to support full date stamping. I think we can change how we stamp internally (say just month/date) knowing now that the field is restricted to 16-bits. Out of curiosity do you know if S_COMPILE3 requires each version field to be 16-bits or can it be larger?

Out of curiosity do you know if S_COMPILE3 requires each version field to be 16-bits or can it be larger?

It needs to be 16-bits, please see: https://github.com/microsoft/microsoft-pdb/blob/082c5290e5aff028ae84e43affa8be717aa7af73/include/cvinfo.h#L3361

Out of curiosity do you know if S_COMPILE3 requires each version field to be 16-bits or can it be larger?

It needs to be 16-bits, please see: https://github.com/microsoft/microsoft-pdb/blob/082c5290e5aff028ae84e43affa8be717aa7af73/include/cvinfo.h#L3361

Got it, thanks for the reference.

This revision was landed with ongoing or failed builds.Dec 28 2021, 3:22 PM
This revision was automatically updated to reflect the committed changes.