demangle xcoff label symbol for llvm-nm

[DiggerLin]

Summary

modified the demangle for the xcoff label symbol.

[jhenderson]

Similar to the comment on the size patch, I think you want a drastically simpler test, which has just enough symbols to exercise the new demangling code (e.g. something that doesn't get demangled, something that has a dot, something that doesn't have a dot, and something that has an empty name). At the moment, it is hard to spot what's actually important in the test.

llvm/tools/llvm-nm/llvm-nm.cpp

619–620

Rather than have all these boolean parameters to change the behaviour of the function, I think you probably want separate demangleMachO and demangleXCOFF functions. They would do the stuff specific to their format, before calling the common demangle function.

Rough outline:

static Optional<std::string> demangleMachO(StringRef Name) {
  // if name not empty, drop front of Name.
  return demangle(Name);
}

static Optional<std::string> demangleXCOFF(StringRef Name) {
  bool HasDot = /* ... */;
  // if HasDot and name not empty, drop front of Name.
  Optional<std::string> Demangled = demangle(Name);
  if (!Demangled)
    return None;
  return "." + *Demangled;
}

static Optional<std::string> demangle(StringRef Name) {
  // common stuff
}

// When calling...
    MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
    if (Demangle) {
      function_ref<Optional<std::string> DemangleFn;
      if (MachO)
        DemangleFn = demangleMachO;
      else if(dyn_cast<XCOFFObejctFile>(&Obj))
        DemangleFn = demangleXCOFF;
      else
        DemangleFn = demangle;
      if (Optional<std::string> Opt = DemangleFn(S.Name)
        Name = *Opt;
    }

621

S = "." + S; seems much clearer.

622

Consider using StringRef::drop_front instead, as it's a little more descriptive - probably only if you agree with my suggested refactor above (and then do it for the Mach-O case too).

[DiggerLin]

address comment

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/demangle.test

2

llvm/tools/llvm-nm/llvm-nm.cpp

628

Nit: delete blank line at start of function.

629

Can XCOFF symbols have an empty name? If so, you need a test case for this. If not, you don't need a Name.empty() check (but you might want some sort of assertion instead).

732–737

This is okay for now, but I think an llvm::function_ref would be better overall, as it allows in the future for other demangle options that take different argument sets. I also, personally, find the function_ref signature easier to read than a function pointer.

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

629

yes, XCOFF symbols can have empty name. it look the yamltoobj do not support a empty symbol Name now @Esme , I can not add the empty symbol name here now. if you not mind I use a binary object file here. I can add the test case.

[DiggerLin]

address comment

[jhenderson]

llvm/tools/llvm-nm/llvm-nm.cpp

628

I still see a blank line here at line 636.

634

Use a better variable name that describes what this represents, rather than describing its entity. Example: Demangled.

637

Don't use else after return.

[DiggerLin]

address comment

[jhenderson]

llvm/tools/llvm-nm/llvm-nm.cpp

629

See my comment in D112450: could you use assembly as your input? If so, I expect you could manage to produce a symbol with an empty name using something equivalent to:

"":
.global ""

[DiggerLin]

add a empty symbol name test.

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/demangle.test

18

I'd suggest doing one of the following:

1. adding --match-full-lines to your FileCheck command here
2. omitting the symbol value and adding {{$}} to the end of the symbol (omitting the symbol value removes noise from the test).

This will prevent this line accidentally matching another symbol with a name. It will also ensure the demangling output looks right on other lines.

21

This test is purely about demangling functionality. Aside from having a single test-case where a symbol isn't demangled, I don't think you need all these other cases that don't demangle.

Similarly, you can omit most of the test cases where demangling does occur, as long as you have one per code path.

I think the following set of test cases is all you should have - ignore the other symbols (or more preferably just rebase this patch on top of @Esme's yaml2obj work, so that you can use yaml2obj to generate exactly the right set of symbols):

1. Empty name
2. Name consisting solely of .
3. Name starting with . that can't be demangled
4. Name starting with . that can be demangled
5. Name not starting with . that can't be demangled
6. Name not starting with . that can be demangled

32

Why's this line here?

[DiggerLin]

llvm/test/tools/llvm-nm/XCOFF/demangle.test

18

there is no symbol value in the test . it is symbol address.

21

1. as I mentioned before, yaml2obj do not support an empty symbol.
2. I will reuse the test_xlclang.o which provided in the https://reviews.llvm.org/D112450 to test empty symbol
3. when yam22obj, I will create to delete the test and add the empty name to test demangle_sym_name.test

[DiggerLin]

llvm/test/tools/llvm-nm/XCOFF/demangle.test

21

sorry , It should be

3. when yam2obj support empty symbol, I will create a patch to delete the test and add the empty name to test demangle_sym_name.test

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/demangle.test

18

"Symbol value" and "symbol address" are usually synonyms in other file formats. llvm-nm by default dumps three columns, "symbol value/address" being the first, and is the one I was referring to.

21

as I mentioned before, yaml2obj do not support an empty symbol.

Then add support for it? I can't imagine it would be hard to do. Alternatively, does it fail if you explicitly do something like Name: ''? I believe that's how we provide empty names in cases in other formats.

llvm/test/tools/llvm-nm/XCOFF/demangle_sym_name.test

2 ↗

(On Diff #392505)

Simplify the name.

3 ↗

(On Diff #392505)

I'd recommend adding --match-full-lines here too, to show that the name demangling has worked properly, and doesn't print some trailing string/garbage after the demangled name.

[jhenderson]

LGTM, with two minor suggestions remaining.

llvm/test/tools/llvm-nm/XCOFF/demangle_sym_name.test

1 ↗

(On Diff #394887)

Nit: follow existing practice in the other tests in llvm-nm, but I prefer - instead of _ as separators, as they are easier to test, e.g. demangle-sym-name.test. Or you could just shorten it to demangle.test, since there's only one thing that is mangled (I assume?).

llvm/tools/llvm-nm/llvm-nm.cpp

628–631

I think I'd find this marginally easier to follow if it was flipped, and the else removed, as per the inline edit.

llvm/test/tools/llvm-nm/XCOFF/demangle.test

+## Test llvm-nm demangling of symbols for XCOFF object files.
+## Test following symbols:
+## 1. Empty name
+## 2. Name consisting solely of .
+## 3. Name starting with . that can't be demangled
+## 4. Name starting with . that can be demangled
+## 5. Name not starting with . that can't be demangled
+## 6. Name not starting with . that can be demangled
+
+# RUN: yaml2obj --docnum=1 %s -o %t.o
+# RUN: llvm-nm --demangle --format=just-symbols %t.o 2>&1 | FileCheck %s --check-prefix=NM-DEMANGLE --match-full-lines
+
+# NM-DEMANGLE:       {{$}}
+# NM-DEMANGLE-NEXT:  .
+# NM-DEMANGLE-NEXT:  ._5func1i
+# NM-DEMANGLE-NEXT:  .func1(int)
+# NM-DEMANGLE-NEXT:  L5func0v
+# NM-DEMANGLE-NEXT:  func0()
+
+--- !XCOFF
+FileHeader:
+  MagicNumber:       0x1DF
+Sections:
+  - Name:            .text
+    Flags:           [ STYP_TEXT ]
+  - Name:            .data
+    Flags:           [ STYP_DATA ]
+Symbols:
+  - Name:            ""
+    Section:         .text
+  - Name:            .
+    Section:         .text
+  - Name:            ._Z5func1i
+    Section:         .text
+  - Name:            ._5func1i
+    Section:         .text
+  - Name:            _ZL5func0v
+    Section:         .data
+  - Name:            L5func0v
+    Section:         .data

llvm/tools/llvm-nm/llvm-nm.cpp

static void darwinPrintStab(MachOObjectFile *MachO, const NMSymbol &S) {
   }
 
   outs() << format(" %02x %04x ", NSect, NDesc);
   if (const char *stabString = getDarwinStabString(NType))
     outs() << format("%5.5s", stabString);
   else
     outs() << format("   %02x", NType);
 }
 
-static Optional<std::string> demangle(const std::string &Name,
-                                      bool StripUnderscore) {
-  const char *Mangled = Name.c_str();
-  if (StripUnderscore && Mangled[0] == '_')
-    Mangled = Mangled + 1;
-
+static Optional<std::string> demangle(StringRef Name) {
   std::string Demangled;
-  if (nonMicrosoftDemangle(Mangled, Demangled))
+  if (nonMicrosoftDemangle(Name.str().c_str(), Demangled))
     return Demangled;
   return None;
 }
 
+static Optional<std::string> demangleXCOFF(StringRef Name) {
+  if (Name.empty() || Name[0] != '.')
+    return demangle(Name);
+
+  Name = Name.drop_front();
+  Optional<std::string> DemangledName = demangle(Name);
+  if (DemangledName)
+    return "." + *DemangledName;
+  return None;
+}
+
+static Optional<std::string> demangleMachO(StringRef Name) {
+  if (!Name.empty() && Name[0] == '_')
+    Name = Name.drop_front();
+  return demangle(Name);
+}
+
 static bool symbolIsDefined(const NMSymbol &Sym) {
   return Sym.TypeChar != 'U' && Sym.TypeChar != 'w' && Sym.TypeChar != 'v';
 }
 
 static void writeFileName(raw_ostream &S, StringRef ArchiveName,
                           StringRef ArchitectureName) {
   if (!ArchitectureName.empty())
     S << "(for architecture " << ArchitectureName << "):";
if (isSymbolList64Bit(Obj)) {
     }
   }
 
   for (const NMSymbol &S : SymbolList) {
     uint32_t SymFlags;
     std::string Name = S.Name;
     MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
     if (Demangle) {
-      if (Optional<std::string> Opt = demangle(S.Name, MachO))
+      function_ref<Optional<std::string>(StringRef)> Fn = ::demangle;
+      if (Obj.isXCOFF())
+        Fn = demangleXCOFF;
+      if (Obj.isMachO())
+        Fn = demangleMachO;
+      if (Optional<std::string> Opt = Fn(S.Name))
         Name = *Opt;
     }
     if (S.Sym.getRawDataRefImpl().p) {
       Expected<uint32_t> SymFlagsOrErr = S.Sym.getFlags();
       if (!SymFlagsOrErr) {
         // TODO: Test this error.
         error(SymFlagsOrErr.takeError(), Obj.getFileName());
         return;