RFR: 8255416: Investigate err_msg to detect unnecessary uses [v2]

[Thomas Stuefe]

On Wed, 28 Oct 2020 21:53:19 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>> src/hotspot/share/utilities/formatBuffer.hpp line 124:
>> 
>>> 122:   // If compilation fails because of ambiguity between this and real constructor, you
>>> 123:   // could drop err_msg use at all.
>>> 124:   inline FormatErrBuffer(const char* msg) { ShouldNotReachHere(); }
>> 
>> I do not think this is a good idea (apart from it being too complex for a not that serious issue).
>> 
>> The asserts fire, of course, only at runtime. But this function is used usually in some error context, as part of of error reporting. I do not think our tests cover all those paths.
>> 
>> Either somehow make this a compile time error or just leave it as it is. We also could, since this buffer object is used usually as input for vm_exit_during_initialization(), give that function a var-arg overload.
>
> Actually, this code prevents a single string argument in compile time. It relies on two constructors to introduce ambiguity in overload resolution that makes C++ compiler complain and abort compilation. Comment above the dummy constructor should clarify that for anyone stepping on compile error.
>   inline FormatErrBuffer(const char* format, ...) ATTRIBUTE_PRINTF(2, 3);
>   inline FormatErrBuffer(const char* msg) { ShouldNotReachHere(); }
> For sanity check, I've used this sample code https://godbolt.org/z/szs6rE
> 
> And the cases that were fixed have been detected by the compiler.
> 
> I don't think that this problem is a serious issue as well. But as for me, it is a minor code complexity increase to ensure that the minor problem of extra string copy will never appear again.

Oh, now I see it. Smart.

But it occurred to me that someone may want to start a err_msg buffer up with a string literal, only to then add additional content via FormatBuffer::append(). So initializing it with a literal and no arguments may be a valid usecase. 

So I'd still prefer keeping this out. Alternatively, if you like to keep it in, could we just not implement the second constructor? Which should give us linker errors if the static check fails.

>> src/hotspot/share/gc/shenandoah/mode/shenandoahMode.hpp line 44:
>> 
>>> 42:     if ((name)) {                                                           \
>>> 43:       const char* msg = "GC mode needs -XX:-" #name " to work correctly";   \
>>> 44:       vm_exit_during_initialization("Error", msg);                          \
>> 
>> Same as above, can be inlined into one call. No need for the temporary variable.
>
> Please see my comment in the thread above.

Whatever you decide with Alexey is fine. The temporary variable is my least favorite option.

-------------

PR: https://git.openjdk.java.net/jdk/pull/905