No dangling warnings on gsl::pointer-type function parameter with the lifetimebound attribute

[hokein]

These are real cases, it would be nice to capture them in clang.

Case1:

class UrlAnalyzed {
  public:
    UrlAnalyzed(string_view url [[clang::lifetimebound]]);
};

std::string StrCat(std::string_view, std::string_view);

void test() {
    UrlAnalyzed url(StrCat("abc", "bcd")); // dangling!
}

Case2:

std::string_view ReturnStringView(std::string_view abc [[clang::lifetimebound]]);

void test() {
    std::string_view sv1 = ReturnStringView(StrCat("bar", "x")); // dangling
}

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Haojian Wu (hokein)

These are real cases, it would be nice to capture them in clang.

Case1:

class UrlAnalyzed {
  public:
    UrlAnalyzed(string_view url [[clang::lifetimebound]]);
};

std::string StrCat(std::string_view, std::string_view);

void test() {
    UrlAnalyzed url(StrCat("abc", "bcd")); // dangling!
}

Case2:

std::string_view ReturnStringView(std::string_view [[clang::lifetimebound]]);

void test() {
    std::string_view sv1 = ReturnStringView(absl::StrCat("bar", "x")); // dangling
}

[hokein]

These two cases can be captured with -Wdangling if we add the lifetimebound attr to string's string_view conversion operator.

The simplified version: https://godbolt.org/z/fE1WKexo7

[hokein]

One option is to add the lifetime attribute to libcxx.

The other option is to teach the gsl-specific code path (handleGslAnnotatedTypes) to respect the lifetimebound attribute.

[hokein]

@Xazax-hun @usx95 thoughts?

[Xazax-hun]

There are pros a cons to annotate the library vs injecting annotations on the fly. Annotating libcxx is the simplest and most elegant solution. Unfortunately, that does not help people using alternative STL implementations (like GCC's or Microsoft's).

I think if we want Clang to be as useful as possible, we'd need to inject these annotations into the standard library implementations, something similar to APINotes.

I suspect that APINotes are still not ready, so in the meantime I am not opposed to special casing this in the warning. But I think all those special casing should go away once we can reliably inject annotations. I think the injection/inference of annotations, and the checking logic being separate is the cleanest possible design here.

[usx95]

I feel gsl-analysis and lifetimebound are somewhat parallel analyses, and they could be more coherent.

It makes sense to me to make gsl-analysis respect lifetimebound. Currently, their interaction seems incoherent to me.
Needing to annotate the conversion operator from gsl::Owner to gsl::Pointer as lifetimebound seems like duplicate work because this seems to be the purpose of gsl-analysis (practically).

[hokein]

I think if we want Clang to be as useful as possible, we'd need to inject these annotations into the standard library implementations, something similar to APINotes.

Agree, and APINotes seems like a good fit.

I suspect that APINotes are still not ready, so in the meantime I am not opposed to special casing this in the warning.

If I understand you correctly, by special casing, do you mean we implicitly attach the lifetimebound attribute to string's conversion operator by default in Clang?

I feel gsl-analysis and lifetimebound are somewhat parallel analyses, and they could be more coherent.

I have a similar feeling about this.
However, in the current implementation, the GSL and lifetimebound analyses are orthogonal. I'm a bit nervous about the implications; the behavior might become harder to reason about.

One of the consequences is:

• After making this change, Clang will start diagnosing this case (with the -Wdangling-gsl), which is good.
• Later, when we add the lifetimebound attribute to the conversion operator, clang will emit a second diagnostic (-Wdangling) for this case as well, resulting in two duplicate warnings.

This is similar to the issue #93386.

[Xazax-hun]

If I understand you correctly, by special casing, do you mean we implicitly attach the lifetimebound attribute to string's conversion operator by default in Clang?

Yes.

However, in the current implementation, the GSL and lifetimebound analyses are orthogonal. I'm a bit nervous about the implications; the behavior might become harder to reason about.

I see a potential way out of this. Is it possible to describe the semantics of GSL annotations in terms of lifetimebound annotations? If that is the case, the processing of GSL annotations would be injecting lifetimebound according to a (hopefully) simple set of rules. This resolves the duplicate warnings, the duplicate code paths, the interactions between the two, and makes reasoning a bit easier.

[pkasting]

FWIW, this precise case came up in Chromium over the weekend. Even a stopgap of annotating constructors in libc++ would help us, since we use libc++ by default. I agree that it's not a perfect solution, but it would be preferable (for us) to doing nothing until the compiler introspects harder.