[Lldb-commits] [PATCH] D68069: Explicitly set entry point arch when it's thumb

Thu Sep 26 17:50:26 PDT 2019

aadsm marked an inline comment as done.
aadsm added a comment.

> For the test, what would you say to writing that as a lit test instead (testing the address class deduction via the disassemble command you mentioned)?

I was actually keen on this since lit is the only type of test I haven't used yet but then thought that it wouldn't really test my change directly (just indirectly). I know I put that as a test in my summary but it was more like a sanity check. The real test here is checking the address class (which is what is changed in the code). There are different things in lldb that uses that like setting a breakpoint or disassembling instructions, that's why I don't feel that testing the consequence is the ideal test. What do you think?

> The yaml is actually fairly readable as is, but given that you felt the need to include the commands used to produce that input, it might be better to actually produce that file via the commands as a part of the test (substituting llvm-mc for as and ld.lld for linker).

I just put it there for completion sake, I always like to have the source of things when I didn't do it by hand. In this case I prefer to have the yaml because I'm not sure if in all scenarios that we run the test we'll be able to assemble arm assembly into an object?

================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2737-2738
+              0,          // Offset in section or symbol value.
+              2,          // Size.
+              true,       // Size is valid.
+              false,      // Contains linker annotations?
----------------
labath wrote:
> This is pretty arbitrary. Would it be possible to just set size_is_valid=false, and let the usual symbol size deduction logic figure out something reasonable here?
To be honest I'm not sure how the size_is_valid = false is used as I've only seen it being used when going through the EH frame FDE entries.

Setting the size to 0 is problematic though, when the symbol is added to the symtab its size will automatically span to the next function symbol boundary. I think this can be dangerous because the symtab for the object file might not have all function boundaries defined and in the event that we have mixed arm/thumb code in it, it will incorrectly mark arm code as thumb. This is why I wanted to be conservative here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68069