This is an archive of the discontinued LLVM Phabricator instance.

[ms] [llvm-ml] Fix case-sensitivity for variables and textmacros
ClosedPublic

Authored by epastor on Dec 2 2020, 1:03 PM.

Details

Summary

Make variables and text-macro references case-insensitive, to match ml.exe.

Also improve error handling for text-macro expansion.

Diff Detail

Event Timeline

epastor created this revision.Dec 2 2020, 1:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 1:03 PM
epastor requested review of this revision.Dec 2 2020, 1:03 PM
thakis added inline comments.Dec 3 2020, 6:50 AM
llvm/lib/MC/MCParser/MasmParser.cpp
1549

This looks up a label, and if that fails, tries to look up a var with the same (lowered) name. Is that intentional? If so, could you mention that in the patch summary? As far as I understand, this change is unrelated to what's currently in the description.

Same with the Unlex changes below which I'm guessing are related to this change here.

I understand all the .lower() changes in the diff, I think I understand the test, but I don't get why the rest is changing yet.

thakis requested changes to this revision.Dec 3 2020, 6:53 AM

(marking "request changes" just so it disappears form my dashboard and can reappear once you've answered -- doesn't necessarily need changes)

This revision now requires changes to proceed.Dec 3 2020, 6:53 AM
epastor edited the summary of this revision. (Show Details)Mar 15 2021, 2:08 PM
epastor updated this revision to Diff 330804.Mar 15 2021, 2:12 PM

Improve comments to clarify the change

llvm/lib/MC/MCParser/MasmParser.cpp
1549

I've added some comments in a new version of the change. Quick summary: this location doesn't change how things are handled - we already tried looking up a Symbol, now we just canonicalize the casing if it happens to be a Variable name.

Also, the UnLex changes below fix error handling for textmacro expansion, and were required to get the test working cleanly. I've updated the patch summary to mention those.

epastor updated this revision to Diff 330807.Mar 15 2021, 2:14 PM

Rebase on HEAD

thakis accepted this revision.Apr 2 2021, 8:13 AM
This revision is now accepted and ready to land.Apr 2 2021, 8:13 AM
epastor updated this revision to Diff 334981.Apr 2 2021, 10:02 AM

Rebase to HEAD

This revision was landed with ongoing or failed builds.Apr 2 2021, 11:08 AM
This revision was automatically updated to reflect the committed changes.