Page MenuHomePhabricator

demangle xcoff label symbol for llvm-nm
ClosedPublic

Authored by DiggerLin on Nov 3 2021, 7:56 AM.

Details

Summary

modified the demangle for the xcoff label symbol.

Diff Detail

Event Timeline

DiggerLin created this revision.Nov 3 2021, 7:56 AM
DiggerLin requested review of this revision.Nov 3 2021, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2021, 7:56 AM

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;
    }
623

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).

632

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

DiggerLin updated this revision to Diff 384809.Nov 4 2021, 10:47 AM

address comment

jhenderson added inline comments.Nov 5 2021, 3:01 AM
llvm/test/tools/llvm-nm/XCOFF/demangle.test
2
llvm/tools/llvm-nm/llvm-nm.cpp
636

Nit: delete blank line at start of function.

637

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).

741–746

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 marked 5 inline comments as done.Nov 5 2021, 12:14 PM
DiggerLin added inline comments.
llvm/tools/llvm-nm/llvm-nm.cpp
637

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 updated this revision to Diff 385148.Nov 5 2021, 12:16 PM
DiggerLin marked an inline comment as done.

address comment

jhenderson added inline comments.Nov 8 2021, 12:21 AM
llvm/tools/llvm-nm/llvm-nm.cpp
636

I still see a blank line here at line 636.

642

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

645

Don't use else after return.

DiggerLin updated this revision to Diff 385604.Nov 8 2021, 12:57 PM

address comment

DiggerLin updated this revision to Diff 385661.Nov 8 2021, 5:07 PM
DiggerLin marked 3 inline comments as done.
jhenderson added inline comments.Nov 9 2021, 1:02 AM
llvm/tools/llvm-nm/llvm-nm.cpp
637

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 ""

add a empty symbol name test.

jhenderson added inline comments.Nov 22 2021, 12:40 AM
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 marked 5 inline comments as done.Dec 7 2021, 12:44 PM
DiggerLin added inline comments.
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 updated this revision to Diff 392505.Dec 7 2021, 12:47 PM
DiggerLin marked 2 inline comments as done.
DiggerLin added inline comments.Dec 7 2021, 12:50 PM
llvm/test/tools/llvm-nm/XCOFF/demangle.test
21

sorry , It should be

  1. 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 added inline comments.Dec 10 2021, 1:01 AM
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.

DiggerLin updated this revision to Diff 394887.Dec 16 2021, 8:15 AM
DiggerLin marked 3 inline comments as done.
jhenderson accepted this revision.Dec 17 2021, 12:37 AM

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
636–639

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

This revision is now accepted and ready to land.Dec 17 2021, 12:37 AM
This revision was landed with ongoing or failed builds.Jan 12 2022, 9:28 AM
This revision was automatically updated to reflect the committed changes.