Skip to content

Refactor clang extra args addition #1531

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 24, 2020
Merged

Refactor clang extra args addition #1531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 40 additions & 47 deletions lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
#include "clang/Driver/Driver.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSet.h"
Copy link
Author

@DeeSee DeeSee Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrian-prantl I think I need some help here.

I get the following error when compiling:

path/to/swift_workspace/llvm-project/lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp:1476:9: error: no type named 'StringSet' in namespace 'llvm'; did you mean 'StringRef'?
  llvm::StringSet unique_flags;
  ~~~~~~^~~~~~~~~
        StringRef

I've triple-checked everything, tried to replace this import with invalid one to make sure I'm editing the right file, cleaned build folder and now I've run out of ideas :(

Copy link

@adrian-prantl adrian-prantl Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringSet is relatively new. Does it actually exist on the branch you are compiling on?

Copy link

@adrian-prantl adrian-prantl Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise you can use StringMap<bool> on master and StringSet on master-next and master-rebranch

Copy link

@adrian-prantl adrian-prantl Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah: http://llvm.org/doxygen/StringSet_8h_source.html
I think it's StringSet<>.

Copy link
Author

@DeeSee DeeSee Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This was the issue. Didn't know that when all template args are defaulted you still need to use template marks :(

Now I can confirm that it's compiling, is it possible to run address sanitizer on this branch?

Copy link

@adrian-prantl adrian-prantl Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no way to run the Asan Jenkins job on a PR, but you can run it locally with the --enable-asan (and --enable-ubsan) build-script options.

Copy link
Author

@DeeSee DeeSee Jul 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've run ./swift/utils/build-script -r --lldb --enable-asan --enable-ubsan locally and it seems to finish successfully

#include "llvm/CodeGen/TargetSubtargetInfo.h"
#include "llvm/IR/DataLayout.h"
#include "llvm/IR/LLVMContext.h"
Expand Down Expand Up @@ -1448,39 +1449,66 @@ void ApplyWorkingDir(SmallString &clang_argument, StringRef cur_working_dir) {
llvm::sys::path::append(clang_argument, cur_working_dir, rel_path);
llvm::sys::path::remove_dots(clang_argument);
}

std::array<StringRef, 2> macro_flags = { "-D", "-U" };

bool IsMultiArgClangFlag(StringRef arg) {
for (auto &flag : macro_flags)
if (flag == arg)
return true;
return arg == "-working-directory";
}

bool IsMacroDefinition(StringRef arg) {
for (auto &flag : macro_flags)
if (arg.startswith(flag))
return true;
return false;
}

bool ShouldUnique(StringRef arg) {
return IsMacroDefinition(arg);
}
} // namespace

void SwiftASTContext::AddExtraClangArgs(std::vector<std::string> ExtraArgs) {
swift::ClangImporterOptions &importer_options = GetClangImporterOptions();
llvm::StringSet<> unique_flags;
for (auto &arg : importer_options.ExtraArgs)
unique_flags.insert(arg);

llvm::SmallString<128> cur_working_dir;
llvm::SmallString<128> clang_argument;
for (const std::string &arg : ExtraArgs) {
// Join multi-arg -D and -U options for uniquing.
// Join multi-arg options for uniquing.
clang_argument += arg;
if (clang_argument == "-D" || clang_argument == "-U" ||
clang_argument == "-working-directory")
if (IsMultiArgClangFlag(clang_argument))
continue;

auto clear_arg = llvm::make_scope_exit([&] { clang_argument.clear(); });

// Enable uniquing for -D and -U options.
bool is_macro = (clang_argument.size() >= 2 && clang_argument[0] == '-' &&
(clang_argument[1] == 'D' || clang_argument[1] == 'U'));
bool unique = is_macro;

// Consume any -working-directory arguments.
StringRef cwd(clang_argument);
if (cwd.consume_front("-working-directory")) {
cur_working_dir = cwd;
continue;
}
// Drop -Werror; it would only cause trouble in the debugger.
if (clang_argument.startswith("-Werror")) {
if (clang_argument.startswith("-Werror"))
continue;
}

if (clang_argument.empty())
continue;

// Otherwise add the argument to the list.
if (!is_macro)
if (!IsMacroDefinition(clang_argument))
ApplyWorkingDir(clang_argument, cur_working_dir);
AddClangArgument(clang_argument.str(), unique);

auto clang_arg_str = clang_argument.str();
if (!ShouldUnique(clang_argument) || !unique_flags.count(clang_arg_str)) {
importer_options.ExtraArgs.push_back(clang_arg_str);
unique_flags.insert(clang_arg_str);
}
}
}

Expand Down Expand Up @@ -3387,41 +3415,6 @@ swift::DWARFImporterDelegate *SwiftASTContext::GetDWARFImporterDelegate() {
return m_dwarf_importer_delegate_up.get();
}

bool SwiftASTContext::AddClangArgument(std::string clang_arg, bool unique) {
if (clang_arg.empty())
return false;

swift::ClangImporterOptions &importer_options = GetClangImporterOptions();
// Avoid inserting the same option twice.
if (unique)
for (std::string &arg : importer_options.ExtraArgs)
if (arg == clang_arg)
return false;

importer_options.ExtraArgs.push_back(clang_arg);
return true;
}

bool SwiftASTContext::AddClangArgumentPair(StringRef clang_arg_1,
StringRef clang_arg_2) {
if (clang_arg_1.empty() || clang_arg_2.empty())
return false;

swift::ClangImporterOptions &importer_options = GetClangImporterOptions();
bool add_hmap = true;
for (ssize_t ai = 0, ae = importer_options.ExtraArgs.size() -
1; // -1 because we look at the next one too
ai < ae; ++ai) {
if (clang_arg_1.equals(importer_options.ExtraArgs[ai]) &&
clang_arg_2.equals(importer_options.ExtraArgs[ai + 1]))
return false;
}

importer_options.ExtraArgs.push_back(clang_arg_1);
importer_options.ExtraArgs.push_back(clang_arg_2);
return true;
}

const swift::SearchPathOptions *SwiftASTContext::GetSearchPathOptions() const {
VALID_OR_RETURN(0);

Expand Down
4 changes: 0 additions & 4 deletions lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,6 @@ class SwiftASTContext : public TypeSystemSwift {

bool AddModuleSearchPath(llvm::StringRef path);

bool AddClangArgument(std::string arg, bool unique = true);

bool AddClangArgumentPair(llvm::StringRef arg1, llvm::StringRef arg2);

/// Add a list of Clang arguments to the ClangImporter options and
/// apply the working directory to any relative paths.
void AddExtraClangArgs(std::vector<std::string> ExtraArgs);
Expand Down