Page MenuHomeFreeBSD

Add ctfdiff
Needs ReviewPublic

Authored by aokblast on Dec 25 2024, 1:29 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 9, 7:33 AM
Unknown Object (File)
Mon, Sep 29, 11:51 PM
Unknown Object (File)
Sat, Sep 20, 4:29 AM
Unknown Object (File)
Sep 12 2025, 2:25 PM
Unknown Object (File)
Sep 10 2025, 9:09 PM
Unknown Object (File)
Aug 31 2025, 12:54 AM
Unknown Object (File)
Aug 28 2025, 3:40 PM
Unknown Object (File)
Aug 28 2025, 1:33 PM
Subscribers

Details

Summary

Wiki Page on introducing how to use this tool: https://wiki.freebsd.org/ShengYiHong/ABIStability

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 61332
Build 58216: arc lint + arc unit

Event Timeline

Remove unused CTF in Makefile

The elf parse, ctf parser is all from ctfdump and refactor it into C++. However, because it is cddl licensed, I am wondering if there is any legal concern?

usr.bin/ctfdiff/ctfdiff.1
8

Should the copyright header be yours?

The elf parse, ctf parser is all from ctfdump and refactor it into C++. However, because it is cddl licensed, I am wondering if there is any legal concern?

So not only included the CDDL headers but also some code derived from CDDL files?

cc @imp

The elf parse, ctf parser is all from ctfdump and refactor it into C++. However, because it is cddl licensed, I am wondering if there is any legal concern?

So not only included the CDDL headers but also some code derived from CDDL files?

cc @imp

Oh! I'm sorry, but this got lost in my inbox... I've been super busy...

So, it's hard to say. At the very least ctftype.cc needs some kind of acknowledgement that the code was derived from the original CDDL code.
The wrapping in C++ coupled with refactoring suggests that it may have reached the threshold to be a new, transformative work. However, since we're
linking with other CDDL code, I'd err on the side of considering it merely a derived work. This would mean labeling with CDDL and possibly moving
this utility to the cddl subtree.

The files generally need a copyright statement, so I'd start with where you got the elf parser from for those files that contain it (ctftypes.cc and ctfdata.cc
if my eye is right). I'd then add your copyright to that, and add the copyright + license statement to each of the other files. I suspect they should be CDDL
for simplicity, but BSDL 2-Clause is also a viable option.

I left a couple of comments about a few things that stood out in the first pass, I will likely have more as I dive into the internals.

I do have a feature-related question - would it be possible to add a way of diffing individual types/functions/objects?
e.g. something along the lines of:

$ ctfdiff -t <type_name> file1 file2

It would make diagnosing and fixing the breakages easier IMO.

Another thing I've noticed is that you've added support for CTFv2. I would personally be in favor of removing CTFv2 support to simplify the implementation since both ctfconvert and ctfmerge emit CTFv3 (and I'm not aware any in-tree users of CTFv2), but I won't insist.

usr.bin/ctfdiff/ctfdata.cc
44

would it be possible to switch to a string_view here?

80

This also applies to other uses of std::cout in this function.

123

This also applies to other uses of std::cout in this function.

153–159
260

Please #define this number.

261

This also applies to other uses of std::cout in this function

710–713
usr.bin/ctfdiff/ctfdiff.cc
24–28

No need to wrap this in a function since it's called only once.

33

The nullptr checks below can be replaced std::string_view::empty().

usr.bin/ctfdiff/ctftype.cc
258–259
usr.bin/ctfdiff/metadata.cc
100
109
usr.bin/ctfdiff/ctfdiff.1
24

You can use the SPDX short form as shown in style(9)