COIN-OR::LEMON - Graph Library

Opened 8 days ago

Last modified 39 hours ago

#634 new defect

Clang related fix

Reported by: Alpar Juttner Owned by: Alpar Juttner
Priority: critical Milestone: LEMON 1.4 release
Component: core Version: hg main
Keywords: Cc:
Revision id:

Description

Current clang version complains about the following function definition in concepts::Readmap.

      Value operator[](const Key &) const {
        return *(static_cast<Value *>(0)+1);
      }

The attached patch fixes it by changing to

        return Value();

But this means that ReadMap::Value must be default constructible. (Thus tests/map_tests.cc had to be changed accordingly.) Unfortunately I couldn't come up with any better solution.

Any idea?

Attachments (1)

clang-fix.patch (671 bytes) - added by Alpar Juttner 8 days ago.

Download all attachments as: .zip

Change History (10)

Changed 8 days ago by Alpar Juttner

Attachment: clang-fix.patch added

comment:1 Changed 8 days ago by Alpar Juttner

Priority: majorcritical

comment:2 Changed 7 days ago by Balazs Dezso

Maybe something like this:

Value operator[](const Key &) const {
  char mem[sizeof(Value)];
  for (std::size_t i = 0; i < sizeof(Value); ++i) {
    mem[i] = char(0);
  }
  return *(reinterpret_cast<Value *>(mem));
}

comment:3 Changed 7 days ago by Alpar Juttner

It seems that

return *(reinterpret_cast<Value *>(sizeof(Value)));

also works, and it may even be safer.

comment:4 in reply to:  3 ; Changed 4 days ago by Peter Kovacs

Replying to Alpar Juttner:

It seems that

return *(reinterpret_cast<Value *>(sizeof(Value)));

also works, and it may even be safer.

I don't understand the usage of sizeof here. Why is your suggestion better than

return *(reinterpret_cast<Value *>(0));

? Anyway, I even don't understand the +1 in the current code.

In fact, in other methods in maps.h, these two lines are used instead:

Value *r = 0;
return *r;

It seems to be equivalent to the line I suggested above. (Is there any difference?) I would prefer using the same solution for each similar method.

comment:5 in reply to:  4 Changed 3 days ago by Alpar Juttner

Replying to Peter Kovacs:

I don't understand the usage of sizeof here. Why is your suggestion better than

return *(reinterpret_cast<Value *>(0));

? Anyway, I even don't understand the +1 in the current code.

The reason is that compilers do not like referencing to NULL. Instead of an arbitrary nonzero value, sizeof(Value) --- or equivalently +1 --- is used to ensure that the value of the underlying pointer is properly aligned. That's why I think this is safer than the solution proposed by Balazs.

In fact, in other methods in maps.h, these two lines are used instead:

Value *r = 0;
return *r;

It seems to be equivalent to the line I suggested above. (Is there any difference?) I would prefer using the same solution for each similar method.

I believe, if you let the CLANG people know this, they will soon fix their compiler to fail on this too. So, yes, we should do the same here as above.

comment:6 Changed 3 days ago by Alpar Juttner

For the record, here is what the compilers say about the alternatives.

code GCC 9 debug GCC 9 optimized CLANG 9 debug CLANG 9 optimized
return *(static_cast<Value *>(0)); OK OK warning(1) warning(1)
return *(static_cast<Value *>(0)+1); OK OK warning(2) warning(2)
return *(static_cast<Value *>(sizeof(Value))); error(3) error(3) error(4) error(4)
return *(reinterpret_cast<Value *>(sizeof(0))); OK OK warning(1) warning(1)
return *(reinterpret_cast<Value *>(0)+1); OK OK warning(2) warning(2)
return *(reinterpret_cast<Value *>(sizeof(Value))); OK OK OK OK

(1)

warning: indirection of non-volatile null pointer will be deleted, not trap [-Wnull-dereference]
        return *(static_cast<Value *>(0));
               ^~~~~~~~~~~~~~~~~~~~~~~~~~

(2)

warning: performing pointer arithmetic on a null pointer has undefined behavior if the offset is nonzero [-Wnull-pointer-arithmetic]
        return *(static_cast<Value *>(0)+1);
                 ~~~~~~~~~~~~~~~~~~~~~~~^

(3)

error: invalid static_cast from type ‘long unsigned int’ to type ‘lemon::concepts::ReadMap<lemon::concepts::Digraph::Arc, int>::Value*’ {aka ‘int*’}
   54 |         return *(static_cast<Value *>(sizeof(Value)));
      |                 ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

(4)

error: cannot cast from type 'unsigned long' to pointer type 'lemon::concepts::ReadMap<lemon::concepts::Digraph::Arc, int>::Value *' (aka 'int *')
        return *(static_cast<Value *>(sizeof(Value)));
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

comment:7 Changed 44 hours ago by Peter Kovacs

I see, thank you for the info.

However, I still don't like this solution too much:

return *(reinterpret_cast<Value *>(sizeof(Value)));

because sizeof(Value) is only used for getting a non-zero arbitrary integer value. Couldn't we use a simple constant instead of it, e.g. 42? Or would it trigger another warning?

Among the alternatives listed so far, I still prefer the one we use in the other methods, although it is 2 lines, not only 1.

Anyway, these methods in concept classes need not be run, only compiled. So what about throwing an exception instead of such pointer tricks?

comment:8 in reply to:  7 ; Changed 40 hours ago by Alpar Juttner

However, I still don't like this solution too much [...] because sizeof(Value) is only used for getting a non-zero arbitrary integer value. Couldn't we use a simple constant instead of it, e.g. 42? Or would it trigger another warning?

As I explained, sizeof(Value) is not just an arbitrary value. It is used because is ensures the proper alignment of the underlying pointer.

Among the alternatives listed so far, I still prefer the one we use in the other methods, although it is 2 lines, not only 1.

Currently it compiles, but I'm sure newer CLANG versions will soon recognize that it is the same as

return *(reinterpret_cast<Value *>(0));

and will complain about it.

Anyway, these methods in concept classes need not be run, only compiled. So what about throwing an exception instead of such pointer tricks?

Good idea. It may work. Hopefully it wont complain that a non void function has no return.

On the other hand, we must be careful, because - if I remember correctly - some codes do call concept checking functions directly.

comment:9 in reply to:  8 Changed 39 hours ago by Peter Kovacs

Replying to Alpar Juttner:

As I explained, sizeof(Value) is not just an arbitrary value. It is used because is ensures the proper alignment of the underlying pointer.

OK, I understand now and accept your solution.

On the other hand, we must be careful, because - if I remember correctly - some codes do call concept checking functions directly.

If these methods are actually run (not only compiled), then none of the suggested solutions seem to be safe. Throwing an exception at least makes the issue explicit and clear (instead of a "nice" segmentation fault). Provided that compilers do not give warnings for that.

Note: See TracTickets for help on using tickets.