[PATCH] D63327: [docs][llvm-nm] Improve symbol code documentation

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 6 inline comments as done.
jhenderson added inline comments.


================
Comment at: docs/CommandGuide/llvm-nm.rst:31
 
- Named object is referenced but undefined in this bitcode file
+a, A
+
----------------
rupprecht wrote:
> Do we print lowercase `a`? `man nm` doesn't include it.
Yes, and indeed GNU nm prints it too:


```
// bar.s
foo = 1234

PS C:\Work\TempWork> clang -c bar.s
PS C:\Work\TempWork> nm bar.o
00000000000004d2 a foo
PS C:\Work\TempWork> \llvm\build\Debug\bin\llvm-nm bar.o
00000000000004d2 a foo
```


================
Comment at: docs/CommandGuide/llvm-nm.rst:51
+
+n
+
----------------
MaskRay wrote:
> I think the semantics (GNU nm behavior) of `n` are similar to what the reverted rL359312 did, but unfortunately I cannot find a reasonable use case to write a test, nor can I find useful tests in the binutils-gdb repository.
> 
> I don't know if relocations into non-SHF_ALLOC sections other than .debug* are reasonable.
> I don't know if relocations into non-SHF_ALLOC sections other than .debug* are reasonable.

We use them in Sony-specific sections at least. I think they are reasonable for additional metadata that needs to refer to the actual code in some way.

I'm okay with the semantics of 'n' changing in the future (this is just one example where the code doesn't necessarily make sense). I'd just like to document what we currently do now - we can document what the behaviour is when we change the code.


================
Comment at: docs/CommandGuide/llvm-nm.rst:65
 
-t
+ Mach-O: absolute symbol or symbol from an unknown section.
 
----------------
mtrent wrote:
> I am not sure about the "Absolute Symbol" note. Can you point me at why you think that's what it means?
> 
> The historical documentation for 's' is "A symbol in a section that is not in __TEXT, __DATA (non-BSS), or in __DATA (BSS). I'd hate to call that "unknown" because "undefined" is such a strong concept and they start with  the same letter and are easy to confuse. Pragmatically, you can use directives to tell clang to write symbols into any arbitrary segment/section; and symbols defined in such a way would be listed as "s". So it's not that they're "unknown" as they are "uncommon" or perhaps "uncanonical." For the sake of  documentation, it might be best to say "Mach-O: symbol not in __TEXT or __DATA. 
> I am not sure about the "Absolute Symbol" note. Can you point me at why you think that's what it means?

See: https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-nm/llvm-nm.cpp#L1005. If the N_Type is N_ABS, the code returns 's'.

I'll update the text slightly to explicitly refer to the section names.


================
Comment at: docs/CommandGuide/llvm-nm.rst:69
 
-T
+ Read-only data object.
 
----------------
mtrent wrote:
> Hmm ... I don't *think* Mach-O will print this value. But, please let me know if you know otherwise, and thanks for bringing this to my attention.
It doesn't. As far as I can see, it's COFF and ELF-specific. I'm not sure whether we want to indicate where something isn't supported by Mach-O only. Happy for feedback on this.


================
Comment at: docs/CommandGuide/llvm-nm.rst:97
 
- Local data object
+ Weak symbol definition. Multiple definitions link together into zero or one definitions.
 
----------------
mtrent wrote:
> I believe we do not print w/W for Mach-O files. If you know otherwise, please let me know.
llvm-nm doesn't use 'w' for Mach-O. See above for my comments about whether we should explicitly state this.


================
Comment at: docs/CommandGuide/llvm-nm.rst:101
 
- Global data object
+ Mach-O: N_STAB symbol.
 
----------------
mtrent wrote:
> Yes, that's what the documentation says ... I've been looking for an example ... 
It's also [[ https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-nm/llvm-nm.cpp#L1000 | what the code says ]]. I have no idea what an N_STAB symbol actually is though. Happy for something better here (or even to delete it entirely).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63327