-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to specify a script's license #3157
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -607,14 +607,12 @@ CallInst *IRBuilderBPF::CreateProbeReadStr(Value *ctx, | |
// int bpf_probe_read_str(void *dst, int size, const void *unsafe_ptr) | ||
FunctionType *probereadstr_func_type = FunctionType::get( | ||
getInt64Ty(), { dst->getType(), getInt32Ty(), src->getType() }, false); | ||
PointerType *probereadstr_func_ptr_type = PointerType::get( | ||
probereadstr_func_type, 0); | ||
Constant *probereadstr_callee = ConstantExpr::getCast( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I might have missed it but what is this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, you're right this could have done with an explanation. It's to produce more useful error messages for the
Without this change:
With this change:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah nice. Thanks for the explanation. |
||
Instruction::IntToPtr, getInt64(read_fn), probereadstr_func_ptr_type); | ||
CallInst *call = createCall(probereadstr_func_type, | ||
probereadstr_callee, | ||
{ dst, size_i32, src }, | ||
probeReadHelperName(read_fn)); | ||
|
||
CallInst *call = CreateHelperCall(read_fn, | ||
probereadstr_func_type, | ||
{ dst, size_i32, src }, | ||
probeReadHelperName(read_fn), | ||
&loc); | ||
CreateHelperErrorCond(ctx, call, read_fn, loc); | ||
return call; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,12 +96,16 @@ CodegenLLVM::CodegenLLVM(Node *root, BPFtrace &bpftrace) | |
llvm::DEBUG_METADATA_VERSION); | ||
|
||
// Set license of BPF programs | ||
const std::string license = "GPL"; | ||
const auto &license = bpftrace_.get_license(); | ||
auto license_var = llvm::dyn_cast<GlobalVariable>(module_->getOrInsertGlobal( | ||
"LICENSE", ArrayType::get(b_.getInt8Ty(), license.size() + 1))); | ||
license_var->setInitializer( | ||
ConstantDataArray::getString(module_->getContext(), license.c_str())); | ||
license_var->setSection("license"); | ||
|
||
// XXX: Set licenses for manually loaded programs. Remove once libbpf is used | ||
jordalgo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// instead: | ||
bpftrace_.resources.license = license; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know it's just temporary but this doesn't feel like a good place to set this. Why not do it in |
||
} | ||
|
||
void CodegenLLVM::visit(Integer &integer) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ Config::Config(bool has_cmd, bool bt_verbose) : bt_verbose_(bt_verbose) | |
config_map_ = { | ||
{ ConfigKeyBool::cpp_demangle, { .value = true } }, | ||
{ ConfigKeyBool::lazy_symbolication, { .value = false } }, | ||
{ ConfigKeyString::license, { .value = std::nullopt } }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit strange to me. The default license is GPL but we set the default config value to none here and only return "GPL" from If the only reason is that we don't have a config priority defined for config value coming from a script comment header (SPDX here), then I believe that we should introduce that. It will also save the introduction of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Thank you @viktormalik for articulating this. I also didn't understand why we didn't want to set the default in the config. |
||
{ ConfigKeyInt::log_size, { .value = (uint64_t)1000000 } }, | ||
{ ConfigKeyInt::max_bpf_progs, { .value = (uint64_t)512 } }, | ||
{ ConfigKeyInt::max_cat_bytes, { .value = (uint64_t)10240 } }, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,31 @@ extern bpftrace::location loc; | |
|
||
namespace bpftrace { | ||
|
||
namespace { | ||
std::string_view parse_license() | ||
{ | ||
std::string_view program = Log::get().get_source(); | ||
constexpr std::string_view spdx_id_marker = "SPDX-License-Identifier: "; | ||
auto spdx_id_pos = program.find(spdx_id_marker); | ||
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we ensure this is inside a comment? Seems like this would match this string anywhere in the file |
||
if (spdx_id_pos == program.npos) | ||
return {}; | ||
|
||
auto license_pos = spdx_id_pos + spdx_id_marker.size(); | ||
auto eol_pos = program.find("\n", spdx_id_pos + 1); | ||
if (eol_pos == program.npos) | ||
return {}; | ||
|
||
auto license = program.substr(license_pos, eol_pos - license_pos); | ||
|
||
// Try to translate some known GPL-v2-compatible licenses into a format | ||
// understood by the Linux kernel | ||
if (license.find("GPL-2.0") == 0) | ||
ajor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return "GPL"; | ||
jordalgo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return license; | ||
} | ||
} // namespace | ||
|
||
Driver::Driver(BPFtrace &bpftrace, std::ostream &o) | ||
: bpftrace_(bpftrace), out_(o) | ||
{ | ||
|
@@ -31,6 +56,12 @@ int Driver::parse_str(std::string_view script) | |
|
||
int Driver::parse() | ||
{ | ||
if (auto license = parse_license(); !license.empty()) { | ||
LOG(V1) << "Found license from SPDX ID: " << license; | ||
ConfigSetter config_setter(bpftrace_.config_, ConfigSource::default_); | ||
config_setter.set(ConfigKeyString::license, std::string{ license }); | ||
} | ||
|
||
// Reset previous state if we parse more than once | ||
root.reset(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This language feels a little weird to me. How about "If a license is not explicitly declared, bpftrace will license the script as GPL v2."