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

Fri Sep 27 01:12:56 PDT 2019

labath added a comment.

In D68069#1685146 <https://reviews.llvm.org/D68069#1685146>, @aadsm wrote:

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

Yeah, lit tests are often indirect to some degree. I think that's one of their main drawbacks (the lack of interactivity being the other). This needs to be balanced with their advantages. In this particular, case I think using the lit approach would be fine (though I haven't looked at the code to see how exactly this information is plumbed), as both disassembly and breakpoint should use the same source for determining the instruction set, but I am fine also fine with keeping the test like this.

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

The things you need are the arm target being built, and having lld checked out (so the test would need an annotation like `REQUIRES: lld, arm`). Not everyone has both of these things enabled (particularly having lld is less common), but they are things that one _can_ enable no matter what is his host or target architecture, so I am not worried by that. And as tests like these become more common, I expect more people will start having these enabled by default.

================
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?
----------------
aadsm wrote:
> 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.
That's true, but in the case we don't have all function symbols, we're down to guessing anyway. For all you know, treating the rest of code as thumb might actually be the right thing to do. (In fact it will almost certainly be correct at least for the next few instructions, since the entry point function is unlikely to be just one instruction long.)

The FDE parsing code actually does exactly the same thing you do here. It searches for possible function entry points, and creates symbols for those locations, if we don't have them already. Since it doesn't know the size of the those symbols it just sets `size_is_valid=false`, and lets it be auto-computed.

That's why I'm pushing to make this consistent with the FDE code, as adding this additional symbol actually improves the accuracy of the sizes computed for the FDE symbols. Likewise, if you set `size_is_valid=false` here, then any additional symbol-searching heuristics we introduce will improve the accuracy of the entry symbol size.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68069