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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
