See D111358 for post-commit review comments; I've answered the questions and comments there.
This review has the minor code updates, and replaces the asm/object-file test with a yaml equivalent.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml | ||
|---|---|---|
| 10 | I might be missing something obvious, but what's the point of this echo? Can't the object be passed directly on the command-line? | |
| 54–57 | How important are these fields for the tool? Are they actually needed, or could you get away without them for the purpose of this test? (I believe the Link: .dynstr is implicit, so you should be able to omit that regardless) Same question for the other sections. | |
| 71–74 | Are the Value and Type fields important to the tool? Also, do you need this many symbols (it's fine if you do, just is a lot!)? | |
| llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp | ||
| 107–108 | Is this check strictly appropriate?
| |
| 170–171 | If the tool can take more than one input (I haven't checked if it can), it might be wise to include the filename in this message. | |
| llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml | ||
|---|---|---|
| 10 | The point is to test that response files work, a feature that is described in the help text. | |
| 54–57 | I got to this point by doing obj2yaml on one of the previous .so files and then deleting things that were clearly irrelevant and didn't make the test fail. I have no idea which fields/sections are necessary and there was a limit to how much fiddling I was willing to do. If you have guidance in this respect I'd be happy to reduce it further. | |
| 71–74 | The tool checks Type and Binding. I have no idea whether the other fields matter; they came with the obj2yaml output. If you tell me I can delete Section and Value, I'm okay to do that. I do need all of these symbols, because the object file has to define exactly the same set of symbols that TLI expects to find for this target. | |
| llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp | ||
| 107–108 | These are the mangling prefixes used in the list of functions known to TLI, and the mangled names are all for C++ operators. If there will be Rust or D-specific library functions added to TLI, then yes this function would need to be updated. It's not intended to work for arbitrary functions, just the ones that TLI knows about. The idea was to avoid the relatively expensive demangling and string comparison for names that wouldn't need it; but as the printable name isn't used that much, I can recode this to do the trial demangle and string compare. | |
Comment Actions
Remove more unnecessary sections/fields from yaml file.
Report filename in a warning message.
Unconditionally demangle function name for future-proofing.
| llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml | ||
|---|---|---|
| 10 | Ah, fair enough. Leave it in, but perhaps add a comment to say that that's what this is testing specifically (it looked to me like this was just trying to workaround something by using a response file). | |
| 54–57 | I'm not familiar with what the tool looks at, so being able to point out which symbols are needed is a little tricky. We've tried to minimise unnecessary obj2yaml output, but it's not always possible to get it to the bare minimum required for a specific piece of code. | |
| 71–74 | Removing the Section field makes the symbols undefined. That may or may not be an issue, but at a guess, you don't want your symbols to be undefined, and such symbols may not want to be considered "in the SDK" for the purposes of the tool, right? | |
| llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp | ||
| 107–108 |
I guess the expensive bit is really checking whether the output is the same as the input, since the demangling will fail as fast as those checks would have done. Perhaps we need a new demangle signature that indicates whether demangling has occurred. Probably only worth it if there is a performance concern though. | |
| 170–171 | Usual format is "warning: file name: message" (see what FileError does for a comparison). | |
| llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml | ||
|---|---|---|
| 71–74 | It never occurred to me that Dynamic Symbols could be undefined, but no I suppose they shouldn't be considered as "in the SDK." I'll have to do something about that. | |
| llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp | ||
| 107–108 | This is the simplest of several demangling APIs. A few hundred string compares probably aren't a performance concern though, in this context. | |
| llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml | ||
|---|---|---|
| 10 | Added a comment here, and a few other places to better describe the testing. | |
| 54–57 | I was actually asking about fields, not symbols, but I think it's as minimal as it can get at this point. | |
| llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp | ||
| 170–171 | Updated to put filename first, here and elsewhere. | |
| llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml | ||
|---|---|---|
| 54–57 | Yes, looks pretty minimal now, since we need lots of symbols. | |
| 71–74 | FYI, undefined dynamic symbols are imports. | |
| llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp | ||
| 170–171 | Is this warning message tested anywhere? I don't see it immediately, and I'd expect test failures if it were tested elsewhere due to the format change. Same goes for the other warnings/errors reported in this file. | |
| 238 | I'd be tempted to use plain English here, rather than class names to describe this situation, as end users don't know about the classes, i.e. "not an archive or object file" | |
| llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp | ||
|---|---|---|
| 170–171 | None of the malformed-input messages are tested. | |
Comment Actions
All looks good to me. I see there are a couple of @MaskRay's comments that you chose not to address (with reasons described in the original patch), but you might want to give him a chance to check before pushing this.
I might be missing something obvious, but what's the point of this echo? Can't the object be passed directly on the command-line?