This is an archive of the discontinued LLVM Phabricator instance.

[Object] object::ELFObjectFile::dynamic_symbol_begin(): skip symbol index 0
ClosedPublic

Authored by Higuoxing on Mar 12 2020, 11:16 AM.

Details

Summary

Note: This revision is very similar to D62296.

In D75756, we need getDynamicSymbolIterators() to skip first NULL symbol in .dynsym. And I believe it might be worth pointing this out in a separate patch to gather you experts' opinions.

I have checked that current code base will not be affected by this change.

dynamic_symbol_begin()
|- dynamic_symbol_end(): Ok
`- getDynamicSymbolIterators()
   |- addDynamicElfSymbols(): llvm/tools/llvm-objdump/llvm-objdump.cpp, Line 934
   |                          Ok, NULL symbol will be omitted by Line 945-947
   |                          StringRef Name = unwrapOrError(Symbol.getName(), Obj->getName());
   |                          if (Name.empty()) continue;
   |- dumpSymbolNameFromObject(): llvm/tools/llvm-nm/llvm-nm.cpp, Line 1192
   |                          There's no test for dumping dynamic debugging symbol. This patch helps improve llvm-nm behavior. (we should add test for this later)
   `- computeSymbolSizes(): llvm/lib/Object/SymbolSize.cpp, Line 52
      |- OProfileJITEventListener::notifyObjectLoaded(): llvm/lib/ExecutionEngine/OProfileJIT/OProfileJITEventListener.cpp, Line 92
      |                                                  Ok, NULL symbol will be omitted by Line 94-95
      |                                                  if (!Sym.getType() || *Sym.getType() != SF_Function) continue;
      |- IntelJITEventListener::notifyObjectLoaded(): llvm/lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp, Line 98
      |                                               Ok, NULL symbol will be omitted by Line 124-126 (same as previous one)
      |- PerfJITEventListener::notifyObjectLoaded(): llvm/lib/ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp, Line 244
      |                                              Ok, NULL symbol will be omitted by Line 254-256, (same as previous one)
      |- SymbolizableObjectFile::create(): llvm/lib/DebugInfo/Symbolize/SymbolizableObjectFile.cpp, Line 73
      |                                    Ok, NULL symbol will be omitted by Line 75
      |                                    res->addSymbol()
      |                                    In addSymbol(), Line 167-168
      |                                    if (!Sec || (Obj && Obj->section_end() == *Sec)) return std::error_code();
      |- dumpCXXData(): llvm/tools/llvm-cxxdump/llvm-cxxdump.cpp, Line 189
      |                 Ok, NULL symbol will be omitted by Line 199-202
      |                 object::section_iterator SecI = *SecIOrErr;
      |                 // Skip external symbols.
      |                 if (SecI == Obj->section_end())
      |                   continue;
      `- printLineInfoForInput(): llvm/tools/llvm-rtdyld/llvm-rtdyld.cpp, Line 418
                                  Ok, NULL symbol will be omitted by Line 430-477
                                  if (Type == object::SymbolRef::ST_Function) {
                                    ...
                                  }

Diff Detail

Event Timeline

Higuoxing created this revision.Mar 12 2020, 11:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2020, 11:17 AM
MaskRay accepted this revision.Mar 12 2020, 11:30 AM
This revision is now accepted and ready to land.Mar 12 2020, 11:30 AM
grimar added inline comments.Mar 13 2020, 2:38 AM
llvm/include/llvm/Object/ELFObjectFile.h
1031

Ok. So seems we are going to make this function to ignore the zero symbol.

Code on the left did not work with sh_size, you are adding the logic that uses it,
but what if sh_size is broken? Here, sh_size % sizeof(Elf_Sym) != 0
means we have a corrupted section isn't?

I'd suggest to isolate error handling. For now we can probably do something like:

// A comment saying we ignore this errors currently.
if (!DotDynSymSec->sh_size || DotDynSymSec->sh_size % sizeof(Elf_Sym))
 return dynamic_symbol_end();
// A comment about what we do.
return {SymbolRef(toDRI(DotDynSymSec, 1), this};

Do we have a test where this method is used when sh_size % sizeof(Elf_Sym) != 0?

Higuoxing marked an inline comment as done.

Addressed comment.

  • Ignore malformed .dynsym section.
Higuoxing updated this revision to Diff 251033.Mar 18 2020, 4:26 AM
Higuoxing marked an inline comment as done.

Update comments.

This need test cases I think.

llvm/include/llvm/Object/ELFObjectFile.h
1031

You do not need curly bracers and else after return. See the sample I've suggested above.

Do we have a test where this method is used when sh_size % sizeof(Elf_Sym) != 0?

This question is unanswered.

This need test cases I think.

Seems that we don't have any test on sh_size % sizeof(Elf_Sym) != 0. Do you have any suggestion on crafting such test?
There are 3 tools using this method: llvm-nm, llvm-objdump, llvm-cxxdump. I think we could write unit test for this in llvm-nm or llvm-objdump folder. What's your opinion?

grimar added a comment.EditedMar 19 2020, 5:45 AM

This need test cases I think.

Seems that we don't have any test on sh_size % sizeof(Elf_Sym) != 0. Do you have any suggestion on crafting such test?

You should be able to use yaml2obj tool, see its test suite for samples (llvm\test\tools\yaml2obj\ELF).

There are 3 tools using this method: llvm-nm, llvm-objdump, llvm-cxxdump. I think we could write unit test for this in llvm-nm or llvm-objdump folder. What's your opinion?

I guess it could be a c++ unit test, like we have in llvm\unittests\Object, I am not sure we might want to create objects from YAML there though, like ObjectYAMLTests does.
So, probably, you can just select and test llvm-objdump.

Given the condition you have:

if (!DotDynSymSec || !DotDynSymSec->sh_size ||
    DotDynSymSec->sh_size % sizeof(Elf_Sym) != 0) {

I think there should be 3 sub-tests: no dynamic section case, sh_size == 0 and sh_size % sizeof(Elf_Sym) != 0.

Higuoxing updated this revision to Diff 251886.Mar 22 2020, 7:15 AM
  • Add some test cases.

Sorry, I forget that llvm-objdump cannot dump dynamic symbols until D75756 lands which depends on this revision. Hence I crafeted these tests based on llvm-nm. Can we use these tests as --dynamic dedicated tests as well (@jhenderson metioned here)?

Add one more test.

Can we use these tests as --dynamic dedicated tests as well (@jhenderson metioned here)?

I think so.

llvm/test/tools/llvm-nm/dynamic-symbols.test
1 ↗(On Diff #251954)

I guess @jhenderson will not be happy to see "good ELF file".
I am not sure it is "good". You should just probably say that
we test how llvm-nm dump dynamic symbols from a valud dynamic section.

3 ↗(On Diff #251954)

Why do you need --debug-syms?

11 ↗(On Diff #251954)

We usually format such pieces a bit differently:

#       DYNSYM:                 U globalsym
#  DYNSYM-NEXT:                 U localsym1
#  DYNSYM-NEXT:0000000000000000 n localsym2
# DYNSYM-EMPTY:
30 ↗(On Diff #251954)

I'd probably group locals together.

39 ↗(On Diff #251954)

Instead of {{.*}} you probably want to define and check FILE, like we do in many other tests.
E.g.:

# RUN: llvm-readobj --dyn-relocations %t1 2>&1 | FileCheck %s -DFILE=%t1 --check-prefix=LLVM
....
# LLVM:      Dynamic Relocations {
# LLVM-NEXT: warning: '[[FILE]]': unable to get name of the dynamic symbol with index 1: st_name (0x1234) is past the end of the string table of size 0x1
53 ↗(On Diff #251954)

I think it is enough to verify that -D == --debug-syms once at the begining. No need to re-test -D everywhere.
(See how we test aliases in llvm-readelf, for example).

79 ↗(On Diff #251954)

What is the difference with NODYNSYM and EMPTY? You should be able to use a single check tag.

90 ↗(On Diff #251954)

I think you should test 32-bits case too. Looks you can just set Size to 1 and adjust this YAML to use -D BITS=32/64,
see llvm-readobj\ELF\hash-histogram.test for sample.

91 ↗(On Diff #251954)

Doesn't seem you need Content? It should be enough either to have Size or Content I believe.

Higuoxing updated this revision to Diff 252204.Mar 23 2020, 7:58 PM

Addressed comments.

  • Good ELF file -> ELF file with valid .dynsym section.
  • Remove nonsense --debug-syms option.
  • Format check tags.
  • Group local symbols in YAML file.
  • Replace {{.*}} by [[FILE]].
  • Merge check tags with similar content.
  • Add ELF 32-bit test

Thanks a lot for taking time reviewing :)

jhenderson added inline comments.Mar 24 2020, 2:15 AM
llvm/include/llvm/Object/ELFObjectFile.h
1032

I'd suggest the following comment instead:

"Ignore errors here where the dynsym is empty or does not have a size that is a multiple of the symbol size. These should be handled elsewhere."

1041

This should have a comment. Perhaps "Do not allow iteration over a dynamic symbol table with a malformed size."

llvm/test/tools/llvm-nm/dynamic-symbols.test
1 ↗(On Diff #252204)

As the switch name is --dynamic, let's rename this test to dynamic.test.

4 ↗(On Diff #252204)

Indent continuation lines like this slightly to make it clear that they are continuations. I find it helps readability of the test:

# RUN: llvm-nm --dynamic %t1.o | \
# RUN:   FileCheck %s --match-full-lines --strict-whitespace --check-prefix DYNSYM

Same goes throughout this test.

60–61 ↗(On Diff #252204)

What does GNU nm do in this situation?

Almost there.
I also think that comments in the test perhaps need a bit of polishing, but I am not a native speaker to suggest much better versions, so will refrain.

llvm/include/llvm/Object/ELFObjectFile.h
1034

You do not need curly bracers and else after return. See the sample I've suggested above

This wasn't addressed.

llvm/test/tools/llvm-nm/dynamic-symbols.test
1 ↗(On Diff #252204)

I'd add a general comment about the whole test at the begining:

"## This is a test for --dynamic/-D option."

30 ↗(On Diff #252204)

Normally, local symbols go before globals in a ELF object.
And you do not need to specify "Binding: STB_LOCAL" because STB_LOCAL is the default binding for a symbol,
used when "Binding" property is not provided.

35 ↗(On Diff #252204)

I'd expect to see EMPTY check lines right below this RUN lines, not at the end of file.
(that is what we normally do).

82 ↗(On Diff #252204)

See. You have a tag named "EMPTY" now. But it is used to test 3 cases:

  1. When there is no .dynsym section.
  2. When .dynsym section is empty.
  3. When .dynsym section is broken.

EMPTY is a valid name only for (2). Hence, should be renamed. Probably NO-SYMS?

grimar added inline comments.Mar 24 2020, 2:23 AM
llvm/include/llvm/Object/ELFObjectFile.h
1034

(the part about "else after return')

Higuoxing marked an inline comment as done.Mar 24 2020, 3:08 AM
Higuoxing added inline comments.
llvm/test/tools/llvm-nm/dynamic-symbols.test
60–61 ↗(On Diff #252204)

GNU nm will dump the part sh_size / sizeof(Elf_Sym) rather than drop them all.

I could revert the change in dynamic_symbol_end() to imitate this behavior.

jhenderson added inline comments.Mar 24 2020, 3:19 AM
llvm/test/tools/llvm-nm/dynamic-symbols.test
60–61 ↗(On Diff #252204)

I think we should imitate GNU (and show that we are for this case with a slightly modified test case).

Higuoxing updated this revision to Diff 252287.EditedMar 24 2020, 5:47 AM

Addressed comments.

  • Rename test case to dynamic.test.
  • Add genral comment for whole test.
  • Indent continuous lines in test.
  • Group local symbols, and remove nonsense Binding for local symbol.
  • Move check tags right below RUN instruction.
  • Rename EMPTY check tag to NO-SYMS

  • Make llvm-nm imitate gnu-nm's behavior.

To make llvm-nm behave like gnu-nm, I have to remove sh_size % sizeof(Elf_Sym) check in dynamic_symbol_begin(), I'm wondering if @grimar is happy with this, since you are concerned about this.

  • Make llvm-nm imitate gnu-nm's behavior.

To make llvm-nm behave like gnu-nm, I have to remove sh_size % sizeof(Elf_Sym) check in dynamic_symbol_begin(), I'm wondering if @grimar is happy with this, since you are concerned about this.

Probably I am not, because this is the patch for Object and not for llvm-nm. What is good for llvm-nm might not work for other tools.
When sh_size % sizeof(Elf_Sym) != 0 it means we have a error.
I guess such things should be fixed on llvm-nm side and not in libObject. We can probably adjust this later though as this patch is still an improvement.

@jhenderson, what do you think?

  • Make llvm-nm imitate gnu-nm's behavior.

To make llvm-nm behave like gnu-nm, I have to remove sh_size % sizeof(Elf_Sym) check in dynamic_symbol_begin(), I'm wondering if @grimar is happy with this, since you are concerned about this.

Probably I am not, because this is the patch for Object and not for llvm-nm. What is good for llvm-nm might not work for other tools.
When sh_size % sizeof(Elf_Sym) != 0 it means we have a error.
I guess such things should be fixed on llvm-nm side and not in libObject. We can probably adjust this later though as this patch is still an improvement.

@jhenderson, what do you think?

Thanks a lot. IMHO, we could let dynamic_symbol_begin() return these symbols, and do error checking/handling in clients. Or, it's not very easy to regain these symbols in client side (Feel free to ignore this)?

I do not have a good answer right now. Seems the API could be revisited and refined.
section_begin() and section_end() just ignore errors now.
symbol_begin() has a different logic from dynamic_symbol_begin that does not look ideal to me either:

DataRefImpl Sym =
    toDRI(DotSymtabSec,
          DotSymtabSec && DotSymtabSec->sh_size >= sizeof(Elf_Sym) ? 1 : 0);

(it is unclear to me how it works when sh_size==1, for example)

I think we should investigate how API is used by each tool, find what they want
and then decide how to clean-up/improve/expand/remove it.

I do not have a good answer right now. Seems the API could be revisited and refined.
section_begin() and section_end() just ignore errors now.
symbol_begin() has a different logic from dynamic_symbol_begin that does not look ideal to me either:

DataRefImpl Sym =
    toDRI(DotSymtabSec,
          DotSymtabSec && DotSymtabSec->sh_size >= sizeof(Elf_Sym) ? 1 : 0);

(it is unclear to me how it works when sh_size==1, for example)

When sh_size == 1, it's similar to toDRI(DotSymtabSec, 0) and symbol_end() will return toDRI(DotSymtabSec, sh_size / sizeof(Elf_Sym). Hence, no symbol is returned.

I think we should investigate how API is used by each tool, find what they want
and then decide how to clean-up/improve/expand/remove it.

Sure, I will take a look at those tools. Thanks :)

I do not have a good answer right now. Seems the API could be revisited and refined.
section_begin() and section_end() just ignore errors now.
symbol_begin() has a different logic from dynamic_symbol_begin that does not look ideal to me either:

DataRefImpl Sym =
    toDRI(DotSymtabSec,
          DotSymtabSec && DotSymtabSec->sh_size >= sizeof(Elf_Sym) ? 1 : 0);

(it is unclear to me how it works when sh_size==1, for example)

When sh_size == 1, it's similar to toDRI(DotSymtabSec, 0) and symbol_end() will return toDRI(DotSymtabSec, sh_size / sizeof(Elf_Sym). Hence, no symbol is returned.

Exactly. It is unclear how well it works for callers now. It seems just hides errors and I would expect to see issues on their side related to this API.
A few methods around return Expected<>, i.e. they do not ignore errors. Perhaps we should think about returning Expected<elf_symbol_iterator> here too.
BTW, getRelocatedSection do this:

Expected<section_iterator>
ELFObjectFile<ELFT>::getRelocatedSection(DataRefImpl Sec) const

while section_begin() don't:

section_iterator ELFObjectFile<ELFT>::section_begin() const

We might want to change the iterators to use the fallible iterator pattern described in the programmer's manual. That should ensure that all clients can cope with bad iteration somehow. I'm not sure I have any particular further insight into how to proceed here beyond that. I think imitating GNU's behaviour for now is a good start, and further improvements can be in later patches.

llvm/test/tools/llvm-nm/dynamic.test
34

"... without a .dynsym section"

66

doesn't match dynamic symbol entries -> isn't a maultiple of the dynamic symbol size

67

as much symbol -> as many symbols
(sh_size / sizeof(Elf_Sym) is missing a closing ')'

68–70

RU -> RUN

Higuoxing updated this revision to Diff 253335.Mar 28 2020, 5:58 AM

Addressed comments.

Hi @grimar, the build log of this patch shows that we actually have a checker that complains sh_size isn't a multiple of sh_entsize (llvm/include/Object/ELF.h: Line 401). But I have no idea why it wasn't triggered on my machine. I use a release build because I do not have enough memory for debug build ...

Ahhhh, After enable -DLLVM_ENABLE_ASSERTIONS=ON,

there's an error: section [index 2] has an invalid sh_size (33) which is not a multiple of its sh_entsize (16)Stack dump. But this error won't show up in a release build. I think we should at least warn user about this?

Ahhhh, After enable -DLLVM_ENABLE_ASSERTIONS=ON,

there's an error: section [index 2] has an invalid sh_size (33) which is not a multiple of its sh_entsize (16)Stack dump. But this error won't show up in a release build. I think we should at least warn user about this?

Where abouts does this message come from?

jhenderson added a comment.EditedMar 30 2020, 12:14 AM

Ahhhh, After enable -DLLVM_ENABLE_ASSERTIONS=ON,

there's an error: section [index 2] has an invalid sh_size (33) which is not a multiple of its sh_entsize (16)Stack dump. But this error won't show up in a release build. I think we should at least warn user about this?

Where abouts does this message come from?

Sorry, didn't see your comment earlier. I assume this is coming from the final test case? If so, that definitely should be a public diagnostic message if we can figure out a way of getting it to llvm-nm.

llvm/test/tools/llvm-nm/dynamic.test
49

with an empty

65

with a malformed

74

I don't think you need --strict-whitespace here. Without it you'll also be able to have only one check-prefix shared between the two cases.

Higuoxing updated this revision to Diff 254515.Apr 2 2020, 7:11 AM

Correct grammatical mistakes. I will try my best to avoid these mistakes next time ...

grimar added a comment.Apr 2 2020, 8:00 AM

Hi @grimar, the build log of this patch shows that we actually have a checker that complains sh_size isn't a multiple of sh_entsize (llvm/include/Object/ELF.h: Line 401). But I have no idea why it wasn't triggered on my machine. I use a release build because I do not have enough memory for debug build ...

I've missed this question/thread somehow, sorry.

Ahhhh, After enable -DLLVM_ENABLE_ASSERTIONS=ON,

there's an error: section [index 2] has an invalid sh_size (33) which is not a multiple of its sh_entsize (16)Stack dump. But this error won't show up in a release build. I think we should at least warn user about this?

Where abouts does this message come from?

I do not think that LLVM_ENABLE_ASSERTIONS should affect on whether we print or not something like this message..

@Higuoxing, if you still have problems with it, please clarify how to reproduce the misbehavior you observe (e.g.. which yaml you use).

Hi @grimar, the build log of this patch shows that we actually have a checker that complains sh_size isn't a multiple of sh_entsize (llvm/include/Object/ELF.h: Line 401). But I have no idea why it wasn't triggered on my machine. I use a release build because I do not have enough memory for debug build ...

I've missed this question/thread somehow, sorry.

Ahhhh, After enable -DLLVM_ENABLE_ASSERTIONS=ON,

there's an error: section [index 2] has an invalid sh_size (33) which is not a multiple of its sh_entsize (16)Stack dump. But this error won't show up in a release build. I think we should at least warn user about this?

Where abouts does this message come from?

I do not think that LLVM_ENABLE_ASSERTIONS should affect on whether we print or not something like this message..

@Higuoxing, if you still have problems with it, please clarify how to reproduce the misbehavior you observe (e.g.. which yaml you use).

Ah, thank you @grimar, this should be fixed in D77289. But the error was consumed, we still need to do some improvements to warn user about it.

grimar added a comment.Apr 2 2020, 8:23 AM

Ok. So we have D77289 and this one connected, I'd start from landing this one first.
(I have no comments regarding this one)

Ok. So we have D77289 and this one connected, I'd start from landing this one first.
(I have no comments regarding this one)

I guess we should land D77289 first? Or we cannot pass the last test case.

## Test llvm-nm dumping ELF file with a malformed .dynsym section header
## whose sh_size isn't a multiple of the dynamic symbol size (sh_size % sizeof(Elf_Sym) != 0).
jhenderson accepted this revision.Apr 3 2020, 1:12 AM

Correct grammatical mistakes. I will try my best to avoid these mistakes next time ...

Don't worry about it (obviously do your best, but don't spend hours agonising about a comment's grammar - it's what I'm here for!).

LGTM. I think landing D77289 first might make slightly more sense, but I don't have a strong opinion.

grimar added a comment.Apr 3 2020, 2:19 AM

I think landing D77289 first might make slightly more sense, but I don't have a strong opinion.

+1. If the problem only with the last test case, I think it could be splitted to another follow-up patch.

Higuoxing updated this revision to Diff 255018.Apr 4 2020, 3:22 AM

Remove last test case. We will add it in a follow up patch.

This revision was automatically updated to reflect the committed changes.