This patch introduces a new tool, llvm-tapi-diff, that compares and returns the diff of two TBD files.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
llvm/include/llvm/TextAPI/Target.h | ||
---|---|---|
63 | nit: s/getTargetTriple/getTripleName or getTargetTripleName | |
llvm/tools/llvm-tapi-diff/DiffEngine.h | ||
132 | I'd prefer this emit an empty string | |
llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp | ||
2 | Please update these header descriptions | |
79 | This path should never be possible, right? Could we instead error out if Bin* can't be casted & early return. and have this be return Engine.compareFiles() |
llvm/include/llvm/Object/TapiUniversal.h | ||
---|---|---|
104 | It looks like the TapiUniversal guarantees that ParsedFile isn't NULL and indeed, some other functions seems to be unconditionally dereferencing this member. Should this return function a (const) reference instead? | |
llvm/include/llvm/TextAPI/Target.h | ||
63 | You're passing Target by value, so this is going to make a copy. Is that intentional? Should this take a const Target& instead? | |
llvm/lib/TextAPI/Platform.cpp | ||
117 | GCC used to complain about a missing return even after an exhaustive switch. Many places add an llvm_unreachable at the end of the function to avoid that. | |
llvm/test/tools/llvm-tapi-diff/tapi-diff-mismatched-number-of-inlines.test | ||
1 | The not binary is all lowercase. I suspect this will fail when running the test on a case-sensitive file system. | |
llvm/tools/llvm-tapi-diff/DiffEngine.cpp | ||
24–26 | I was under the impression std::equal already ensured that the two ranges are of equal length. Is this meant as an optimization to quickly discard cases where the number of element are different? If so, keep in mind that for some iterator types like forward iterators, this is going to be O(n). Unless you know this is only going to be called with a random access iterator for which this operation is O(1) this might actually hurt performance. | |
31 | T, U and V are pretty cryptic. It looks like U is some sort of TargetVec type, so maybe call it TargetVecT. | |
llvm/tools/llvm-tapi-diff/DiffEngine.h | ||
23–42 | The enums and classes in this header could use a Doxygen comment explaining what they are used for. To me it's not clear from their name what their purpose is. | |
185 | Above you use LHS and RHS. Is there a reason to use One and Two here? Personally I'd stick with the former. | |
llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp | ||
32 | Please use WithColor::error() for consistency with other tools (and color output :-) | |
42 | Instead of dumping the error here, why not return it and let the caller deal with it? Below there's a call to convertFileToBinary and without checking the implementation it looks like it's lacking error handling. | |
66 | Some/most llvm tools use EXIT_SUCCESS and EXIT_FAILURE, which make it much easier to quickly spot exit codes. |
changed getTargetTriple to getTargetTripleName; fixed header descriptions; fixed unreachable path; return const reference from getInterfaceFile; passing Target as const reference into getTargetTripleName; added llvm_unreachable to getOSAndEnvironmentName; NOT -> not in tests; removed unnecessary comparison in checkSymbolEquality; refactored typenames for addValToDiffVec template function; added Doxygen comments for DiffEngine.h; One and Two => LHS and RHS in llvm-tapi-diff; added WithColor; return llvm::Error or std::unique_ptr<Binary> for convertFileToBinary instead of only dumping error; used EXIT_FAILURE instead of returning 1;
llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp | ||
---|---|---|
2 | we should also replace symbol table comparator with something like (tapi|tbd) file comparator | |
12 | s/llvm-nm/llvm-tapi-diff | |
80 | Please add a testcase that goes through an error path. bin/llvm-tapi-diff: error: src/llvm-project/llvm/test/tools/llvm-tapi-diff/Inputs/vC.tbd: No such file or directory Expected<T> must be checked before access or destruction. Unchecked Expected<T> contained error: No such file or directoryPLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace. Stack dump: 0. Program arguments: bin/llvm-tapi-diff src/llvm-project/llvm/test/tools/llvm-tapi-diff/Inputs/v4B.tbd src/llvm-project/llvm/test/tools/llvm-tapi-diff/Inputs/vC.tbd Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it): 0 llvm-tapi-diff 0x00000001084d4dd7 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 39 1 llvm-tapi-diff 0x00000001084d3ff8 llvm::sys::RunSignalHandlers() + 248 <omitted> [1] 85533 abort bin/llvm-tapi-diff See https://llvm.org/docs/ProgrammersManual.html#error-handling for more details. |
updated header descriptions, updated error handling for convertFileToBinary and if a file cannot be cast as TapiUniversal.
llvm/lib/TextAPI/Platform.cpp | ||
---|---|---|
118 | Should this be llvm::MachO::PlatformKind? | |
llvm/tools/llvm-tapi-diff/DiffEngine.cpp | ||
2 | This seems to be missing a few dashes. | |
221–222 | This is still using 1 and 2 while everything else is now using LHS and RHS. | |
325–327 | The llvm style guidelines say no braces around single-line ifs. | |
llvm/tools/llvm-tapi-diff/DiffEngine.h | ||
49–59 | Rather than documenting the class as a whole, it's good practice to document the individual members like this. This is much more maintainable when a member gets added or removed, and Doxygen will associate the comment with the variable for you, which means you don't have to repeat the type etc. | |
55–57 | Should these members really be public? If the answer is yes, you'd want to use struct instead of class, which has public visibility by default. It's not so much about avoiding the access specifier on line 53, but more about conveying your intention to other developers. | |
147 | This probably also needs an unreachable. Same for the function below. There might be more in this patch. | |
202 | Several files seem to be missing newlines at the end. |
llvm.MachO.PlatformKind => llvm::MachO::PlatformKind in platform.cpp; removed 1 and 2 in favor of LHS and RHS in DiffEngine.cpp; refactored comments in DiffEngine.h; changed DiffOutput from class to struct; added llvm_unreachable in switch statements in DiffEngine.h; Added new lines at end of files.
llvm/tools/llvm-tapi-diff/DiffEngine.cpp | ||
---|---|---|
10–11 | This is not a header tho :-) | |
93–96 | Could we reduce the density of this code by adding a helper that looks something like: void DiffAttribute(StringRef Name, std::vector<DiffOutput>& Output, AttributeDiff Attr) { Output.push_back(getSingleAttrDiff(Attr, Name)); } and then simplify these calls to: DiffAttribute("Install Name", ReturnedOutput, DiffScalarVal<StringRef, AD_Diff_Scalar_Str>(Order, Interface->getInstallName())); Having the name of the attribute first makes it clear what you're diffing. I'd probably also rename ReturnedOutput to something shorter (Output) to reduce the length of those calls. | |
llvm/tools/llvm-tapi-diff/DiffEngine.h | ||
65–66 | Can these two be private? These members look like implementation details. Same in the other classes. | |
73 | I see (this->Order == lhs) ? "< " : "> ") in several places. How about creating a helper that takes a InterfaceInputOrder and returns a StringRef to avoid this duplication? | |
133 | Why are these functions implemented in the header? | |
172 | The LLVM Coding Guide says that comments should be sentences, i.e. capitalized and with a period at the end. | |
223 | You're passing the vector by value, so this will copy all its element for printing. Can this be a const ref? |
llvm/tools/llvm-tapi-diff/DiffEngine.cpp | ||
---|---|---|
129 | you don't need this conditional, the body of the for loop would never run if its empty | |
172 | These don't need to be scoped | |
llvm/tools/llvm-tapi-diff/DiffEngine.h | ||
57 | Most, if not all, instances of this type are named Diff Diff.DiffVec is redundant and is also used as a variable name in other places. | |
82 | nit: this-> calls are unecessarcy | |
102 | Remove the typo at the end. | |
llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp | ||
2 | sorry for the confusion on notation, I mean either tapi or tbd should be in the description. |
Hey, just a few notes on proper error handling.
llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp | ||
---|---|---|
41 | The idiomatic solution for this is ExitOnErr as described here: | |
53 | Could the result be returned directly here? Like this? return createBinary(BufferOrErr.get()->getMemBufferRef()); | |
74 | I think with ExitOnError this could be: std::unique_ptr<Binary> BinLHS = ExitOnErr(convertFileToBinary(InputFileNameLHS)); It is not uncommon to have it as a static global in a tool's driver cpp and use it directly all over the place. There is a number of good examples, e.g. llvm-link https://github.com/llvm/llvm-project/blob/81bc732816107f6aff4fd61980f7b03cc92332b5/llvm/tools/llvm-link/llvm-link.cpp#L296 |
fixed heading on DiffEngine.cpp; made helper to collect single scalar values; ReturnedOutput => Output; made implementation details private for class variables; made helper to set order indicator for print functions; moved functions that did not need to be in header; removed unneeded conditional; removed unneeded scope; DiffVec => Values, TargValues, DocValues; addValToDiffVec => addDiffToTargetSlice; removed unnecessary this-> calls; removed typo in comment; fixed heading on llvm-tapi-diff.cpp; fixed error handling in llvm-tapi-diff.cpp
A few small nits and this should be ready to go.
llvm/tools/llvm-tapi-diff/DiffEngine.cpp | ||
---|---|---|
384–387 | LHS? DocLHS? | |
458 | This line got me thinking that it might be nice to make the raw_ostream an argument to printDifferences and do the same for compareFiles below. That aligns nicely with LLVM's library design, where the actual implementation (the DiffEngine) can remain unaware of where its output is being printed while centralize the fact that it's going to stdout in llvm-tapi-diff.cpp. | |
llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp | ||
30–33 | This is generally lowercase. |
Doc1 => DocLHS; Doc2 => DocRHS; made raw_ostream argument in compareFiles and printDifferences; lowercase argument prompts;
llvm/test/tools/llvm-tapi-diff/tapi-diff-matching-tbd.test | ||
---|---|---|
2 | Shouldn't you add llvm-tapi-diff to llvm/test/CMakeLists.txt to make sure it gets built when you build check-llvm? And to the tools.extend( bit in llvm/test/lit.cfg.py? |
llvm/tools/llvm-tapi-diff/DiffEngine.h | ||
---|---|---|
58 |
Storing a subclass of AttributeDiff into a unique_ptr<AttributeDiff> is not OK, because AttributeDiff does not have a virtual destructor. So, this tool is invoking UB. Just adding the virtual destructor to AttributeDiff is probably ok -- there doesn't seem to be any reason to avoid using virtual dispatch in this tool's class hierarchy, afaict. |
The tests for this tool are observed to be failing on multiple big-endian platforms:
clang-s390x-linux: https://lab.llvm.org/buildbot/#/builders/94/builds/4116
clang-ppc64be-linux: https://lab.llvm.org/buildbot/#/builders/52/builds/8035
Thanks for letting us know! @spowell from reading the lit output, it appears like the output order for the target slices isn't stable. I've reverted while we investigate and fix.
I'm pretty sure this also causes a build failure on older, but supported versions of GCC due to this bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56480):
../tools/llvm-tapi-diff/DiffEngine.cpp:35:71: error: specialization of template<class T, llvm::DiffAttrKind U> void llvm::DiffScalarVal<T, U>::print(llvm::raw_ostream&, std::__cxx11::string) in different namespace [-fpermissive] std::string Indent) { ^
It looks like the TapiUniversal guarantees that ParsedFile isn't NULL and indeed, some other functions seems to be unconditionally dereferencing this member. Should this return function a (const) reference instead?