[PATCH] D104965: [ms] [llvm-ml] Support built-in text macros

[Eric Astor via Phabricator via llvm-commits]

epastor marked an inline comment as done.
epastor added inline comments.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:1180
+  auto BuiltinIt = BuiltinSymbolMap.find(IDLower);
+  auto VarIt = Variables.find(IDLower);
+  if (BuiltinIt != BuiltinSymbolMap.end()) {
----------------
thakis wrote:
> 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() ... )
>     ...
> }
> ```
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.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7699
+    // Current local date, formatted MM/DD/YY
+    time_t TT = time(nullptr);
+    struct tm *TM = localtime(&TT);
----------------
thakis wrote:
> 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)
I was adding that in a followup commit, but I've merged it in here now.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7700
+    time_t TT = time(nullptr);
+    struct tm *TM = localtime(&TT);
+    SmallString<32> TmpBuffer;
----------------
thakis wrote:
> 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.
Switched to `localtime_r`; I don't think `localtime` has any real advantage here.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7708
+  case BI_TIME: {
+    // Current local time, formatted HH:MM:SS
+    time_t TT = time(nullptr);
----------------
thakis wrote:
> 24-hour clock I guess? Not clear from the comment.
Clarified.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7724
+  case BI_FILENAME:
+    return sys::path::stem(sys::path::filename(
+                               SrcMgr.getMemoryBuffer(SrcMgr.getMainFileID())
----------------
thakis wrote:
> sys::path::stem() already extracts the filename, no need to call sys::path::filename() too. This is not supposed to include the extension, yes?
Interesting, I didn't realize stem did both. Fixed!


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7727
+                                   ->getBufferIdentifier()))
+        .upper();
+  case BI_CURSEG:
----------------
thakis wrote:
> Do we have to do the upper()?
If we want to match ML.EXE, yes - it always uses an upper-cased form of the stem for the `@Filename` built-in.


================
Comment at: llvm/test/tools/llvm-ml/builtin_symbols.asm:43
+; CHECK: FileCur =
+; CHECK-SAME: builtin_symbols_t5.inc
+; CHECK: FileName =
----------------
thakis wrote:
> 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`
Good point; cleaned up.


================
Comment at: llvm/test/tools/llvm-ml/builtin_symbols.asm:53
+; CHECK-LABEL: t6:
+; CHECK: Date = {{([[:digit:]]{2}/[[:digit:]]{2}/[[:digit:]]{2})}}
+; CHECK-NOT: {{[[:digit:]]}}
----------------
thakis wrote:
> with `/timestamp:` you could put actual numbers here :)
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104965/new/

https://reviews.llvm.org/D104965