This is an archive of the discontinued LLVM Phabricator instance.

[ms] [llvm-ml] Support built-in text macros
ClosedPublic

Authored by epastor on Jun 25 2021, 8:07 PM.

Details

Summary

Add support for all built-in text macros supported by ML64:
@Date, @Time, @FileName, @FileCur, and @CurSeg.

Diff Detail

Event Timeline

epastor created this revision.Jun 25 2021, 8:07 PM
epastor requested review of this revision.Jun 25 2021, 8:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2021, 8:07 PM
epastor updated this revision to Diff 356227.Jul 2 2021, 11:15 AM

Rebase on parent

epastor updated this revision to Diff 356238.Jul 2 2021, 11:50 AM

Rebase on parent

thakis added inline comments.Jul 5 2021, 7:42 PM
llvm/lib/MC/MCParser/MasmParser.cpp
1184

Why do you do both lookups? Inserting into Variables with a built-in name errors, so we know that if it's in BuiltinSymbolMap, it can't be in Variables. So do

auto BuiltinIt = ...
if (BuiltinIt != end()) {
  ...
} else {
  auto VarIt = ...;
  if (VarIt != end() ... )
    ...
}
3660

Like above, looking up both in this loop looks weird to me. I'd structure this like

while (true) {
  // Lookup builtinText, check if not end, handle builtin case, continue
  // Same for variables
  break;
}
7712

Please add a flag to set the the timestamp on the commandline, for deterministic build reasons. lld-link has /timestamp:, so I guess let's add that here too. (And then set TT to the arg of /timestamp: if it's passed and to time(nullptr) else)

7713

Hm, this makes this function non-thread-safe. I think that's fine, but maybe add a // Not thread-safe at the end of the line to make that explicit.

7716

Instead of lines 7701-7705, do this:

char TmpBuffer[sizeof("mm/dd/yy")];
strftime(TmpBuffer, sizeof(TmpBuffer), "%D", TM);
return TmpBuffer;

Don't even need a dedicated variable for TM then.

7721

24-hour clock I guess? Not clear from the comment.

7722

/timestamp: support there too

7724

similar to above:

char TmpBuffer[sizeof("HH:MM:SS")];
strftime(TmpBuffer, sizeof(TmpBuffer), "%T", TM);
return TmpBuffer;
7737

sys::path::stem() already extracts the filename, no need to call sys::path::filename() too. This is not supposed to include the extension, yes?

7740

Do we have to do the upper()?

llvm/test/tools/llvm-ml/builtin_symbols.asm
44

Instead of the previous two lines, you could do CHECK: FileCur = {{.*}}builtin_symbols_t5.inc Could even use {{.*[\\/]}} to check that there's a directory separator in front of builtin_symbols

54

with /timestamp: you could put actual numbers here :)

epastor updated this revision to Diff 358023.Jul 12 2021, 12:14 PM
epastor marked 11 inline comments as done.

Add -timestamp and -utc flags to support deterministic builds

epastor marked an inline comment as done.Jul 12 2021, 12:16 PM
epastor added inline comments.
llvm/lib/MC/MCParser/MasmParser.cpp
1184

There seemed to be a lot of work going on in LLVM to avoid excessively-nested if statements, so I thought this was a worthwhile tradeoff. Thanks for correcting! Fixed.

7712

I was adding that in a followup commit, but I've merged it in here now.

7713

Switched to localtime_r; I don't think localtime has any real advantage here.

7721

Clarified.

7737

Interesting, I didn't realize stem did both. Fixed!

7740

If we want to match ML.EXE, yes - it always uses an upper-cased form of the stem for the @Filename built-in.

llvm/test/tools/llvm-ml/builtin_symbols.asm
44

Good point; cleaned up.

54

I had to add another flag for this, because MASM canonically uses the machine's local timezone for its built-ins; -utc now forces the use of GMT for the timezone.

epastor updated this revision to Diff 358036.Jul 12 2021, 12:42 PM
epastor marked an inline comment as done.

Rebase on parent

thakis accepted this revision.Jul 14 2021, 12:17 PM

Looks good now except for below.

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

Is that available on all platforms? If so, ok.

Does "%D" for format string not work?

7719

Does "%T" not work as format string?

This revision is now accepted and ready to land.Jul 14 2021, 12:17 PM
epastor updated this revision to Diff 360444.Jul 21 2021, 7:14 AM

Switch back to gmtime/localtime (not thread-safe, but in a generally-safe location - and more cross-platform)

Also address strftime comments.

epastor marked 2 inline comments as done.Jul 21 2021, 7:14 AM
This revision was landed with ongoing or failed builds.Jul 21 2021, 8:46 AM
This revision was automatically updated to reflect the committed changes.